nvm icon indicating copy to clipboard operation
nvm copied to clipboard

Use package.json engines version?

Open kievechua opened this issue 9 years ago • 142 comments

Run nvm use will read local package.json's engines.node and change version accordingly. Or anyway that auto switch the version.

kievechua avatar Feb 10 '15 02:02 kievechua

Hmm - engines.node is only meant to be advisory, and no part of the node ecosystem uses it for any other purpose. For example, if a package specifies "0.8" that doesn't mean it won't work on iojs or 0.12 - it might be problematic to default the user to 0.8 just because the author hadn't updated their package.json.

I could definitely add this as an alternative, when .npmrc wasn't available - I'm just not convinced it would be a good thing. Also, the engines field can specify semver ranges - so I'd need to use semver to resolve that between the available node versions - which is a dependency that requires node in software that doesn't have node as a dependency.

ljharb avatar Feb 10 '15 05:02 ljharb

NPM treats engines.node as advisory, but that's not the only way to use package.json. It's actually very convenient when everyone on the team, the CI server, and production servers all pick the same version, based on package.json. And a huge time saved when shuffling between multiple projects (consulting, microservices, migration, etc).

So it would be of great help if nvm could pick up on the current project's specific choice of iojs/Node.

assaf avatar Mar 24 '15 00:03 assaf

@assaf it already can! Just put a .nvmrc file in your project repo, and do a bare nvm install or nvm use in the project directory.

ljharb avatar Mar 24 '15 00:03 ljharb

How do you tell .nvmrc to pick the version specified in package.json?

assaf avatar Mar 24 '15 00:03 assaf

That's not what I meant - I meant you just manually put a version in that file and commit that to the repo.

ljharb avatar Mar 24 '15 00:03 ljharb

That's not what I was asking for. Having multiple files that specify which version of iojs/Node is in use, just means more things breaking because "works on my machine". Common sense is to keep the project DRY and have a single authoritative specification of the version in use.

assaf avatar Mar 24 '15 00:03 assaf

Right, I agree with that. The problem is that node.engines in package.json specifies the minimum version a module works with - ie, most of mine say 0.4, but I wouldn't want nvm to use 0.4 by default. By contrast, .nvmrc specifies the version I want to use it with, which is the maximum version it works with - iojs or stable for most of mine.

Thus, since they're for two separate purposes, I wouldn't want to get more DRY than two separate configurations. "Too DRY" can be worse than "not DRY enough".

ljharb avatar Mar 24 '15 00:03 ljharb

If you don't like this feature don't use it. I'd like to have such a feature that I will then choose to use for my projects.

assaf avatar Mar 24 '15 00:03 assaf

"if you don't like it then don't use it" is not really a good justification for adding any feature to any project. It's nice that you want it, and thanks for offering your thoughts, but:

  • I don't think the feature makes sense semantically - in other words, that the package.json "minimum supported version range" field makes sense as a "run it under this one single version of node/io.js"
  • Parsing JSON from the shell is prohibitively complex - we can't use any npm modules and would have to use a technique that worked in bash, dash, ksh, and zsh. Can you suggest one?
  • The semver range issue I mentioned above.

In other words, even if I thought this was a good feature on its face, there's just no way it's practical, so it simply can't happen.

I'm happy to review a PR in the future if someone can come up with a simple way to achieve it, but I remain opposed on the semantic appropriateness of this feature and would need to be convinced.

ljharb avatar Mar 24 '15 00:03 ljharb

I'm most familiar with https://github.com/stedolan/jq which Heroku uses to parse package.json as part their deploy setup. Semver can also solved: https://github.com/heroku/heroku-buildpack-nodejs/blob/master/lib/build.sh#L133

PR would be pointless, we can't even agree that my use case for nvm is even worth keeping this issue open for.

assaf avatar Mar 24 '15 00:03 assaf

Yes but that would be an additional dependency to install, which currently nvm requires installing none (simply curl or wget to install node/iojs versions).

If you wrote a small commandline script that did what you wanted and supplied the version to nvm use, that would at least convince me that it's possible in a practical sense.

ljharb avatar Mar 24 '15 00:03 ljharb

https://gist.github.com/assaf/ee377a186371e2e269a7

assaf avatar Mar 24 '15 15:03 assaf

Thanks, that's great - I'm not stoked on the reliance on a heroku app for semver resolution, and the usual advice in #iojs on freenode is to put the iojs version in engines.node, so I'd expect that to be supported.

nvm_json_extract looks solid tho, even though it's a bunch of support functions.

Currently, we have nvm-exec - what would you think about adding a new executable file to the repo that, when run, prints out the contents of engines.node or engines.iojs (and perhaps picks intelligently by default, and can be overridden to only look at a provided field)? Then at least, you could do nvm use $(nvm package-json-version) or something, and that'd be a step in the direction you want, without running afoul of my semantic concerns.

ljharb avatar Mar 25 '15 01:03 ljharb

If you set engines.node to 1.6.2 it will call nvm use v1.6.2. If you set engines.iojs to 1.6.2 it will call nvm use iojs-v1.6.2. If you set engines.node to 0.12.0 and engines.iojs to 1.6.2, it will call nvm use iojs-v1.6.2. It follows how people use engines today to set target versions.

The semver version is used for resolving ranges. Heroku maintains it as part of their build infrastructure, a front-end to nodejs.org/iojs.org, so it's already used by many of the people who will find this feature useful, to develop and deploy against the same version.

However, if you need repeatable builds, you should set the specific version in package.json and only that version will be used. No substitutions. And no dependencies on 3rd parties.

This works. It allows you to use package.json to specify which version of io.js/Node you want your code to run against. And it's based on a common deployment pattern (Heroku but also other PaaS influenced by them).

It does not deprecate or change existing functionality. It should still be possible use .nvmrc, but this could co-exist for developer who prefer to set the engine version in package.json and have it picked up on local, CI, and production.

assaf avatar Mar 25 '15 03:03 assaf

Awesome, that behavior makes sense to me, thanks for clarifying.

Currently, if I don't have a .nvmrc and run nvm use, I get an error and help output - adding an additional fallback would be a breaking change (which isn't a problem, just worth noting).

ljharb avatar Mar 25 '15 04:03 ljharb

I don't think it's a good idea to break current behavior. Could be a new command, or use an alias, maybe nvm use package?

assaf avatar Mar 25 '15 04:03 assaf

Interesting idea! As an alias, it'd have to be overridable, just like node/stable/unstable/iojs are, but maybe that's OK. That would certainly make it work with every nvm command seamlessly.

ljharb avatar Mar 25 '15 04:03 ljharb

Alternatively, it could be a path, so nvm use <version | file> first looks up an alias with that name, if that fails, it looks up a file with that name. That way, it could be a shell script somewhere inside the project, pointing to the package.json in a parent directory.

And if you're in a directory that contains a package.json, nvm ls could also list that version, so you would see:

package.json -> iojs-v1.6 (-> iojs->1.6.2)

assaf avatar Mar 25 '15 04:03 assaf

+1 to @assaf's request here. I was in the middle of coding stuff into my shell files to do exactly this when I discovered this issue. I'd also like to note that from my perspective there are big, important differences between an application and a reusable module, and many things in the npm ecosystem fail to recognize this, causing me much frustration. This is an application-driven feature request where your code is the first thing the node binary loads, and I think engines.node and engines.iojs are good ways to express an application's chosen runtime version.

focusaurus avatar Mar 30 '15 20:03 focusaurus

I ran across this issue when I had the same thought that it would be convenient that nvm use would just pickup whatever I have in my package.json. After reading the comments here I agree with @ljharb and can see that this will likely cause more problems than it is worth since most people probably do not use the package.json engines data like I do with CI to indicate what version of node to install on the build agent.

I would like share that you can still use the info from package.json with nvm by utilizing jq as follows:

$ nvm use $(jq -r .engines.node package.json)

And if you don't have the version installed yet, you can use the same concept with nvm install

$ nvm install $(jq -r .engines.node package.json)

Yeah, thats a lot of typing, but you could alias it if you really wanted to.

jamsyoung avatar Apr 07 '15 17:04 jamsyoung

A version of grabbing engines.node without the external dependency on jq is: node -p -e 'require("./package").engines.node'

focusaurus avatar Apr 07 '15 18:04 focusaurus

Although that won't work unless you already have node installed, which isn't something to rely on when using a node version manager :-)

ljharb avatar Apr 07 '15 18:04 ljharb

Is anyone actively working on a command line nvm < use | install > package.json?

bettiolo avatar Jun 01 '15 17:06 bettiolo

FYI, nodenv has a passable plugin api copied from rbenv. It was pretty straightforward to build a plugin to parse package.json engines after @assaf did all the hard work :)

We're looking at using sh-semver to evaluate semver ranges without a network call or a node dependency.

hurrymaplelad avatar Nov 09 '15 04:11 hurrymaplelad

Just wanted to chime in to say that I agree with @assaf's original sentiment that locking to specific node version should be encouraged -- lack of version constraint is a good thing imho, should folks choose to use engines.node

patcon avatar Dec 10 '15 21:12 patcon

:+1: To the original suggestion. It's super helpful in having a smooth process for getting people up and running on a project locally, especially from zero. I'm imagining a workflow that would be as simple as:

  • clone project from Github
  • install NVM
  • nvm install package.json or something, which would install the correct version of Node and NPM for the project and npm install all the modules
  • npm start

Even better, if there were a way to nvm install-repo [email protected]:..., we could get rid of step 1.

acjay avatar Dec 22 '15 17:12 acjay

@acjay that's already doable if you put .nvmrc in the github repo. then you can just run nvm install and/or nvm use and it will work.

ljharb avatar Dec 22 '15 17:12 ljharb

:+1: @ljharb I somehow missed that. Still feel package.json or npm-shrinkwrap.json (latter would need a core issue) would be suitable, but that's otherwise very helpful. Thanks!

patcon avatar Dec 23 '15 04:12 patcon

@ljharb Ah nice! So then we're just a short hop away from my dream scenario :)

But thanks for the info, I can definitely live with a .nvmrc.

acjay avatar Dec 23 '15 04:12 acjay

The difficulty remains in using POSIX to read JSON, with no node available ¯\_(ツ)_/¯

ljharb avatar Dec 23 '15 06:12 ljharb

Good point!

acjay avatar Dec 23 '15 06:12 acjay

@ljharb haven't used it, but there's this: "Pure-POSIX-shell JSON parser." https://github.com/rcrowley/json.sh (BSD-licensed)

geophree avatar Apr 08 '16 21:04 geophree

This workaround assumes node, but would help keep .nvmrc in sync with package.json: "scripts": { "preinstall": "echo $(node -p -e 'require(\"./package\").engines.node') > .nvmrc && nvm install && nvm use",

But why would nvm not be found in the sub-shell (works fine outside of npm run)? > echo $(node -p -e 'require("./package").engines.node') > .nvmrc && nvm install && nvm use sh: 1: nvm: not found

tomdavidson avatar Aug 19 '16 16:08 tomdavidson

I still think this is a good idea. I understand this is the point of an .nvmrc but our repos are already cluttered with dotfiles, and it seems silly to add one more just for this when there's a place for that in the package.json (which, to me, ought to be the singular point of truth for all package configuration)

brettneese avatar Feb 27 '17 23:02 brettneese

@brettneese engines points to a semver range, and is and has always been purely advisory. I understand that you want the engines field to be proscriptive, but nvm shouldn't be the first tool to respect it by default (not counting warnings).

ljharb avatar Feb 27 '17 23:02 ljharb

There's also engineStrict, which is prescriptive

brettneese avatar Feb 27 '17 23:02 brettneese

Which npm 3 and later have removed, because it turns out to be very bad for the ecosystem. engineStrict is dead.

ljharb avatar Feb 27 '17 23:02 ljharb

Sure, ok. But straight from the docs:

You can specify the version of node that your stuff works on

Wouldn't it make sense that "the version of node that your stuff works on" is the version that NVM uses?

https://docs.npmjs.com/files/package.json#engines

brettneese avatar Feb 27 '17 23:02 brettneese

The docs are inaccurate there - you can specify the versions of node that your stuff works on.

nvm doesn't work with semver ranges, it only works with a full or partial single version string - which can't be inferred from the "engines" field without a semver parser.

ljharb avatar Feb 27 '17 23:02 ljharb

That, in and of itself, is not terribly difficult due to the very nature of semver. Cloudflare did it in bash here: https://github.com/cloudflare/semver_bash/blob/master/semver.sh (Not that I particularly trust anything Cloudflare does anymore ;-)) We hard-code our node versions so that's a non-issue for us but I understand that would need to be in place.

I might even be willing to hack this together at some point and put in a PR to stop bikeshedding (I don't expect project owners to take care of all user requests), but given the response so far I'm not sure it'd be accepted anyway, and I'm still not sure I understand why. It does seem like that's exactly what the field is for and some deployment engines even use it (I think Heroku does, for instance, but it's been ages since I've used Heroku.)

Right now, if we were to properly follow convention, updating our node version for our project would require updating 4 files, at least (.nvmrc, Dockerfile, package.json, readme.) I found and responded to this issue because our package.json was out-of-date and I was having some issues, because, honestly, I thought package.json was our primary point of truth.

Yes, the docs also say it's advisory, but if our tools don't make our jobs easier and our workflow less convoluted, then what's the point in having them at all?

brettneese avatar Feb 28 '17 00:02 brettneese

I'm always open to reviewing a PR! https://github.com/creationix/nvm/issues/651#issuecomment-85827779 is an idea to make it explicit.

I agree that having to update 4 files seems excessive - i'm not sure why the readme would mention a node version tho, nor why the Dockerfile wouldn't read it out of package.json or nvmrc?

ljharb avatar Feb 28 '17 00:02 ljharb

I implemented it in bash here: https://github.com/creationix/nvm/pull/1535

edwmurph avatar May 30 '17 00:05 edwmurph

@brettneese engines points to a semver range, and is and has always been purely advisory. I understand that you want the engines field to be proscriptive, but nvm shouldn't be the first tool to respect it by default (not counting warnings).

Actualy, this is not. Heroku pulls node and npm version from here by default and design (before and among anywhere else)

cyrilchapon avatar Jan 10 '19 08:01 cyrilchapon

@cyrilchapon since it's a range, does it pick the latest in the range?

ljharb avatar Jan 10 '19 09:01 ljharb

I don't have a clue, but I think one could find the strategy (and probably replicate it) inside heroku repos ?

Edit : probably inside https://github.com/heroku/heroku-buildpack-nodejs Edit2: in here https://github.com/heroku/heroku-buildpack-nodejs/blob/4cf68e4e63698cdb56ee18737533482d99d25a74/lib/binaries.sh

cyrilchapon avatar Jan 10 '19 11:01 cyrilchapon

Ah, their strategy requires a running server to resolve the semver range; that won’t work for nvm. Good to have the data point, tho!

ljharb avatar Jan 10 '19 17:01 ljharb

Does it? Isn't it just polling an endpoint? Or am I misunderstanding?

JSON: https://nodebin.herokai.com/v1/node/linux-x64/latest?range=8.x https://nodebin.herokai.com/v1/node/linux-x64/latest?range=<8.2 https://nodebin.herokai.com/v1/node/linux-x64/latest?range=>8.x https://nodebin.herokai.com/v1/node/linux-x64/latest?range=^8.2 https://nodebin.herokai.com/v1/node/linux-x64/latest?range=~8.2

Or txt: https://nodebin.herokai.com/v1/node/linux-x64/latest.txt?range=8.x

patcon avatar Jan 16 '19 18:01 patcon

That, itself, is a list of endpoints.

ljharb avatar Jan 17 '19 03:01 ljharb

I am confused, but that's ok -- this is a long thread. I'm assuming your response is short-hand for saying that a half-way-there UX improvement (that piggybacks on Heroku's semver-resolution strategy and infra) is a show-stopper since it only addresses non-offline users :)

If so, then I feel differently, but genuinely respect your decision!

patcon avatar Jan 17 '19 23:01 patcon

@patcon yes, definitely - it's more than just offline users, tho (since nvm ls-remote is also a factor) but also that I don't want to add yet another remote dependency and network call. Proxies already pose problems for nodejs.org/iojs.org; it would be a burden to these users to be forced to add, or create, yet another mirror for a "semver API".

ljharb avatar Jan 18 '19 00:01 ljharb

Wouldn't that be OK just to bind-to-http-resolution the nvm install (and nvm ls-remote), using package.json[engines] and somehow keep the nvm use local (without HTTP call) with a clever algorithm processing semver-specified release in package.json against installed releases.

I mean, after all nvm install will definately make some HTTP roundtrips, and "processing a semver against a list of installed versions" wouldn't be so hard, would it ?

cyrilchapon avatar Apr 25 '19 07:04 cyrilchapon

@cyrilchapon See https://github.com/nvm-sh/nvm/issues/651#issuecomment-455380459

ljharb avatar Apr 25 '19 16:04 ljharb

Would it be possible to do something like eslint-plugin-react does for React version - i.e. specify in the config file that you want to use the version specified in package.json?

// .eslintrc
{
  "settings": {
    "react": {
      "version": "detect"
    }
  }
}

Perhaps something like this:

// .nvmrc
engines.node // or whatever the property is in package.json

That could also avoid the problem of semver ranges by simply not allowing it if you wish to use this feature, as this would make the feature opt in.

mflodin avatar Nov 01 '19 10:11 mflodin

That could also avoid the problem of semver ranges

Or you could specify the range as well, kinda like browserlist config.

mrchief avatar Nov 01 '19 19:11 mrchief

just to compare....N does this already....according to their docs

Screen Shot 2020-08-28 at 1 46 08 PM

the idea of having to specify my node version in two places is a non-starter. No way that won't fall out of sync.

jasonrberk avatar Aug 28 '20 17:08 jasonrberk

It would be really nice to add this feature:

  • First check if .nvmrc is present and use that version if the file exists (current behavior)
  • If not, check for engines.node config in package.json

Supporting strict version (i.e. "12.18.3") should be easy to implement and already a good start.

pmrotule avatar Sep 04 '20 11:09 pmrotule

Alternatively, it's not necessary to use "engines" property. We can have our own "nvmrc" property in package.json.

The package.json can have a special field called "nvmrc" which would contain the NVM's .nvmrc file contents. It's quite easy to parse package.json with grep.

Here is how you retrieve package version (cross platform version of grep): grep -o '"version": "[^"]*' package.json | grep -o '[^"]*$'

So, NVM could check if .nvmrc file presents OR "nvmrc": "lts" value is present in package.json.

Here is bash POC:

if test -f ".nvmrc"; then
  NODE_VERSION = `cat .nvmrc || ""`
elif test -f "package.json"; then
  NODE_VERSION = `grep -o '"nvmrc": "[^"]*' package.json | grep -o '[^"]*$' || echo ""`
else
  NODE_VERSION = ""
fi

koresar avatar Sep 28 '20 00:09 koresar

@koresar sure, an nvmrc property in package.json wouldn't be a range, it'd just be the same as .nvmrc, except then nvm would still have to have a JSON parser. Why is that better than using a .nvmrc file?

ljharb avatar Sep 28 '20 04:09 ljharb

  1. Nope. No need for JSON parser. See my POC above.
  2. Why people want to put .nvmrc to package.json? There are answers above in this thread. My personal use case - our deployment scripts do not copy dot-files for security reasons.

koresar avatar Sep 28 '20 06:09 koresar

@koresar many legit things live in dotfiles; that seems like a huge flaw in your deployment scripts. The majority of the answers upthread are about having a single source of truth - having two fields in the same file doesn't achieve that.

re your POC, that won't work unless there's a single space after "nvmrc": , which isn't guaranteed, and it doesn't handle a bunch of other whitespace characters that are perfectly valid in JSON. Anything that touches JSON without being a proper JSON parser is automatically broken.

ljharb avatar Sep 28 '20 06:09 ljharb

@ljharb good job spotting the "space problem" in my POC. You are good with RegEx. 👍 So you probably know that this grep -o '"version":\s*"[^"]*' package.json | grep -o '[^"]*$' || echo "" would work with any number of space or tab characters. With some more lines of code we could make this work for new-lines as well.

I find your reply deliberately misleading and unproductive. Somehow you are looking for the proofs to tell other people are wrong, rather than collaborate effectively.

So, again, we can safely parse out single string out of a JSON file using RegEx. Moreover, I believe that there is a 99.9999% chance that this grep would work as is. Which is high enough for production usage.

We already have two sources of truth - package.json and .nvmrc files. How come joining two sources into single source of truth doesn't achieve the requested feature?

And lastly, our deployment scripts "huge flaw" is kinda more difficult than you can imagine. There is more to that. Please, avoid basing your conclusions on my rather not informative sentence. I can' disclose more information at this point. Sorry.

Have a good day.

koresar avatar Sep 28 '20 06:09 koresar

JSON isn't regular, so no, regular expressions simply aren't safe. I appreciate that it would work 99% of the time, but if it's not 100%, it's not safe enough for nvm.

ljharb avatar Sep 28 '20 06:09 ljharb

Mate, not 99%. I said 99.9999%.

Please, avoid faking other people words. It's unproductive.

Thanks.

koresar avatar Sep 30 '20 12:09 koresar

Good news everyone! There is a JSON parser written in pure shell script.

https://github.com/dominictarr/JSON.sh

Here is how I can extract any value from a JSON file:

cat package.json | JSON.sh -b | grep '\[\"version\"\]'

This means that NVM can 100% safely keep its version in the ["nvmrc"] (root) value of the package.json.

Who else still wants to keep the nvmrc value inside the package.json? I do. :)

koresar avatar Sep 30 '20 12:09 koresar

@koresar ok, 99.99999999999% is still not sufficient. It's 100% or nothing. I wasn't "faking" any words, I was approximating/paraphrasing, which is a normal thing humans do in conversation. Please don't be overly pedantic, it's unproductive. Thanks.

ljharb avatar Sep 30 '20 18:09 ljharb

Unfortunately, at a quick glance that tool isn't compatible with ksh due to its use of local declarations on the same line as a variable initialization.

I agree that using a full JSON parser is the only safe approach, and that one's pretty close to what we'd need - but it's still not clear that the complexity and bytes weight is a good tradeoff for being able to have your two sources of truth in the same file.

ljharb avatar Sep 30 '20 18:09 ljharb

This is still an issue and I think many would like a solution.

In an ideal world, you specify the version range in package.json engines and nvm would try to infer the best thing to do - use the top end of the range?

If that isn't feasible, or specifying a specific version is still required for a use case, then specifying a specific version in addition to the range is fine, but I think it would still be better to colocate it with the engines range so that they are next to each other which should be easier to keep in sync.

theoephraim avatar Jan 27 '21 18:01 theoephraim

@theoephraim "try to infer" isn't something i'd consider ideal.

A package.json field that specifies a single nvm-style version-ish would certainly work, and could live alongside engines, but then nvm would have to add a JSON parser just to avoid a separate file.

ljharb avatar Jan 27 '21 18:01 ljharb

When building libraries, one may have a very wide range, so inferring may not really make sense, and throwing an error if a specific version is not specified would probably be best.

But usually for an internal project, you would have a specific version you're running in prod, and you'd like all your environments to match that. In this case, having NVM infer that you want to use 1.2.3 from a range string of "1.2.3" or "~1.2.3" seems quite reasonable, no?

I'd think that if some extra limitations were imposed on when this feature (using package.json engines if .nvmrc file doesnt exist), it may even be quite easy to set up. For example if we assume the package.json file is properly formatted and indented, we may not even need to parse the JSON - we could probably get the version range in engines using a simple regex.

So basically, if no nvmrc file is found, try to find a version from package.json engines making assumptions about it being formatted a certain way. If we find a range and it is tight enough that we can reasonably infer the user's intent, go ahead and use it. I'd assume most folks that want this feature would be willing to comply with whatever limitations are imposed in this case.

theoephraim avatar Jan 27 '21 21:01 theoephraim

I understand the convenience factor there, yes. I'm not convinced that that minor convenience (having one source of truth in package.json instead of two in package.json and .nvmrc) is worth the complexity of parsing JSON.

We definitely can't assume the file is properly formatted, or that it's indented at all, let alone properly.

ljharb avatar Jan 27 '21 21:01 ljharb

I definitely hear you - but... why not? This would be an optional feature - if you want to use it, it's required that you format your package.json file in the standard way. If nvm finds no nvmrc file and cant reasonably figure out what to do from your package.json file, an error is still thrown.

theoephraim avatar Jan 27 '21 22:01 theoephraim

I've explained why not - because JSON parsing in posix would be a very complex and heavyweight thing to add to nvm.sh for a relatively insignificant benefit. It's not reasonable to have an implicit requirement that your package.json be formatted in any particular way (there is no "the" standard way), and the risk of doing the wrong thing silently is high.

ljharb avatar Jan 27 '21 22:01 ljharb

A package.json field that specifies a single nvm-style version-ish would certainly work, and could live alongside engines, but then nvm would have to add a JSON parser just to avoid a separate file.

Could it shell out to the current Node installation to use JSON.parse?

nickmccurdy avatar Jan 27 '21 23:01 nickmccurdy

@nickmccurdy no, because we can't assume any node version is installed - the first time you run nvm install you'd want the version to be read from package.json.

ljharb avatar Jan 27 '21 23:01 ljharb

I do not understand the arguments "complexity of parsing JSON" and "JSON parsing in posix would be a very complex". The complexity is near 0 since there is a robust JSON.sh solution.

I do not understand the argument that JSON.sh "isn't compatible with ksh". I know that NVM gurus would solve this easily.

I do not understand the argument that JSON.sh is "heavyweight". The current NVM is 4115 lines. The JSON.sh is 180 (if you remove CLI args handling). 4115 + 180 = 4295 lines. Not heavy to me.

koresar avatar Jan 29 '21 00:01 koresar

Weight is measured in more than lines. Inlining JSON.sh is a massive maintenance burden for me to take on - it’s vendoring a dependency, with ksh patches, replicating all their tests, and if they ever make any updates, it’s figuring out how to upstream those.

ljharb avatar Jan 29 '21 01:01 ljharb

I understand the implementation time cost.

JSON.sh had no updates in 5 years. Not sure what to maintain there. I foresee 0 maintenance effort there. So I don't understand the "maintenance burden" argument.

koresar avatar Feb 01 '21 10:02 koresar

I think even if we create a new field in package.json looks cleaner than and actual .rc file with just one setting

agnjunio avatar Feb 28 '21 17:02 agnjunio

@ljharb is it acceptable to add a grammar parser? I mean it shouldn't be hard to wirte a specific grammar parser to find "engines" and "node" keywords. If I, or someone else, would write it, how I can test it to meet the acceptance criteria?

Bessonov avatar May 16 '21 09:05 Bessonov

For example, I got a quick&dirty version of parser in bash, but struggle with sh. As I see in this comment, the solution should work in bash, dash, ksh, and zsh. Is this list still exhaustive? What's about semver?

Bessonov avatar May 16 '21 09:05 Bessonov

The list includes those 5, yes.

To safely parse json requires a full json parser, regardless of the property names being located.

ljharb avatar May 16 '21 13:05 ljharb

About JSON parsers. I invite everyone to read this issue starting from the comment: https://github.com/nvm-sh/nvm/issues/651#issuecomment-701360709 In other words, there is a robust JSON.sh parser written in shell (180 lines of code), it was already discussed that even having great JSON parser there is no way NVM will start reading the node version from ./package.json.

koresar avatar May 19 '21 04:05 koresar

But, well, the proposed parser doesn't fulfill criteria of nvm. It just doesn't work on all target platforms.

Bessonov avatar May 19 '21 17:05 Bessonov

This is something that came across my mind at work this week... Then I saw this issue, created Feb 2015 😬

I tried to come at this from a different angle:

Rather than getting nvm to read package.json, why not at least automatically create an .nvmrc file based on package.json so things can stay in sync & nvm can use it?

I'm not sure if there's anything else that does that out there, maybe there is... it's quite simple... but here is my effort to solve one of the issues I think people are trying to solve here (that being, not having to declare the same thing in two different places) https://www.npmjs.com/package/package-node-to-nvmrc.

rubengmurray avatar May 20 '21 21:05 rubengmurray

From my perspective, making it an opt-in feature (by not creating an .nvmrc file) that may have some harsh limitations in order to use it (like formatting your package.json file a certain way that would allow us to detect the version using a simple regex) would be a very reasonable compromise that could help many people. It could even log a big nasty warning whenever the feature is triggered saying something like:

Autodetected node version using package.json engines - using version "X.Y.Z"
If this was not what you expected, please follow the package.json formatting restrictions
listed <here> or just define an .nvmrc file

For a huge number of projects, it would just work as expected. In a few cases, it would try to run but be unable to detect a version and throw an error. In a very small set of cases, it may do something unexpected. A developer hitting those edge cases could either then try to adjust their package.json file until it does what they were expecting, or create an .nvmrc file.

theoephraim avatar May 20 '21 21:05 theoephraim

@rubengmurray that's a great approach, except that it uses husky, which is a very dangerous thing (automatically installed git hooks, i mean, not just that specific tool)

@theoephraim it would have to only ever have false negatives - iow, it might be ok if it fails wrongly, but it's not ok if it installed the wrong version wrongly.

ljharb avatar May 20 '21 23:05 ljharb

@ljharb Interesting... I'm not so experienced with git hooks to be honest, husky just seemed an easy way to implement the hook. Why it is they're 'very dangerous'... Is there potential for clash / overwrite with existing .git/hooks/pre-commit?

rubengmurray avatar May 20 '21 23:05 rubengmurray

@rubengmurray it's a security hole. there's a reason git itself doesn't allow you to ship automatically-installed git hooks with a repo.

ljharb avatar May 20 '21 23:05 ljharb

But, well, the proposed parser doesn't fulfill criteria of nvm. It just doesn't work on all target platforms.

I think we discussed that already. We came to conclusion that it would be quite easy for Shell gurus to adopt that parser to all (actually one) target platforms.

koresar avatar May 26 '21 05:05 koresar

https://itnext.io/locking-the-node-js-version-in-your-projects-70268c877421

For now, perhaps this is the "best" answer? It allows us to define our specific (not minimum) version in node.engines, have (and get benefits of) .nvmrc, but also have explicit node defined (and switched to); all while maintaining a single point of truth in package.json.

rainabba avatar Nov 04 '21 18:11 rainabba

If you mean autogenerating nvmrc, sure, that’s totally viable.

ljharb avatar Nov 04 '21 18:11 ljharb

@ljharb Just some food for thoughts, what about making use of the fact that Yarn and NPM can both parse package.json?

For example here is my current solution to enforce one node version in one place only:

package.json

{
  "engines": {
    "node": "16"
  },
  "scripts": {
    "nv": "echo $npm_package_engines_node"
  }
}

Use the version specified in package.json:

nvm use $(yarn --ignore-engines -s nv)

or

nvm use $(npm run -s nv)

politician avatar Nov 05 '21 00:11 politician

Those package vars i believe don't work anymore, and either way, nvm has to be able to work with no node versions installed.

ljharb avatar Nov 05 '21 01:11 ljharb

Ok that's fair enough!

I've done this in the end, it doesn't cover all use cases but at least will cover the one where one forgets to update .nvmrc

package.json

{
  "engines": {
    "node": "16"
  },
  "scripts": {
    "nv": "echo $npm_package_engines_node",
    "postinstall": "yarn -s nv > .nvmrc"
  }
}

Then just run nvm use as usual.

politician avatar Nov 05 '21 01:11 politician

I propose this in package.json. I.e. avoid using existing "engines" property:

{
  // ...
  "nvm": "16"
  // ...
}

koresar avatar Nov 15 '21 06:11 koresar

@koresar sure, that would solve the "range" problem but would still have two problems: needing to parse JSON, and having two sources of truth (since engines.node still exists)

ljharb avatar Nov 15 '21 06:11 ljharb

  • We already have two source of truth nowadays: package.json and .nvmrc (but package.json is ignored but NVM atm)
  • JSON parsing is a solved problem. JSON can be reliably parsed with just 100 LOC. See https://github.com/nvm-sh/nvm/issues/651#issuecomment-769483899

@ljharb I think I debunked the argument "problem of JSON parsing" two times in this issue. It is actually not a problem. Google: JSON.sh

The only actual argument I saw here is "implementation time cost". It is indeed a downside.

koresar avatar Nov 15 '21 08:11 koresar

That is why it is a huge problem - not because it’s impossible, but because the complexity and byte size required to do it is prohibitive massive. 100 lines of dense code is WAY too much to casually add it to nvm.

If the issue were “debunked” then nvm would have the feature; it continues to be an issue, which is why this remains open.

ljharb avatar Nov 15 '21 16:11 ljharb

A few months ago I adopted a convention I personally think is preferable to having this feature as a part of nvm because it could be used on both Windows and Mac. I use nvm-windows which is similar but different to this particular project, but the API is similar, so your mileage may vary.

  1. after installing nvm, install the latest node version, just so you can run npm (or whatever node version you want)
    nvm install node && nvm use node
    
  2. when you need to use engines, use the environment variable from npm, like $npm_package_engines_node
    • I use windows, so I use cross-env to use the environment variable, like this:
      npm install -g cross-env && cross-env-shell \"nvm install $npm_package_engines_node && nvm use $npm_package_engines_node\"
      
  3. wrap it all up within an npm script, so you can call it (we installed the latest node in step 1)
    "scripts": {
      "setup": "npm install -g cross-env && cross-env-shell \"nvm install $npm_package_engines_node && nvm use $npm_package_engines_node\""
    },
    
  4. run the script whenever you're switching projects, or when you are building in CI
    npm run setup
    npm install
    

To recap:

The first time, you can install whatever node version you want. Any other time, you only need to run npm run setup. Kind of weird in CI environments which are built from scratch on each run, because you install the latest node and then immediately install a different node. But if you're OK with that, it just works.

I don't know if using npm to install npm will shatter dimensions, but it hasn't caused an issue for me yet.

johnrom avatar Jan 13 '22 23:01 johnrom

As an alternative to coding this directly into nvm, you can add it to the shell script located in the README

NOTE: This does not handle semantic versions in the node engine property and requires that jq be installed.

# Calling nvm use automatically in a directory with a .nvmrc or package.json file
autoload -U add-zsh-hook
load-nvmrc() {
  local node_version="$(nvm version)"
  local nvmrc_path="$(nvm_find_nvmrc)"

  # Check for .nvmrc
  if [ -n "$nvmrc_path" ]; then
    local nvmrc_node_version=$(nvm version "$(cat "${nvmrc_path}")")

    if [ "$nvmrc_node_version" = "N/A" ]; then
      nvm install
    elif [ "$nvmrc_node_version" != "$node_version" ]; then
      nvm use
    fi

    return
  fi

  # Check for node engine in package.json
  if [ -f package.json ]; then
    local pkg_node_version=$(jq -r .engines.node package.json)
    if [ "$pkg_node_version" != "null" ]; then
      local nvmrc_node_version=$(nvm version "$pkg_node_version")
      if [ "$nvmrc_node_version" = "N/A" ]; then
        nvm install "$pkg_node_version"
      elif [ "$nvmrc_node_version" != "$node_version" ]; then
    	nvm use "$pkg_node_version"
      fi
      return
    fi
  fi

  # Default nvm version
  if [ "$node_version" != "$(nvm version default)" ]; then
    echo "Reverting to nvm default version"
    nvm use default
  fi
}
add-zsh-hook chpwd load-nvmrc
load-nvmrc

stuft2 avatar Jan 18 '22 20:01 stuft2