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

feat: drop node 10, 11, and programmatic api #3762

Merged
merged 1 commit into from Oct 7, 2021
Merged

Conversation

wraithgar
Copy link
Member

@wraithgar wraithgar commented Sep 15, 2021

BREAKING CHANGE: This will drop official support for node versions less
than v12. It also removes the main export as something that will
work, 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.

@wraithgar wraithgar requested a review from a team as a code owner September 15, 2021 19:14
@wraithgar
Copy link
Member Author

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.

Copy link
Collaborator

@ljharb ljharb left a 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
Comment on lines 47 to 52
".": [
{
"default": "./lib/npm.js"
"default": "./lib/no-api.js"
},
"./lib/npm.js"
"./lib/no-api.js"
],
Copy link
Collaborator

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').

Copy link
Member Author

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"
Copy link
Collaborator

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.

Copy link
Member Author

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.

@isaacs
Copy link
Contributor

isaacs commented Sep 15, 2021

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 node . in the npm project, but require("npm") would still produce the desired explosion.

$ node . --version
7.23.0

$ node -e 'require("./")'
/Users/isaacs/dev/npm/cli/index.js:4
  throw new Error('The programmatic API was removed in npm v8.0.0')
  ^

Error: The programmatic API was removed in npm v8.0.0
    at Object.<anonymous> (/Users/isaacs/dev/npm/cli/index.js:4:9)
    at Module._compile (node:internal/modules/cjs/loader:1095:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1124:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:816:12)
    at Module.require (node:internal/modules/cjs/loader:999:19)
    at require (node:internal/modules/cjs/helpers:93:18)
    at [eval]:1:1
    at Script.runInThisContext (node:vm:129:12)
    at Object.runInThisContext (node:vm:305:38)

@wraithgar
Copy link
Member Author

I still like the idea of leaving the ci changes in there to be extremely explicit

@wraithgar wraithgar marked this pull request as draft September 20, 2021 13:54
@darcyclarke darcyclarke added semver:major backwards-incompatible breaking changes Release 8.x work is associated with a specific npm 8 release labels Sep 23, 2021
@isaacs
Copy link
Contributor

isaacs commented Sep 28, 2021

altho to be fair, the fork should be that "bin" points to the CLI and "main" only points to the programmatic API, rather than using require.main :-)

For external users, sure. But node . <args> is really handy while developing the npm cli.

@wraithgar wraithgar marked this pull request as ready for review October 5, 2021 21:01
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@fritzy fritzy left a 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
@wraithgar wraithgar merged commit a13d9d5 into release-next Oct 7, 2021
@wraithgar wraithgar deleted the gar/node-12 branch October 7, 2021 16:07
gfyoung added a commit to forking-repos/cli that referenced this pull request Oct 7, 2021
gfyoung added a commit to forking-repos/cli that referenced this pull request Oct 7, 2021
wraithgar pushed a commit that referenced this pull request Oct 11, 2021
xref: #3762

PR-URL: #3861
Credit: @gfyoung
Close: #3861
Reviewed-by: @wraithgar
@sancarn
Copy link

sancarn commented Nov 9, 2021

Out of curiosity, why was the API dropped?

@ljharb
Copy link
Collaborator

ljharb commented Nov 9, 2021

@sancarn ftr, it hasn't worked reliably nor been supported (or under semver) since npm 2 or 3.

@wraithgar
Copy link
Member Author

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

@relu91
Copy link

relu91 commented Feb 22, 2022

@sancarn ftr, it hasn't worked reliably nor been supported (or under semver) since npm 2 or 3.

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 npm API (see global-npm dependents is there any chance that we can have the programmatic API back? Or do you have any suggestions for workarounds? is the child_process.exec() function an option?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 8.x work is associated with a specific npm 8 release semver:major backwards-incompatible breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants