FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: remove deprecated `node:sys` module by anonrig · Pull Request #49520 · nodejs/node · GitHub

/ node Public

lib: remove deprecated node:sys module - #49520

Closed
anonrig wants to merge 1 commit into
nodejs:mainfrom
anonrig:remove-node-sys
ConversationCommits (1)ChecksFiles changed
Closed

lib: remove deprecated node:sys module#49520
anonrig wants to merge 1 commit into
nodejs:mainfrom
anonrig:remove-node-sys

Conversation

anonrig commented Sep 6, 2023

Copy link
Copy Markdown
Member

sys module has been deprecated for a really long time (since node 6). I propose removing it with Node 21.

cc @nodejs/tsc

anonrig added semver-major PRs that contain breaking changes and should be released in the next major version. deprecations Issues and PRs related to deprecations. labels Sep 6, 2023
nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 6, 2023
mcollina approved these changes Sep 6, 2023
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

benjamingr commented Sep 6, 2023

Copy link
Copy Markdown
Member

@sindresorhus for the npm module update (probably lets you delete a line or some such)

benjamingr commented Sep 6, 2023

Copy link
Copy Markdown
Member

@ljharb also pinging you for https://github.com/inspect-js/is-core-module/blob/main/core.json

ljharb commented Sep 6, 2023

Copy link
Copy Markdown
Member

Thanks, I'll update is-core-module once this is merged.

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2023

nodejs-github-bot commented Sep 6, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53770/

MoLow approved these changes Sep 6, 2023
View reviewed changes

MoLow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. Should we run a CITGM before landing?

mscdex added the needs-citgm PRs that need a CITGM CI run. label Sep 6, 2023
aduh95 requested changes Sep 6, 2023
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm going to add the traditional -1 on that, see #35407 (comment) for more context.

ljharb commented Sep 6, 2023

Copy link
Copy Markdown
Member

lol, i forgot i'm the one who tried this last time :-p

Trott requested changes Sep 6, 2023
View reviewed changes

Trott left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If we remove the internal sys module, then require('sys') can load code from node_modules, needlessly introducing supply-chain vulnerabilities to people running old code. The maintenance cost of keeping sys around is effectively zero (except for pull requests like this that comes up every once in a while). The cost to the ecosystem of removing it is potentially significant. Therefore, we should never remove it unless something happens to cause the maintenance cost to increase, but that seems very unlikely.

We can change it from a deprecation warning to throwing an error if we want to push people a little harder to stop using it. I'm not sure there's much of a point to that, though. If we're never going to remove it, why give people additional grief for using it?

anonrig closed this Sep 8, 2023
anonrig deleted the remove-node-sys branch September 8, 2023 22:22

benjamingr commented Sep 10, 2023

Copy link
Copy Markdown
Member

Can't we ask @sh1mmer for the sys package on npm if he doesn't mind? It looks unused anyway and he probably squatted it to prevent abuse/malware

Trott commented Sep 10, 2023

Copy link
Copy Markdown
Member

Can't we ask @sh1mmer for the sys package on npm if he doesn't mind? It looks unused anyway and he probably squatted it to prevent abuse/malware

That mitigates but does not eliminate the risk for users. Attackers can still find other ways to create node_modules/sys. If removing sys was a big win for Node.js maintenance, then it would be worth it. (We should probably politely and respectfully ask the maintainer for the module name anyway, and the same with any other imodule names on npm that we don't own and that clash with built-in module names.)

The maintenance cost of keeping sys around is negligible. So let's not remove it. Removing it would have no significant benefit for the maintainers or users of Node.js, but it would increase the opportunity for attackers.

ronag commented Sep 10, 2023

Copy link
Copy Markdown
Member

An alternative is to have a separate list of explicitly reserved module names.

GeoffreyBooth commented Sep 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Let’s add a comment to the code along the lines of

// ! Before you propose removing this, read https://github.com/nodejs/node/pull/35407#issuecomment-700693439

richardlau mentioned this pull request Apr 24, 2024
Closed
RafaelGSS mentioned this pull request Sep 3, 2024
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott requested changes
aduh95 aduh95 requested changes
mcollina mcollina approved these changes
benjamingr benjamingr approved these changes
MoLow MoLow approved these changes

Assignees

No one assigned

Labels

deprecations Issues and PRs related to deprecations. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL