Skip to content

Support nvmrc pseudo version #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

AdrieanKhisbe
Copy link

Hello @ehmicky ,

I was looking to add a specifc feature to your awesome nve, and end up here, after passinig by nvexeca 🙂

So here is the Proposal:

  • Problem addressed: with nve, running on the .nvmrc version of project and others, without having to explicitly say which exact my .nvmrc is
  • Related Prs: no pr opened
  • Solution proposed: during option resolving, intercept early on nvmrc pseudo variable, and replace it by the version range stored in .nvmrc (using find-up)
  • Checklist
    • I have read the contribution guidelines.
    • I have added tests (we are enforcing 100% test coverage).
    • I have added documentation in the README.md, the docs directory (if
      any) and the examples directory (if any).
    • The status checks are successful (continuous integration). Those can be
      seen below. (github actions are greenish on my rep, they failed at coverage report)

🙂

@codecov
Copy link

codecov bot commented Mar 31, 2020

Codecov Report

Merging #4 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
src/options.js 100.00% <100.00%> (ø)
Flag Coverage Δ
#linux 83.49% <100.00%> (+0.55%) ⬆️
#mac ?
#node_v10_17_0 83.49% <100.00%> (+0.55%) ⬆️
#node_v13_12_0 100.00% <100.00%> (ø)
#windows 89.96% <100.00%> (+0.33%) ⬆️

@ehmicky
Copy link
Owner

ehmicky commented Mar 31, 2020

Thanks a lot @AdrieanKhisbe!

Could you please tell me more about your use case?
Generally speaking nve is meant for single commands while nvm is better for changing the Node.js version in a whole shell session or project.
I am assuming if one has a .nvmrc in a project, it would be simpler to use nvm use once instead of prefixing all commands with nve nvmrc?

@AdrieanKhisbe
Copy link
Author

You're welcome :)

You're totally right about the final point, this is not meant to replace nvm, and nvm use.
When developing, and just npm install, npm run lint, npm test or else, the nvm use work perfectly fine.

For me there is two use cases behind adding to nve/get-node the ability to interpret nvm configuration:

  • nve and this comes very handy if I want to run some tests on different node versions, including the .nvmrc one, just before to push to git repo and trigger the CI,
    In that case I would just use nve nvmrc 10 12 13 npm test
    (Otherwise, I'll have to explicitly test the node 8 version my .nvmrc lock to. nve 8.16.1 10 12 13 npm test for instance)

  • An other, but less important, use case is when I go very briefly into some repository locked to a different version than my default node, and I just need to run one command.
    That will permit me to do it without to switch my shell default node, and then switch back.
    (If I'm here for more than two commands, yes indeed nvm use is definitively best)

nvmrc would just be a node version alias, as current could be a pseudo version referring to the node version invoking nve/get-node.

I hope I wasn't too verbose :)

@AdrieanKhisbe
Copy link
Author

(rethinking about it, maybe you would prefer this to happen at another level, likenormalize-node-version maybe?)

@ehmicky
Copy link
Owner

ehmicky commented Apr 1, 2020

The feature makes sense from the angle you are describing above, thanks for explaining!

Yes, it should actually probably be implemented inside normalize-node-version instead, would that be ok?

Please note after being loaded from file, the version would need to go through all of of normalize-node-version logic in order to benefit from version ranges normalization (12 -> 12.16.1) and invalid versions checks.

Also it would be nice to also support not only .nvmrc but also:

find-up supports multiple filenames so that should be easy.

If supporting more than just nvmrc, the alias should probably renamed. I suggest .:

  • means "from the current directory" (and up)
  • fast to type
  • easy to visually and programmatically distinguish from other versions and from the actual command.
  • this would conflict with . being a built-in in Bash but source can be used instead in Bash

Please let me know what you think.

@AdrieanKhisbe
Copy link
Author

@ehmicky Very interesting suggestions you made! :)

. is well found alias! with semantic in it! 😉
And what do you say about - for current node version (the one that runs the lib/nve)?
(- as previous dir/branch for cd or git

If you agree, I'm going then to port the feature to normalize-node-version and introduce the following alias:

  • . alias that refer to .node-version .naverc .nvmrc
  • - : node version running the lib / nve (suggestion)

@ehmicky
Copy link
Owner

ehmicky commented Apr 1, 2020

Yes, - is a good idea too (using process.version)!

You might want to make it in a separate PR though, once . is merged, so you don't need to resolve potential PR comments on both PRs.

@ehmicky
Copy link
Owner

ehmicky commented Apr 1, 2020

Actually, looking into it, - might be an issue with CLI flags parsing. It might also confuse some users since it looks like a CLI flag.

What about = ("same" version), _ ("last return value" in Node.js REPL) or @ instead?

@AdrieanKhisbe
Copy link
Author

I haven't thought of cli parsing!

I'll say = even if somethings tells me the command could look weird.

would work, but maybe _ would be better?

@ehmicky
Copy link
Owner

ehmicky commented Apr 1, 2020

Yes _ might look the less weird 👍
It is also a fairly safe character in many shells (including Bash and cmd.exe).

@AdrieanKhisbe
Copy link
Author

Cool!
I might come back with a PR on normalize-node-version later in the evening 🙂

AdrieanKhisbe added a commit to AdrieanKhisbe/normalize-node-version that referenced this pull request Apr 1, 2020
…ect version locks (.node-version,.nvmrc,.naverc)

Follow up discussion on client library get-node: ehmicky/get-node#4
AdrieanKhisbe added a commit to AdrieanKhisbe/normalize-node-version that referenced this pull request Apr 1, 2020
…ect version locks (.node-version,.nvmrc,.naverc)

Follow up discussion on client library get-node: ehmicky/get-node#4
@AdrieanKhisbe
Copy link
Author

@ehmicky I just opened the Pull request. as you might see in the reference github inserted.

Tell me what you think of the implementation.


Also, I'm tempted to introduce resolving of nvm aliases (lts and user defined), if you're open to it.

$ nvm ls
# .... user defined alias
default -> 8.16.2 (-> v8.16.2)
node -> stable (-> v12.16.1) (default)
# ....
lts/* -> lts/erbium (-> v12.16.1)
lts/argon -> v4.9.1 (-> N/A)
lts/boron -> v6.17.1 (-> N/A)
lts/carbon -> v8.17.0
lts/dubnium -> v10.19.0 (-> N/A)
lts/erbium -> v12.16.1
# .... eve

# then:
$ nvm_resolve_alias lts/carbon
v8.17.0
$ nvm alias carbon 2> /dev/null
lts/carbon -> v8.17.0

AdrieanKhisbe added a commit to AdrieanKhisbe/normalize-node-version that referenced this pull request Apr 3, 2020
…oject version locks

Supported files for node locks: .node-version, .nvmrc, .naverc

Follow up discussion on client library get-node: ehmicky/get-node#4
AdrieanKhisbe added a commit to AdrieanKhisbe/normalize-node-version that referenced this pull request Apr 7, 2020
…oject version locks

Supported files for node locks: .node-version, .nvmrc, .naverc

Follow up discussion on client library get-node: ehmicky/get-node#4
@AdrieanKhisbe
Copy link
Author

ehmicky/normalize-node-version#1 having landed, released and integrated via fc7c5d7 and following,
I think we can close. :)

@AdrieanKhisbe AdrieanKhisbe deleted the support-nvmrc-pseudo-version branch April 13, 2020 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants