feat: add env use local Node version management (nvm) functionality, implementing devEngines.runtime.version#9707
feat: add env use local Node version management (nvm) functionality, implementing devEngines.runtime.version#9707danielbayley wants to merge 5 commits intopnpm:mainfrom
env use local Node version management (nvm) functionality, implementing devEngines.runtime.version#9707Conversation
Ok, implemented, and working nicely for me locally… Please test @zkochan. Also @jwanner83 @GeoffreyBooth @moisout, any input from you guys would be appreciated… |
env use local Node version management functionalityenv use local Node version management (nvm) functionality, implementing devEngines.runtime.version
GeoffreyBooth
left a comment
There was a problem hiding this comment.
I think this calls env use to automatically switch to the defined Node version, regardless of onFail? I think that corresponds to onFail: 'download', but that's not the default behavior. By default I think it would just error that the user is running the wrong Node version (same as onFail: 'error').
pnpm/src/main.ts
Outdated
| if (!semver.valid(runtimeNodeVersion)) { | ||
| await updateProjectManifest(prefix, { | ||
| devEngines: { | ||
| runtime: { | ||
| name: 'node', | ||
| version: nodeVersion | ||
| } | ||
| } | ||
| }).then(() => { | ||
| const message = `"devEngines.runtime.version": "${runtimeNodeVersion}" was resolved to "${nodeVersion}"` | ||
| logger.info({ message, prefix }) | ||
| }) | ||
| ).extraBinPaths |
There was a problem hiding this comment.
I thought the idea was to keep the range in devEngines and save the exact version in useNodeVersion, no?
There was a problem hiding this comment.
I thought the idea was to keep the range in devEngines and save the exact version in useNodeVersion, no?
Ah, I think we got wires crossed slightly @zkochan on this…
I’m thinking if
devEngines.runtime.versionis unset, or has been manually set to a range, resolve that withgetNodeVersion(), and set it to a specific version (i.e. it replacesuseNodeVersion), but if it is already set to a specific version, that is the source of truth, and it doesn’t get overwritten, only read from (again, in place ofuseNodeVersion). I’m not suggesting anything randomly alter that version, other than the user either manually changingdevEngines.runtime.version, or setting it viapnpm env use <version>(which would also resolve to a specific version before writing).
ok
I was thinking ideally we could try and deprecate useNodeVersion, as there is always too much config! I thought if we could pin down devEngines.runtime.version as an exact version, this would simplify things, but maybe I am overlooking other reasons why that should be a SemVer range? For now I have taken the useNodeVersion approach. Seems to be working pretty well, but do test…
There was a problem hiding this comment.
I am fine with devEngines.runtime.version getting pinned down to an exact version. But according to the spec it can be a range, no?
There was a problem hiding this comment.
I am fine with
devEngines.runtime.versiongetting pinned down to an exact version. But according to the spec it can be a range, no?
Yeah. So in the previous iteration, I had it resolving the range provided by devEngines.runtime.node to an exact version, and then overwriting that, rather than leaving useNodeVersion in the mix to complicate matters…
I think this is a key decision, which affects how I structure the rest of the code around it in this PR, so would be good to get @GeoffreyBooth’s thoughts on this behaviour in particular, since he wrote the spec.
Another idea might be to restrict it to a caret ^ range?
cc: @ljharb @wesleytodd
There was a problem hiding this comment.
Another idea might be to restrict it to a caret ^ range?
That is still not an exact version. It doesn't make a difference.
There was a problem hiding this comment.
I think this is a key decision, which affects how I structure the rest of the code around it in this PR, so would be good to get @GeoffreyBooth’s thoughts on this behaviour in particular, since he wrote the spec.
The point of devEngines is to ensure that developers of a particular project are running the intended Node version and package manager as appropriate to develop that project. Generally major versions are sufficient for this purpose, though if people want to lock down exact versions, sure, why not. The onFail field tells the thing doing the checking (here pnpm, or also npm in its implementation) what should happen if the running version is outside of the specified range; so if the specified Node version is >= 24 and the user is running 22, the default behavior is to just error and exit, but onFail: download could do Corepack/env use-like behavior to change the currently-running Node version to the newest version that fits within the specified range. There's also onFail: warn which just prints that the versions mismatch but ignores the issue, and onFail: ignore which doesn't do anything. (That turns devEngines essentially into documentation, like a comment.)
Rather than going straight to onFail: download, it might be simpler to start with onFail: error | warn | ignore, which is what npm has implemented, and land that first and then tackle download as a follow-up.
So in the previous iteration, I had it resolving the range provided by devEngines.runtime.node to an exact version, and then overwriting that
I don't know why pnpm would ever overwrite anything within devEngines. The point of the field is for developers to keep their team in sync running the correct versions of the software needed to develop the project. If devEngines wants to lock the team into Node 22, then pnpm shouldn't be updating it to specify Node 24. If devEngines wants to tell the team they should be using npm, then pnpm shouldn't be updating it to specify that the team should use pnpm. That would defeat the purpose of the field.
There was a problem hiding this comment.
I don't know why pnpm would ever overwrite anything within devEngines.
I think users expect the package manager to make changes to package.json, so I don't see that as a clear problem. I agree in most cases it feels likely folks would want to manually edit, but I also like the idea of tools starting to write the field (either on init or as a recommendation when running) since that will help raise awareness that the feature exists and how to use it.
I admit I did not take the time to fully read the implementation, but is it currently updating the range in an experience that may not be directly asked for by the user?
There was a problem hiding this comment.
I haven't read the code of this PR; I was just responding to what was written above. But if this PR edits anything in the version field, that's problematic. The point of that field is to define the range of versions of Node or a package manager that are acceptable to use in developing the current project. Such a range should only be updated by a human, because there's no way for a tool like pnpm to know what an acceptable range would be. Maybe this project needs to be limited to Node 22 because that's the newest Node version that AWS lambda supports; or Node 20 because that's the newest version that some instrumentation library supports; who knows. The point being, it's not safe to update automatically, because there's no way for the tool to know the motivation behind the current value.
There was a problem hiding this comment.
Generally major versions are sufficient for this purpose, though if people want to lock down exact versions, sure, why not.
But if this PR edits anything in the version field, that's problematic.
I don't know why pnpm would ever overwrite anything within
devEngines. The point of the field is for developers to keep their team in sync running the correct versions of the software needed to develop the project. IfdevEngineswants to lock the team into Node 22, then pnpm shouldn't be updating it to specify Node 24.
@GeoffreyBooth To be clear, ‘overwrite’ here meant resolving the existing devEngines.runtime.version range to an exact version, then overwriting it with that specific version, not straying outside of the boundaries set by that initial range. So as per your example, Node 22 would never become 24. More like ^22 would become e.g. 22.17.0.
If
devEngineswants to tell the team they should be using npm, then pnpm shouldn't be updating it to specify that the team should use pnpm. That would defeat the purpose of the field.
This wouldn’t happen either, as onFail (or lack thereof) would throw, as in #9742.
Again, only devEngines.runtime.version would ever be overwritten under the above potential scenario…
The other being we leave it as a range always, and keep useNodeVersion around to nail down the specific version—or preferably an entry in the lockfile as @zkochan mentioned. That’s the key decision here, for which I am seeking consensus…
is it currently updating the range in an experience that may not be directly asked for by the user?
@wesleytodd Only resolving that range to a specific version within said range, or in the specific scenario where the user is delibrately running env use and onFail === 'download'.
Does all of that make sense?
|
It would be ideal, for a library's CI, to also have a way to opt in to looking at "the union of engines and devEngines", not just "devEngines", since the devEngines range is likely to be a subset of the engines range. |
wesleytodd
left a comment
There was a problem hiding this comment.
Awesome to see this moving forward. Are there plans to also address the other fields of devEngines? I wouldn't think this needs to block anything, just wondering if packageManager and os (the main two after runtime that I think matters most) are on your plans.
Feel free to donate :)
I have already implemented
Interesting @ljharb, but sounds like a candidate for a separate issue/follow-up PR…
Ok, added a |
|
@danielbayley without that possibility, it basically forces engines and devEngines to be identical, which largely (but not entirely) erases the point of the feature. |
I agree it sounds useful. Regardless of whether it should be a follow-up PR, how do you envisage those changes to the algorithm exactly? I think I can use something like |
|
Ideally an environment variable (and perhaps also a command line argument) that sets a config flag for this behavior, that way CI can set it once in a workflow and forget about it. |
| if (opts.useNodeVersion && semver.satisfies(opts.useNodeVersion, nodeVersion)) { | ||
| nodeVersion = opts.useNodeVersion | ||
| } else { | ||
| const { version, onFail } = getNodeRuntime(opts.rootProjectManifest.devEngines) ?? {} | ||
| if (version) { | ||
| const message = `"Node.js version "${nodeVersion}" is incompatible with "devEngines.runtime.version: ${version}"` | ||
| switch (onFail) { | ||
| case 'download': | ||
| overwriteVersion = version | ||
| break | ||
| case 'ignore': | ||
| return '' | ||
| case 'warn': | ||
| logger.warn({ message, prefix }) | ||
| break | ||
| case 'error': | ||
| default: throw new PnpmError('INVALID_NODE_VERSION', message) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't understand what is happening here. Why should the range passed via the CLI argument be validated? That's what the user asks to be used. If there are some existing settings, just update them.
There was a problem hiding this comment.
I don't understand what is happening here. Why should the range passed via the CLI argument be validated? That's what the user asks to be used. If there are some existing settings, just update them.
We’ve been having severe network problems in my area, with atrocious internet speeds, like being back in the 90’s with dialup! So I was made keenly aware of how expensive unnecessarily calling getNodeVersion() more than once is (downloadNodeVersion()— which we don’t necessarily want to run, if onFail specifies otherwise—calls it again, internally). So this was an attempt to avoid repeat calls to that, resolving things against the given devEngines.runtime.version range using semver methods, if possible.
Specifically, since main.ts has already resolved the range from devEngines.runtime.node to an exact version, and “passed” it to useEnv via useNodeVersion, the if here can potentially compare the 2 without either having to call getNodeVersion() or getNodeRuntime() again, under certain circumstances. For example, if the given CLI arg is 20 and the devEngines.runtime.node range is something like ^20, semver.satisfies this, and we can skip the rest and continue… Does that make sense @zkochan?
Maybe we could memoize getNodeVersion() or something… What do you think?
There was a problem hiding this comment.
@zkochan I altered getNodeVersion() to avoid unnecessary API calls if already given a valid SemVer version. Seems to speed things up quite a bit!
|
|
||
| if ('webcontainer' in process.versions) { | ||
| globalWarn('Automatic installation of different Node.js versions is not supported in WebContainer') | ||
| } else if (!config.useNodeVersion && devEngines) { |
There was a problem hiding this comment.
ok, and what if useNodeVersion is set? You just removed that whole logic that existed before for useNodeVersion. It is a breaking change.
There was a problem hiding this comment.
also, useNodeVersion should have bigger priority than devEngines. As we discussed, useNodeVersion should store the exact version.
There was a problem hiding this comment.
ok, and what if useNodeVersion is set? You just removed that whole logic that existed before for useNodeVersion. It is a breaking change.
Ah was just missing the extra else! I wasn’t testing with useNodeVersion, so accidentally bypassed it. Good catch @zkochan, fixed.
also, useNodeVersion should have bigger priority than devEngines.
Yeah the missing else also fixes that, so useNodeVersion should behave as before.
| const {version} = getNodeRuntime(devEngines) ?? {} | ||
| if (version) { | ||
| const {nodeVersion} = await getNodeVersion(config, version) | ||
| config.useNodeVersion = nodeVersion ?? undefined |
There was a problem hiding this comment.
This config change should be saved in the pnpm-workspace.yaml file.
There was a problem hiding this comment.
Then wouldn’t that forcibly create pnpm-workspace.yaml file on every run of pnpm, regardless of whether there is any other configuration desired, or which command is being run?
There was a problem hiding this comment.
yes, it will create pnpm-workspace.yaml to lock the node version. The alternative solution is to save it in the lockfile as I mentioned earlier.
There was a problem hiding this comment.
you don't like this solution. That's OK. I will add it to the lockfile in a different PR and then we can finish this pr.
There was a problem hiding this comment.
yes, it will create pnpm-workspace.yaml to lock the node version.
you don't like this solution. That's OK.
I just think it’s bad DX to force things like this on the user with no alternative.
The alternative solution is to save it in the lockfile as I mentioned earlier.
I think that would be better, regardless of whether we decide to deprecate useNodeVersion in favour of devEngines.runtime.version as an exact version.
I will add it to the lockfile in a different PR and then we can finish this pr.
Ok, cool.
Sounds good. This question was for you and @zkochan. If you land this, would the goal be to land those other PRs so that they go out in a single release that "adds devEngines support" or would it go out in individual releases? |
I think ideally the initial release with |
@wesleytodd @GeoffreyBooth See #9742 for these additional checks… |
48cc009 to
5c5bc0b
Compare
|
I have created a new PR that adds support for "runtime" dependencies: #9755. This way the node.js version range will be resolved to an exact version and locked in the lockfile as a regular dependency. An integrity checksum will also be saved for the resolved version. I probably won't enable it for the existing settings yet (like useNodeVersion). So, it will work only for devEngines. |
|
I am also interested in something like "prodEngines". Like say I want to publish a CLI tool and I want to tell pnpm what runtime to install for that CLI tool. |
This is great. They are dependencies so this make sense.
How would that be different than |
|
You can already do that by setting eg |
Isn’t that what regular
Feels like an ugly CSS hack 😅 |
|
Coming back to this PR. I have added support for runtime dependencies via #9755. As I mentioned earlier, I did this by introducing a new version protocol: This will be saved to devEngines then. I did a new PR for that part: #9774 So, I am not sure whether we should make |
pnpm/pnpm#9707 (comment) ```bash pnpm add -D node@runtime:22.19.0 ```
pnpm/pnpm#9707 (comment) ```bash pnpm add -D node@runtime:22.19.0 ```
|
Folks, one small part that I am not sure whether is in this PR, or should/could be added separately: enforcing devEngines.runtime (when onFail is is empty=error/warn), which is what npm supports. I am slowly migrating projects from npm to pnpm, adding devEngines.packageManager (with onFail=warn due to known problems with pnpm calling npm for some operations) and also switching some from typical yet incorrect when published e.g. |
|
This is implemented in pnpm v11 via a new |
|
Thank you @zkochan and @danielbayley as well for pushing this forward! |
This PR implements #3434.
It also takes into account
package.*devEngines.runtime.version, possibly closing #8153.Closes #9364, and probably #7211 plus #7645.
See also related discussion around this here: nvm-sh/nvm#651.
npmimplementation here: npm/cli#7766.