Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: use NPM engine-strict=true #890

Closed
wants to merge 1 commit into from

Conversation

bmish
Copy link
Contributor

@bmish bmish commented Apr 9, 2022

Add NPM config engine-strict=true to prevent the npm install step during CI from succeeding if some dependencies aren't compatible with the Node version we are testing.

It turns out there are still numerous incompatibilities with Node 10 which release-it is supposed to support, as you can see from the Node 10 CI warnings output:

Run npm install
npm WARN deprecated coffee-script@1.12.7: CoffeeScript on NPM has moved to "coffeescript" (no hyphen)
npm WARN deprecated gulp-header@1.8.12: Removed event-stream from gulp-header
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN notsup Unsupported engine for compress-brotli@1.3.6: wanted: {"node":">= 12"} (current: {"node":"10.24.1","npm":"6.14.12"})
npm WARN notsup Not compatible with your version of node/npm: compress-brotli@1.3.6
npm WARN notsup Unsupported engine for lru-cache@7.8.0: wanted: {"node":">=12"} (current: {"node":"10.24.1","npm":"6.14.12"})
npm WARN notsup Not compatible with your version of node/npm: lru-cache@7.8.0
npm WARN notsup Unsupported engine for eslint-plugin-ava@13.2.0: wanted: {"node":">=12.22 <13 || >=14.17 <15 || >=16.4"} (current: {"node":"10.24.1","npm":"6.14.12"})
npm WARN notsup Not compatible with your version of node/npm: eslint-plugin-ava@13.2.0
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@~2.3.2 (node_modules/chokidar/node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@2.3.2: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})
npm WARN notsup Unsupported engine for espree@9.3.1: wanted: {"node":"^12.22.0 || ^14.17.0 || >=16.0.0"} (current: {"node":"10.24.1","npm":"6.14.12"})
npm WARN notsup Not compatible with your version of node/npm: espree@9.3.1
npm WARN notsup Unsupported engine for eslint-visitor-keys@3.3.0: wanted: {"node":"^12.22.0 || ^14.17.0 || >=16.0.0"} (current: {"node":"10.24.1","npm":"6.14.12"})
npm WARN notsup Not compatible with your version of node/npm: eslint-visitor-keys@3.3.0

By default, npm only warns on these incompatibilities, which is why we need the config option. Yarn fails on them by default, which is how I noticed release-it still lacked Node 10 support when trying to install it in my repository:

yarn install v1.22.18
error lru-cache@7.8.0: The engine "node" is incompatible with this module. Expected version ">=12". Got "10.24.1"

https://www.stefanjudis.com/today-i-learned/prevent-npm-install-for-not-supported-node-js-versions/#how-to-prevent-%60npm-install%60-with-an-unsupported-node.js-version

In order for CI in this PR to succeed, some dependencies will need to be downgraded.

Follow-up to #888 #886

@webpro
Copy link
Collaborator

webpro commented Apr 9, 2022

Thanks, I totally agree. Will investigate and fix. Didn't catch this, as I see don't get any warnings or errors with a clean install:

lars@Neo ~/p/release-it-14 master ❯ cat .npmrc 
save-exact=true
engine-strict=true
lars@Neo ~/p/release-it-14 master ❯ npm install
audited 633 packages in 3.418s

123 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

lars@Neo ~/p/release-it-14 master ❯ node -v
v10.24.1
lars@Neo ~/p/release-it-14 master ❯ npm -v
6.14.12
lars@Neo ~/p/release-it-14 master ❯ 

@bmish
Copy link
Contributor Author

bmish commented Apr 9, 2022

Try a fresh install (rm -rf node_modules) with Node 10 locally.

@webpro
Copy link
Collaborator

webpro commented Apr 9, 2022

Try a fresh install (rm -rf node_modules) with Node 10 locally.

I did, it was even a fresh clone. The output in the Action worries me a little:

npm WARN deprecated coffee-script@1.12.7: CoffeeScript on NPM has moved to "coffeescript" (no hyphen)
npm WARN deprecated gulp-header@1.[8](https://github.com/release-it/release-it/runs/5955867815?check_suite_focus=true#step:6:8).12: Removed event-stream from gulp-header
npm ERR! code ENOTSUP
npm ERR! notsup Unsupported engine for compress-brotli@1.3.6: wanted: {"node":">= 12"} (current: {"node":"10.24.1","npm":"6.14.12"})
npm ERR! notsup Not compatible with your version of node/npm: compress-brotli@1.3.6
npm ERR! notsup Not compatible with your version of node/npm: compress-brotli@1.3.6
npm ERR! notsup Required: {"node":">= 12"}
npm ERR! notsup Actual:   {"npm":"6.14.12","node":"10.24.1"}

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/runner/.npm/_logs/2022-04-0[9](https://github.com/release-it/release-it/runs/5955867815?check_suite_focus=true#step:6:9)T[16](https://github.com/release-it/release-it/runs/5955867815?check_suite_focus=true#step:6:16)_30_09_829Z-debug.log

I see that compress-brotli ("node": ">= 12") is some transitive dependency from got v11 ("node": ">=10.19.0"). This "strict" stuff isn't always helpful, as in this case release-it probably doesn't have any coverage/usage of the brotli package.

@bmish
Copy link
Contributor Author

bmish commented Apr 10, 2022

@webpro it's true that some transitive dependencies might not matter to us. We also don't care much about what Node versions our dev-dependencies support as they won't affect our consumers. But I'm not aware of any alternative to using the strict engine check to ensure we support the correct versions of Node, especially for our consumers using Yarn as they have the strict engine check enabled by default and will be prevented from installing it under Node 10.

To be fair, Node 10 is end-of-life already (and even Node 12 will be EOL soon), so this isn't a HUGE priority, but it would be nice to fix. I guess either of us can see if it's easy to fix in the coming days. And once release-it drops Node 10/12 support in the next major release, we can upgrade back to all the latest dependencies.

@webpro
Copy link
Collaborator

webpro commented Apr 10, 2022

Turns out npm v6 (which is the default shipped with Node.js 10) didn't support engine-strict=true

Anyway, I went down the rabbit hole in #892, any chance you could give that one a spin? Are you using Yarn v1?

@webpro webpro closed this Apr 10, 2022
@bmish
Copy link
Contributor Author

bmish commented Apr 11, 2022

@webpro yes I'm using Yarn v1. I tried that branch and it installed successfully with Yarn v1 and Node 10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants