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.

Sign up for GitHub

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

Jump to bottom

Engines and Node 18+Windows tests #5475

Merged
merged 3 commits into from
Sep 8, 2022
Merged

Engines and Node 18+Windows tests #5475

merged 3 commits into from
Sep 8, 2022

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

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
package-lock.json Outdated Show resolved Hide resolved
@lukekarrys
Copy link
Contributor Author

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

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

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
Open
Closed
Open
@lukekarrys lukekarrys mentioned this pull request Nov 1, 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

@ljharb ljharb ljharb left review comments

@wraithgar wraithgar wraithgar approved these changes

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