-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Scripts: Update Node and NPM versions in check-engines script. #19680
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the guideline we're using as a reference here?
Is it based on the Getting Started recommendation of "latest active LTS"?
If so:
- Does this recommendation necessarily extend to
@wordpress/scripts
, which is intended to be used as a utility independent from Gutenberg? I think it's a reasonable case to be made that the defaults can adhere to Gutenberg's own guidelines - What about NPM version? Is that assumed to be the version shipped with the latest active LTS?
We used to be very explicit about this in the documentation:
- Current latest active LTS release
- Latest NPM version
This explicitness was lost as part of the changes of #17004. I reintroduced some in #18923, but apparently missed the NPM version. I think many of the issues we're seeing with frequent changes to package-lock.json
(example) can be attributed to the fact we don't really have explicit or enforced guidelines around this.
So, couple things I suggest:
Here:
- Decide if we anticipate we'd want to reintroduce "latest NPM" as the recommended version, and if so, update the NPM version to reflect (at least closer to) what that is currently (6.13.6).
- Ideally, this is something we don't have to maintain manually. See also related "Separately" item.
- Add a CHANGELOG entry, mostly in that I think we should consider whether this be treated as a breaking change, since we'd want to bump the major in the next publish
Separately:
- Clarify NPM version in "Getting Started" guide
- Add support to read from project local
.nvmrc
in this script (if it doesn't already exist) - If intending to have this follow Gutenberg recommendations, consider having these values be dynamically inferred (retrieve latest Node LTS version, retrieve latest NPM version). I seem to recall some discussion about this previously, so there could be some existing issue tracking it.
Issue: #14201 |
I opened this on a suggestion from @gziolo because it immediately fixes the incompatibility issues that were blocking contributions that added new dependencies with no apparent drawbacks. I think the version numbers are just the most conservative bump that fixes the issue. I think we should enforce LTS for everything and use nvm to resolve them, but that shouldn't block this PR. |
Note:I've created https://meta.trac.wordpress.org/ticket/4974 to have the w.org "build server" upgraded, merging this PR or #18048 and merging to w.org SVN may cause explosions before this upgrade occurs |
I think this is trickier, I think For instance, a project I'm working on uses When a Node.js LTS is dropped,
npm does not have LTS versions, just the latest version If my recollection serves me correctly npm dropped any LTS policies it previously had in npm 5.x |
@ntwb What's the status of this in Core? |
'--node', '>=10.0.0', | ||
'--npm', '>=6.0.0', | ||
'--node', '>=12', | ||
'--npm', '>=6.1', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We set the minimum version of npm to 6.9
to be able to use aliases with Prettier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
0f36426
to
543f368
Compare
No description provided.