[BUG] npm should refuse to install a version of npm that has an engines conflict

[wraithgar]

Current Behavior:

I can install a version of npm in an environment where the engines property in the packument does not include the version of node it is being run under.

Expected Behavior:

npm refuses to install a version of npm for which it has a conflict between the current node version and the engines entry in the version of npm you are trying to install.

Environment:

• npm: 7.5.2

Problem statement

Today we allow users to install npm even though they might be on a version of node that we don't want to or can't support (ex. node@<10).

Features

• Test adding a preinstall script to npm@7 which warns/errors our of unsupported environment

Exit criteria

• script should not allow you to end up in a broken state/environment
• warning should provide context/details on how to fix or upgrade

[ljharb]

to clarify, is this a regression from npm 6?

[wraithgar]

No npm 6 will happily try to install npm 7 if you are running in node 8, even though the engines entry in npm 7 says "node": ">=10"

[ljharb]

hm, @isaacs has implied many times that engines.npm is special and always behaves like enginesStrict :-/

[wraithgar]

This is for engines.node not engines.npm.

[ljharb]

ahhh thank you for clarifying.

[wraithgar]

Current thinking is that if a pre-install script did this check, we would not be required to be using npm7 itself to do the install in order to benefit from this. The version that's being installed can do this check.

[wraithgar]

Trip report: adding a preinstall script that fails makes the global install of the npm cli break (which it already currently does), but it now breaks in such a way that it rolls back and if you are using nvm it decides that node version is invalid and switches you back to default and you can't even get back to node 8 to run the install.sh script.

I think the nvm version switching was just me juggling terminals wrong. It still does break npm so this isn't going to work as originally planned, see next comment for sh output.

[ljharb]

I’m confused; how does the preinstall script break nvm?

[wraithgar]

Here's what it does

~/D/npm $ npm install ./cli/npm-7.5.3.tgz -g

> npm@7.5.3 preinstall /Users/wraithgar/.nvm/versions/node/v8.17.0/lib/node_modules/npm
> node scripts/preinstall.js

Error: Unsupported engine
Please see "Requirements" on https://www.npmjs.com/package/npm for more information about what versions of node are supported

npm WARN notsup Unsupported engine for npm@7.5.3: wanted: {"node":">=10"} (current: {"node":"8.17.0","npm":"6.14.11"})
npm WARN notsup Not compatible with your version of node/npm: npm@7.5.3

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! npm@7.5.3 preinstall: `node scripts/preinstall.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the npm@7.5.3 preinstall script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/wraithgar/.npm/_logs/2021-02-09T17_01_27_207Z-debug.log
~/D/npm $ npm
TypeError: Cannot destructure property `stat` of 'undefined' or 'null'.

it's totally gone from node_modules now

$ ls /Users/wraithgar/.nvm/versions/node/v8.17.0/lib/node_modules/
json/

I'm gonna make the PR for this since it's already pushed and we can debug it there.

[wraithgar]

Just a note you can't npm install ./cli itself because there are dev dependencies that cause other install errors before the preinstall even runs. This is why I used a tgz in the example, I npm pack --production in the PR branch to test it.

[wraithgar]

We also can't patch 6 because the user story this is meant to fix is someone w/ the existing npm 6 who is running npm install -g npm@latest, they'd never get this interim npm@6.

[wraithgar]

New plan: we are going to make npm 7 treat itself as a special case when installing globally. If you are installing npm itself, globally, it will do a very strict, very early, engines check.

We will also update install.sh so that it does this same check, repurposing the script in #2661 for use there instead of as a preinstall

[darcyclarke]

Believe this got fixed/landed in v7.23.0 - ref. #3731