Use package.json engines semver expression #1535
Conversation
|
I'd love to see this become part of @edwmurph I wonder if you could limit the network requests even further (once per day or even per configurable time interval) by saving the |
There was a problem hiding this comment.
This is a massive amount of code to support a very edge use case imo.
Additionally, adding an additional file (besides nvm.sh and nvm-exec) that need to be available is actually a huge breaking change, because a lot of scripts hardcode references to these files.
Additionally, overwriting shell builtins (like cd)is a huge no, that I will never permit to be innvm` - that's hugely disruptive and not something a tool should be doing. The readme contains instructions for those who want to do this, but I won't ship code that does it for the user.
I appreciate the effort!
nvm_helper.sh
Outdated
| @@ -0,0 +1,419 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
A shebang should only be present in an executable file, not in a sourced file…
There was a problem hiding this comment.
I was manually executing it while I was developing it. This would be fixed in the final version. Again I was just looking for high level feedback on how this logic could be leveraged by nvm.
test/fast/nvm_helper
Outdated
| set -e | ||
|
|
||
| testing="true" | ||
| source ../../nvm_helper.sh |
There was a problem hiding this comment.
… but here, this file is sourced
|
"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? |
|
@edwmurph i totally agree that the current algorithm when no version is supplied:
could be enhanced to, for example:
Assuming that reading the semver range and interpreting it is something we can ship. |
|
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.
|
|
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 It's wonderful to be exploring what it would look like - but please don't be under any illusions.
|
|
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? |
|
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. |
|
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. |
|
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? |
|
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. |
nvm.sh
Outdated
| return 1 | ||
| fi | ||
| # TODO handle error case | ||
| # TODO implement without node? |
There was a problem hiding this comment.
it'd definitely need to be implemented without node; nvm install on a system with no nodes installed needs to work for all forms of local version resolution.
There was a problem hiding this comment.
I started down the path of also adding this logic to nvm install but to your point, I'm thinking I should back that out and only add this logic to nvm use. If you agree with that, then do you still think this would need to be implemented without node?
There was a problem hiding this comment.
Oh, I definitely think it should eventually be in install as well; but either way the semver parsing and range logic should be completely offline.
Yes, every part of nvm needs to be implemented without node, full stop.
There was a problem hiding this comment.
This PR needs a reliable (works with package.json files that aren't perfectly spaced/formatted) and extensible (if npm changes the "engines" field again, this code would need to be adjusted) way to retrieve the semver expression from the package.json. After a little research, it looks like the only options are:
-
implementing a full blown industrial json parser in bash just to use for this one edge case (reliable and extensible but too much code)
-
hacking together a regular expression that interprets a nested value (engines.node) in the package.json file (not reliable and not easily extensible)
-
imposing a dependency (https://stedolan.github.io/jq/)
-
using node
I agree that ideally, this should work without node but given the other options, it looks like using node might be the best choice with the only caveat that it wouldn't work for the 1% of use cases where systems have no nodes installed yet.
There was a problem hiding this comment.
Your analysis seems correct to me, and these are the exact reasons that this feature doesn't exist yet in nvm.
However, I think you underestimate; a significant percentage of the use case for nvm is "no node is installed yet"
There was a problem hiding this comment.
On a system with no node installed, nvm install currently reads from .nvmrc or the default alias, and installs it. This works the same if you have a node version installed.
It would be very inconsistent if nvm was capable of reading from package.json if you had a node installed, but that this ability magically appeared once you'd installed a node version.
There was a problem hiding this comment.
Also, it's not obscure - it's the primary use case for a node version manager: installing node and then managing it.
There was a problem hiding this comment.
Sorry @ljharb I didn't realize you've already been in this exact debate multiple times before.
nodejs/version-management#13
nodejs/version-management#12
There was a problem hiding this comment.
Since node is out of the question, is it worth exploring the idea of using regex to read the node version from package.json? The following command looks like it would work?
PKG_JSON_SEMVER_EXPRESSION=$(cat package.json \
| tr '\n' ' ' \
| grep -o '"engines":\s*{\(\s*"\(node\|npm\)":\s*\"[0-9.<>=*~^xXv\s]\+",\?\s*\)\+}' \
| grep -o '"node":\s*\"[0-9.<>*~^xXv\s]\+"' \
| awk -F: '{ print $2 }' \
| sed 's/[",]//g')
There was a problem hiding this comment.
All ideas are worth exploring :-)
Using regex seems like it'd be very brittle, since json can have all kinds of indentation, and multiple duplicate keys. However, with sufficient tests, it might be worth including.
|
Cannot wait for this feature! |
|
|
||
| # TODO add more test cases | ||
| TEST_CASES=( | ||
| '{\n\t"engines": {\n\t\t"node":"1.1.1"\n\t}\n}"' '1.1.1' |
There was a problem hiding this comment.
for test cases - let's make a dir containing a whole bunch of .json files, that are all example package.json files. That'll be easier to understand from a human perspective.
There was a problem hiding this comment.
On second thought, if we store each test case in its own example package.json file and make them human readable, it'd be difficult to see which ones are testing which type(s) of spacing. Also I can think of at least 25 test cases this should probably have, so I think it'd be easier to get the full picture of all the tests if they were listed as they currently are in a single file. What do you think?
There was a problem hiding this comment.
i would assume that whatever spacing is present in a .json file, is verbatim the test case - human readable isn't the goal as much as human understandable :-)
nvm.sh
Outdated
| if ! ((in_quotes)); then | ||
| if [[ "$i" == "{" ]]; then | ||
| open_brackets=$((open_brackets+1)) | ||
| elif [[ "$i" == "}" ]]; then |
There was a problem hiding this comment.
since we support posix, we can't use [[ ]] syntax - that's bash, not sh.
nvm.sh
Outdated
|
|
||
| nvm_get_node_from_pkg_json() { | ||
| local engines_object='' | ||
| local open_brackets=0 |
There was a problem hiding this comment.
because ksh doesn't have local, the local foo and foo=0 pieces have to be on separate lines.
nvm.sh
Outdated
|
|
||
| printf "$1" \ | ||
| | tr -d '[[:space:]]' \ | ||
| | grep -o '"engines":{".*' \ |
There was a problem hiding this comment.
please prefix commands with command so they override user aliases
| "engines": { | ||
| "node": "${NODE_SEMVER_EXPRESSION}" | ||
| }, | ||
| "descriptiion": "fake" |
nvm.sh
Outdated
| fi | ||
| if ! ((in_quotes)); then | ||
| if [ "$in_quotes" == "false" ]; then |
There was a problem hiding this comment.
== also is a bashism, not a posixism
edwmurph
force-pushed
the
master
branch
8 times, most recently
from
d0216f2 to
c2af1f4
Compare
|
@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? |
|
Occasionally it can be, due to network conditions - particularly the io.js ones. I've reran the errored builds. |
|
@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. |
|
@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
force-pushed
the
master
branch
2 times, most recently
from
8758c51 to
68e830d
Compare
|
@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 |
|
Any chance this can be approved/merged soon? |
|
@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. |
|
👍 Having both commited a .nvmrc and a For us, I could think of 2 strategies :
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. |
|
@edwmurph Would you consider coming back to this for a refactor and simplification. Its been a while and I bet your simplifications skills have gotten better during covid. Would love to see this feature happen!! |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@edwmurph wondering if you are still planning to rewrite the implementation? Apparently you're the only one who took the effort to solve this (imho) shortcoming in nvm. |
|
@distinctgrey i can definitely give this another shot but i need an nvm maintainer to provide some buy in and direction to avoid wasting time |
ljharb
force-pushed
the
master
branch
2 times, most recently
from
c6cfc3a to
c20db2a
Compare
|
@edwmurph There is also now |
Update the
nvm usealgorithm to dynamically interpret the semver expression from the local package.json's engines.node value.Key:
✅ : completed
☑️ : existing nvm functionality
🔜 : in progress
❌ : not started
Update to
nvm usealgorithm: