Uh oh!
There was an error while loading. Please reload this page.
lib: remove deprecated node:sys module - #49520
Conversation
e2ba3ca to
a2af6d7Comparebenjamingr
commented
Sep 6, 2023
@sindresorhus for the npm module update (probably lets you delete a line or some such) |
benjamingr
commented
Sep 6, 2023
@ljharb also pinging you for https://github.com/inspect-js/is-core-module/blob/main/core.json |
ljharb
commented
Sep 6, 2023
Thanks, I'll update is-core-module once this is merged. |
nodejs-github-bot
commented
Sep 6, 2023
MoLow
left a comment
There was a problem hiding this comment.
LGTM. Should we run a CITGM before landing?
aduh95
left a comment
There was a problem hiding this comment.
I'm going to add the traditional -1 on that, see #35407 (comment) for more context.
ljharb
commented
Sep 6, 2023
lol, i forgot i'm the one who tried this last time :-p |
There was a problem hiding this comment.
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?
benjamingr
commented
Sep 10, 2023
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
That mitigates but does not eliminate the risk for users. Attackers can still find other ways to create The maintenance cost of keeping |
ronag
commented
Sep 10, 2023
An alternative is to have a separate list of explicitly reserved module names. |
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 |
sysmodule has been deprecated for a really long time (since node 6). I propose removing it with Node 21.cc @nodejs/tsc