nvm-sh / nvm

Node Version Manager - POSIX-compliant bash script to manage multiple active node.js versions
MIT License
78.48k stars 7.87k forks source link

Use package.json engines version? #651

Open kievechua opened 9 years ago

kievechua commented 9 years ago

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

ljharb commented 9 years ago

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.

assaf commented 9 years ago

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.

ljharb commented 9 years ago

@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.

assaf commented 9 years ago

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

ljharb commented 9 years ago

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

assaf commented 9 years ago

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.

ljharb commented 9 years ago

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".

assaf commented 9 years ago

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.

ljharb commented 9 years ago

"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:

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.

assaf commented 9 years ago

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.

ljharb commented 9 years ago

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.

assaf commented 9 years ago

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

ljharb commented 9 years ago

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.

assaf commented 9 years ago

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.

ljharb commented 9 years ago

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).

assaf commented 9 years ago

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?

ljharb commented 9 years ago

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.

assaf commented 9 years ago

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)
focusaurus commented 9 years ago

+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.

jamsyoung commented 9 years ago

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.

focusaurus commented 9 years ago

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

ljharb commented 9 years ago

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

bettiolo commented 9 years ago

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

hurrymaplelad commented 8 years ago

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.

patcon commented 8 years ago

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

acjay commented 8 years ago

:+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:

Even better, if there were a way to nvm install-repo git@github.com:..., we could get rid of step 1.

ljharb commented 8 years ago

@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.

patcon commented 8 years ago

:+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!

acjay commented 8 years ago

@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.

ljharb commented 8 years ago

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

acjay commented 8 years ago

Good point!

geophree commented 8 years ago

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

tomdavidson commented 8 years ago

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

brettneese commented 7 years ago

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)

ljharb commented 7 years ago

@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).

brettneese commented 7 years ago

There's also engineStrict, which is prescriptive

ljharb commented 7 years ago

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

brettneese commented 7 years ago

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

ljharb commented 7 years ago

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.

brettneese commented 7 years ago

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?

ljharb commented 7 years ago

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?

edwmurph commented 7 years ago

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

cyrilchapon commented 5 years ago

@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)

ljharb commented 5 years ago

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

cyrilchapon commented 5 years ago

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

ljharb commented 5 years ago

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!

patcon commented 5 years ago

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

ljharb commented 5 years ago

That, itself, is a list of endpoints.

patcon commented 5 years ago

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!

ljharb commented 5 years ago

@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".