-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
feat: drop node 10, 11, and programmatic api #3762
Conversation
This is an exploratory PR to test CI, discuss if we are ready for this, and start giving people a heads up that this is on our radar. |
71c9508
to
2404704
Compare
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.
Please do give me as much of a heads up as possible before npm 8 being released and marked as latest
, so i can update nvm install-latest-npm
:-)
package.json
Outdated
".": [ | ||
{ | ||
"default": "./lib/npm.js" | ||
"default": "./lib/no-api.js" | ||
}, | ||
"./lib/npm.js" | ||
"./lib/no-api.js" | ||
], |
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.
you could also just remove the .
section entirely, and modern node will error on require('npm')
.
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.
This way the error shows that it's intentional, and explains when it was dropped.
@@ -235,6 +235,6 @@ | |||
}, | |||
"license": "Artistic-2.0", | |||
"engines": { | |||
"node": ">=10" | |||
"node": "^12.13.0 || ^14.15.0 || >=16" |
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.
Conditional exports weren't added until 12.17, and pattern trailers 12.20; imports
was added in 12.19 - in case you want to raise the threshold farther.
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.
I think "when that version entered LTS" is the least distruptive and easiest to argue for.
It seems like we can make the patch a lot smaller by doing something like this So we wouldn't have to update CI or break the habit of typing
|
I still like the idea of leaving the ci changes in there to be extremely explicit |
For external users, sure. But |
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.
I read through the diff, then I ran and explored it locally. I asked questions in slack.
LGTM
BREAKING CHANGE: - Drop official support for node versions less than v12. - Drop support for `require('npm')` - Update a few subdependencies that dropped node10 support, and brought in the latest node-gyp PR-URL: #3762 Credit: @wraithgar Close: #3762 Reviewed-by: @fritzy
xref: #3762 PR-URL: #3861 Credit: @gfyoung Close: #3861 Reviewed-by: @wraithgar
Out of curiosity, why was the API dropped? |
@sancarn ftr, it hasn't worked reliably nor been supported (or under semver) since npm 2 or 3. |
We are also going to be refactoring and cleaning up npm's internals. Dropping the api support allows us to do this iteratively instead of trying to cram it all into a single semver major release. cf #3959 |
Well as far as I can tell it worked...(maybe it wasn't a bug but a feature? 😄 ). Also, considering that there are at least 35 packages that are depending on |
BREAKING CHANGE: This will drop official support for node versions less
than v12. It also removes the
main
export as something that willwork, effectively dropping a supported programmatic api via
require('npm')
Dependency changes are all updates to get on the latest node-gyp, and while making that change sync those packages' engines/ci environments with this one. No functional changes.