lib: remove deprecated node:sys module#49520
Conversation
e2ba3ca to
a2af6d7
Compare
|
@sindresorhus for the npm module update (probably lets you delete a line or some such) |
|
@ljharb also pinging you for https://github.com/inspect-js/is-core-module/blob/main/core.json |
|
Thanks, I'll update is-core-module once this is merged. |
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.
|
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?
|
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 The maintenance cost of keeping |
|
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