Skip to content

Engines and Node 18+Windows tests#5475

Merged
wraithgar merged 3 commits intolatestfrom
lk/enginges
Sep 8, 2022
Merged

Engines and Node 18+Windows tests#5475
wraithgar merged 3 commits intolatestfrom
lk/enginges

Conversation

@lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Sep 6, 2022 •
edited
Loading

No description provided.

@lukekarrys lukekarrys requested a review from a team as a code owner September 6, 2022 21:21
@lukekarrys
Copy link
Contributor Author

lukekarrys commented Sep 6, 2022

Engines updates to workspaces will be updated by template-oss once npm/template-oss#180 lands.

@ljharb
Copy link
Contributor

ljharb commented Sep 6, 2022

Unfortunately nvm's CI on travis-ci is currently broken (because they changed a docker container out from under me) so i won't be able to cut a release until that's fixed, so nvm install-latest-npm will break for users on node ^12.13 (which includes all of my packages).

I don't think this is npm's problem to solve, to be clear, just giving a heads up.

ljharb
ljharb reviewed Sep 6, 2022
View reviewed changes
@lukekarrys
Copy link
Contributor Author

lukekarrys commented Sep 6, 2022

Unfortunately nvm's CI on travis-ci is currently broken (because they changed a docker container out from under me) so i won't be able to cut a release until that's fixed, so nvm install-latest-npm will break for users on node ^12.13 (which includes all of my packages).

This will be released as 9.0.0-pre.0 and pushed to the next-9 dist-tag initially and won't go out as 9.0.0 and latest for a little bit. Does that affect how install-latest-npm will work?

@ljharb
Copy link
Contributor

ljharb commented Sep 6, 2022

It won't break anything until it's published as "latest".

@wraithgar
Copy link
Member

wraithgar commented Sep 7, 2022

The thing that bit us in the past was when node-gyp added an engines declaration that was more restrictive than ours. We'll be more proactive about this in the future, making sure we speak up if that's about to happen again there, and we will just have to take other third party engines updates as they come. It doesn't appear many packages right now are adopting things in 16 that require them to set an engines more restrictive than >=16 so leaving this PR as-is will likely work.

The Pin to LTS decision of npm@8 was one born out of "we have to come up with something and this seems good". We know more now than we did then, including the fact that node backports new features in LTS lines after a version has gone into LTS. Pinning to LTS as the floor doesn't help us as much as we thought it did.

I think this PR should be merged as-is. We will likely be revisiting the engines field more often in the future than in the past so it won't be years before we can correct something.

@ljharb
Copy link
Contributor

ljharb commented Sep 7, 2022

Major versions are always hugely painful for the ecosystem and cause lots of churn. If a less restrictive engines range means there will be more major versions, then I implore you to make it as restrictive as possible, with the goal of having as few major bumps as possible.

@lukekarrys lukekarrys marked this pull request as draft September 7, 2022 20:20
@npm-cli-bot
Copy link
Collaborator

npm-cli-bot commented Sep 7, 2022 •
edited
Loading

found 1 benchmarks with statistically significant performance regressions

  • app-medium: cache-only
timing results
app-large clean lock-only cache-only cache-only
peer-deps		 modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 50.093 ±0.45 25.840 ±0.10 31.960 ±12.62 27.165 ±1.21 3.904 ±0.08 4.046 ±0.35 3.499 ±0.08 16.653 ±0.27 3.211 ±0.01 4.628 ±0.19
#5475 52.153 ±3.59 27.182 ±0.62 32.747 ±13.00 29.762 ±0.64 4.072 ±0.07 4.275 ±0.34 3.320 ±0.10 17.466 ±0.44 3.254 ±0.12 5.065 ±0.42
app-medium clean lock-only cache-only cache-only
peer-deps		 modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 36.229 ±0.83 21.259 ±1.24 17.386 ±0.44 17.929 ±0.83 3.428 ±0.03 3.515 ±0.05 3.239 ±0.13 11.567 ±0.12 2.976 ±0.00 4.185 ±0.01
#5475 36.670 ±0.55 22.176 ±0.31 19.777 ±0.32 19.498 ±1.66 3.630 ±0.00 3.691 ±0.01 3.493 ±0.23 12.653 ±0.12 3.264 ±0.04 4.383 ±0.06

@lukekarrys lukekarrys changed the title feat: update node engines in package.json Engines and Node 18+Windows tests Sep 7, 2022
@lukekarrys lukekarrys marked this pull request as ready for review September 7, 2022 21:24
wraithgar
wraithgar requested changes Sep 8, 2022
View reviewed changes
Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second commit should be a fix and include info about what changed in lib/cli.js

wraithgar and others added 3 commits September 8, 2022 09:06
@wraithgar
feat: update supported node engines in package.json
672e11f 
This also replaces the previous check for known broken versions of node
with an exception handler for syntax errors in order to try and give a
nicer error message when attempting to run npm on older node versions.

BREAKING CHANGE: `npm` is now compatible with the following semver range
for node: `^14.17.0 || ^16.13.0 || >=18.0.0`

Ref: npm/statusboard#519
@wraithgar
fix: attempt more graceful failure in older node versions
1ea3c2c
@lukekarrys @wraithgar
chore: fix failing tests in windows + node 18
036d25c 
Some of our tests were failing in windows after testing on node 18. The
reason was the inability to clean up the logs dir. This changes forces
a few tests to run in order and also cleans up any use of multiple
`t.testdir` calls in a single child test which can cause problems.
@wraithgar wraithgar self-requested a review September 8, 2022 16:08
@wraithgar
Copy link
Member

wraithgar commented Sep 8, 2022

Pulled the cli.js changes into a separate fix

wraithgar
wraithgar approved these changes Sep 8, 2022
View reviewed changes
Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting on CI, everything looks good now.

@wraithgar wraithgar merged commit c927afe into latest Sep 8, 2022
@wraithgar wraithgar deleted the lk/enginges branch September 8, 2022 16:21
This was referenced Sep 8, 2022
Merged
Closed
Open
@lukekarrys lukekarrys mentioned this pull request Nov 1, 2022
Closed
@npm-cli-bot npm-cli-bot mentioned this pull request Nov 5, 2022
Closed
This was referenced Nov 5, 2022
Closed
Closed
@npm-cli-bot npm-cli-bot mentioned this pull request Nov 16, 2022
Closed
@npm-cli-bot npm-cli-bot mentioned this pull request Nov 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wraithgar wraithgar wraithgar approved these changes

+1 more reviewer

@ljharb ljharb ljharb left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lukekarrys @ljharb @wraithgar @npm-cli-bot