Use package.json engines semver expression

[edwmurph]

Update the nvm use algorithm to dynamically interpret the semver expression from the local package.json's engines.node value.

Key: :white_check_mark: : completed :ballot_box_with_check: : existing nvm functionality :soon: : in progress :x: : not started

Update to nvm use algorithm:

1. ☑️ try to resolve to node version in local .nvmrc
2. ✅ try to resolve to node version in package.json "engines.node" value (tested ✅)

1. ✅ Retrieve semver from local package.json's engines.node (tested ✅)
2. ✅ Resolve retrieved semver to the newest compatible node version (tested ✅)

3. ☑️ try to resolve to node version in inherited .nvmrc
4. ☑️ try to resolve to use default
5. ☑️ if error and show help text

[joshdick]

I'd love to see this become part of nvm! 👍

@edwmurph I wonder if you could limit the network requests even further (once per day or even per configurable time interval) by saving the curl'd data to a file, possibly a hidden file in $NVM_DIR or $HOME or , and checking that file's modification time to determine whether a new copy should be downloaded?

[edwmurph]

"overwriting shell builtins is a huge no"

I completely agree however I think there are other ways nvm could leverage the ability to dynamically interpret semver expressions. For instance what do you think about instead hooking this logic into calls to "nvm use" when there are no other arguments and there isn't a version specified in a local .nvmrc?

[ljharb]

@edwmurph i totally agree that the current algorithm when no version is supplied:

1. check for .nvmrc
2. use default, if present
3. error and show help text

could be enhanced to, for example:

1. check for local .nvmrc
2. check for package.json "engines"
3. check for inherited .nvmrc
4. use default, if present
5. error and show help text

Assuming that reading the semver range and interpreting it is something we can ship.

[edwmurph]

That sounds great! There's still a little work left to do but I don't see why we couldn't get this ready to ship. Most of the remaining work is just ironing out some implementation decisions that I could use your input on.

1. When interpreting ranges, how do you think a version should be chosen? E.g. Should the newest version be chosen or the oldest? The current implementation has a flag that lets you choose but is this configuration point something you think we should keep?
2. The algorithm for interpreting semver expressions relies on having a list of up-to-date node versions. Right now I'm retrieving them from http://nodejs.org/dist/. Do you feel comfortable with this dependency? Also per @joshdick's suggestion, do you think this list should be cached in a file to further limit the number of network requests?
3. What do you think about the name nvm_helper?

[ljharb]

I don't want to get your hopes up, though - this (and any implementation of semver parsing) is almost certainly too large to be included in nvm with its current architecture.

It's wonderful to be exploring what it would look like - but please don't be under any illusions.

1. I don't think that's a necessary configuration point; when choosing two semver ranges (the one in package.json, and the one representing all installed versions) always choose the latest matching version, full stop.
2. Semver is a static format - i'm confused why we need a list of the remote versions at all for nvm use/nvm ls/nvm alias. Potentially for install, but we already grab that list as part of ls-remote during install, so that's not really an issue.
3. As I said above, 100% of the code needs to live in nvm.sh or nvm-exec, so the name isn't really important right now.

[edwmurph]

Good points I agree with all three of your responses thanks.

Also, could you clarify your concern about this logic being too large to be included? Are you saying you don't like that "visually" there are too many lines of code to add to nvm.sh for an edge use case?

[ljharb]

It's not really about visual size, it's about how much code there is to reason about, how likely bugs will be, how hard it will be to fix those bugs, etc.

[edwmurph]

Ok that makes sense. I have some ideas I'd like to explore around how to simplify the semver parser logic so I'm going to pursue this a little further. My goal is to update this PR by this weekend with a more polished proposal of the enhanced nvm use algorithm that accommodates your concerns as best as possible. Thanks for all your input.

[edwmurph]

In order to setup a dev environment, I'm assuming I need to run the makefile but I can't get it to complete because 3 tests are locally failing out of the box for me. Any chance you could try to help me debug? If so Is this thread the best space for that?

[ljharb]

You don't need to run the Makefile to get set up.

@edwmurph let's continue that discussion (including which tests are failing) on #1339

[edwmurph]

Oh perfect thanks for pointing that out. I'm guessing my questions will be answered in that thread but I'll message you in there if I'm still stuck.

[runk]

Cannot wait for this feature!

[edwmurph]

@ljharb Is the continuous-integration check flaky by any chance? I'm specifically asking about my previous two commits. Are the errors reported in the failed build accurate?

My last commit only removed some comments and the build is indicating that that commit broke the installation_node test suite.

If I just keep retrying will they eventually pass?

[ljharb]

Occasionally it can be, due to network conditions - particularly the io.js ones. I've reran the errored builds.

[edwmurph]

@ljharb Is there a way I can manually trigger a rebuild? Also What do you think about the PR in it's current state? It basically extends the same functionality of .nvmrc to package.json files. If this idea were to ever be incorporated into nvm, this might be a good slice to stop at for now.

[ljharb]

@edwmurph unfortunately no, travis-ci doesn't give you that ability. I can do it, or you can force push to trigger a whole new build.

I'll give the whole PR a review now if you're ready :-)

[edwmurph]

@ljharb thanks for the review.

parsing the semver range (there's an entire node lib, semver, to handle this - replicating all of the relevant logic in posix is not likely to be easy)

I have some ideas on how to make this implementation simpler. Specifically, the range evaluation can be built off some existing nvm functionality (e.g. nvm_version_greater etc.) and maybe it can just extend the functionality in nvm_remote_version (this would in turn extend the functionality of the .nvmrc)? Regardless this would still be a lot of work so if there's a way this implementation could be delivered in iterations, it would make this feature a lot more approachable.

My new proposal is for this PR to only copy the functionality that exists for interpreting the node version from .nvmrc (only support a node version as defined by the nvm --help output and unsupported versions would be handled the same as .nvmrc handles them today). Then the next iteration can tackle the rest of the semver parser evaluation.

[ljharb]

Since existing "engines" fields already most commonly contain a full semver range, I don't think an MVP for this feature can support any less than that :-/

[jhenaoz]

Any updates here?

[edwmurph]

@jhenaoz I finished the implementation for interpreting basic semver's from the package.json.

Also I'm currently still working on the logic for interpreting complex semver expressions. My idea for this is the following algorithm:

step 1: convert complex input semver range into the following grammar

semver         ::= comparator_set ( ' || '  comparator_set )*
comparator_set ::= comparator ( ' ' comparator )*
comparator     ::= ( '<' | '<=' | '>' | '>=' | '' ) version_part '.' version_part '.' version_part
version_part   ::= ['0'-'9']+ | 'x'

e.g. ~1.2.3 || ^1.4.0 -> >=1.2.3 <1.3.0 || >=1.4.0 <2.0.0

step 2: resolve each comparator set (e.g. comparator_set1 || comparator_set2) to the newest possible compatible node version

The naive way to implement this would be to iterate (from newest to oldest) through the list of existing node versions, and find the first one that satisfies all comparators. Version range comparisons could leverage existing nvm functions. Also there is an existing way in nvm to retrieve all current node versions but that list doesn't appear to be cached so I may or may not need to implement caching logic.

step 3: choose the highest node version among the resolved comparator sets .

this should be pretty straightforward.

Current status: I'm nearly finished with step 1 and have partial implementations for step 2 and step 3. Then after this main logic is implemented, I'll need to develop a strategy for extensively testing all of this. Also while reading/interpreting semver expressions in nvm's environment is definitely possible, it's still unclear to me whether the final implementation of this PR will end up being too large to be included in nvm.

[edwmurph]

@ljharb I completed the first step of this implementation of extracting the engines.node value from the local package.json file and finished extensively testing this logic. Could you review nvm_get_node_from_pkg_json() and "test/fast/Unit Tests/nvm_get_node_from_pkg_json"?

I also got the rest of the implementation working but am still refactoring/optimizing/testing it so those other areas are not worth your full review at this point.

Also I added a temporary file called "semver.md" that documents the overall design of the implementation.

[ljharb]

(Sorry for the delay; I will hopefully be able to review soon)

[paulswebapps]

Hello all. Question: Is this feature still in the works? I was about to hand-role my own system to create a single-source-of-truth for the node version, but if this is going to be released in the near future, I would hold-off on doing so. Thanks for your hard work!

[edwmurph]

@paulswebapps The current implementation in this PR works but there is still some optimizing and testing left to do. Now that we have a proof of concept, I'm just waiting to do the remaining work until I get some review from @ljharb to help figure out whether the current approach has the potential to be a feature of nvm.

[paulswebapps]

@edwmurph Thanks for the update 👍

[markmhendrickson]

@edwmurph @ljharb Sorry for arriving late to the party here, but has this effort stalled or is there another implementation somewhere else?

[edwmurph]

@markmhx What's there is not polished code yet, but it does prove that the algorithm works so I was hoping to get some high-level feedback on the changes in nvm.sh before I continued with the implementation. However, I think the biggest fear people have with this functionality is that it requires a lot of code to support a small use case. So I guess one path forward would be for me to work on simplifying/optimizing it in the coming month and maybe it'll get a little more traction.

[yvele]

Just a simple remark, maybe we can use something easier to parse like a custom nvm field in package.json:

{
  "nvm" : {
     "use" : "8.10.0"
   }
}

we can even use aliases

{
  "nvm" : {
     "use" : "lts"
   }
}

[edwmurph]

@yvele that would be doable and much simpler but what additional value would that bring that you couldn't already do by specifying a semver for nvm use in the .nvmrc?

https://github.com/creationix/nvm#nvmrc

Also the benefit of using the engines.node field in the package.json is that developers will only have to specify a single field in the package.json that both nvm and npm use instead of defining the required node version for the project in multiple places.

[yvele]

@edwmurph many popular tools like babel, eslint, jest, etc. allows the configuration to be embeded in package.json.. That allows a simpler repository structure without a lot of hidden/rc files (.editorconfig, .nvmrc, .npmrc, .gitignore, .mailmap, etc..) 🤷‍♂️

PS: I personally prefer the config to be explicit instead of parsing semver and ending with an approximate node version

[edwmurph]

@yvele I appreciate the feedback and could go either way on a custom field in the package.json vs. just using the engines.node field.

As for your P.S., I agree with you and also previously proposed something similar. Specifically I think that this PR should just focus on interpreting node versions in the format defined in nvm --help output. E.g.:

Note: <version> refers to any version-like string nvm understands. This includes:
  - full or partial version numbers, starting with an optional "v" (0.10, v0.1.2, v1)
  - default (built-in) aliases: node, stable, unstable, iojs, system
  - custom aliases you define with `nvm alias foo`

But @ljharb indicated earlier in this thread that this feature couldn't support anything less than full semver range interpretation. That being said, I'm not sure if his opinion has changed or not because that was said almost a year ago.

[yvele]

Ok so .npmrc within package.json should have it's own issue. Sorry for the digression 😉

[edwmurph]

Somehow I messed up the git history for my branch and it was causing problems merging in changes from upstream so I had to squash merge all my changes up to this point.

[edwmurph]

@ljharb At this point I believe I've simplified/optimized this to the best of my ability and am looking for feedback on the approach before I write the rest of the tests.

[ljharb]

@edwmurph thanks, i'll take another pass

[edwmurph]

@ljharb Thank you for your previous review. At this point I have addressed all your comments. I also finished writing the rest of the tests and now believe the implementation has extensive test coverage. So when you have time, I would appreciate another round of review.

Currently, this feature requires the addition of 536 lines to nvm.sh which is ~14.6% (536/3651=.146..) increase.

Also I understand that this work is exploratory in that it's unclear if this feature is too large to be included in nvm with the current architecture. However, I would really like to understand the particular bottlenecks to see if they can be overcome.

[danawoodman]

Any chance this can be approved/merged soon?

[edwmurph]

@danawoodman right now it looks like this PR is currently stalled on the problem of it being prohibitively complex. It does have extensive test coverage which provides some solid assurances. But admittedly debugging any issues that arise would not be easy for someone unfamiliar with the implementation.

So while this implementation does work for all the test cases I could think of, for this to be merged it would probably need to be further simplified to the point where it's easier to understand/debug. How much more simplification can take place is a question I'm not sure about but I would welcome anyone with more POSIX experience than me to open a PR against the fork I've been working out of to move this along.

[cyrilchapon]

👍

Having both commited a .nvmrc and a engines in package.json is error prone. Especially for us, loving-nvm-heroku-users.

For us, I could think of 2 strategies :

• Spam heroku each day to ask for respect .nvmrc versions (which lack the feature of describing npm version also)
• Upgrade nvm to respect package.json engines, which is a standard nowadays, even if "advisory"

I think the second one is more reasonable, isn't it ?

This is probably a lot of code, and semver appliance strategy review, but this is not at all "useless" for everyone. I'd be glad to elaborate on the use cases.