lib: end-of-life the deprecated require('sys') #26292

sam-github · 2019-02-24T19:05:07Z

See: #2405

It looks like this was going to be considered for Node.js 6.x, so now that 12.x is upcoming, maybe we are ready?

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

See: nodejs#2405

nodejs-github-bot · 2019-02-24T19:05:08Z

@sam-github sadly an error occured when I tried to trigger a build :(

vsemozhetbyt · 2019-02-24T19:27:54Z

doc/api/deprecations.md

@@ -561,7 +564,7 @@ changes:
    description: Runtime deprecation.
 -->

-Type: Runtime
+Type: End-of-Life

 The `sys` module is deprecated. Please use the [`util`][] module instead.


Maybe:

Suggested change

The `sys` module is deprecated. Please use the [`util`][] module instead.

The `sys` module was removed. Please use the [`util`][] module instead.

(Not sure where the leading space emerges from)

targos · 2019-02-24T20:18:55Z

.eslintrc.js

@@ -137,7 +137,7 @@ module.exports = {
    'no-path-concat': 'error',
    'no-proto': 'error',
    'no-redeclare': 'error',
-    'no-restricted-modules': ['error', 'sys'],
+    'no-restricted-modules': ['error'],


Suggested change

'no-restricted-modules': ['error'],

'no-restricted-modules': 'error',

This line should be deleted entirely. If you don't specify a second argument, the rule does nothing.

targos · 2019-02-24T20:19:18Z

doc/api/deprecations.md

@@ -551,6 +551,9 @@ The `REPLServer.prototype.convertToContext()` API has been removed.
 ### DEP0025: require('sys')
 <!-- YAML
 changes:
+  - version: REPLACEME
+    pr-url: XXX


Suggested change

pr-url: XXX

pr-url: https://github.com/nodejs/node/pull/26292

Fishrock123 · 2019-02-24T22:52:50Z

i know there are still modules that use this 😐

Trott

The maintenance burden of the sys module is roughly zero. The cost to the ecosystem of removing sys is not zero. I don't think we should remove unless there is a compelling benefit to doing so. It's fine to leave it permanently deprecated.

Trott · 2019-02-25T05:20:28Z

I'm surprised test/parallel/test-sys.js does not show up as being removed in the diff. That test should fail with this change.

Fishrock123 · 2019-02-25T05:47:53Z

To be clear: I think there is a cost of keeping sys around. There is a cost of confusion to our users. I do not think this is good to keep going in perpetuity, as it will only become worse.

Trott · 2019-02-25T06:10:00Z

To be clear: I think there is a cost of keeping sys around. There is a cost of confusion to our users. I do not think this is good to keep going in perpetuity, as it will only become worse.

@Fishrock123 I'm surprised at the notion that the cost of confusion to users is significant in this instance. I have literally never heard of anyone being confused about sys vs. util since 4.0.0, probably even earlier than that. It doesn't seem to ever come up in the help repo, in this repo, in StackOverflow, or in IRC. Am I wrong?

Trott · 2019-02-25T06:31:45Z

Might not be a bad idea for someone to do something like https://github.com/evanlucas/find-sys all over again (maybe using gzemnid?).

bnoordhuis

We decided years ago to never give up the 'sys' namespace for security reasons but this PR appears to be doing just that. require('sys') (or any other built-in module) should never load third-party code.

jasnell

I agree with @bnoordhuis ... we cannot remove the module name completely.

We could however, upgrade from a deprecation warning to an actual thrown Error.

BridgeAR

Throwing an error as suggested by @jasnell seems best to me instead of freeing the namespace.

cjihrig · 2019-02-25T16:21:12Z

I agree that we should not give up the name sys. Assuming that we don't give up the name, throwing an error doesn't seem productive... we might as well continue with the existing behavior.

sam-github · 2019-02-25T19:02:56Z

I didn't think this would be so controversial! My reading of #2405 was that it was likely to be deleted for 6.x, so 12.x seemed like a pretty safe overshoot.

Anybody using sys has been getting console warnings that its deprecated for 3.5+ years. Personally, I think that's a fair amount of time to update, but I don't feel really strongly about it. I can add a test that the deprecation is emitted and call it a day if that's what the consensus is.

The presence of sys has some cost, at the very least, that _linkedlist got deleted and _stream_wrap.js is about to get deprecated along with #2405's discussion made me think this was the time to cleanup sys, so it cost me some time. I guess a comment in the source saying "we'll never delete this, please don't try, see #26292" would be good if that's the decision

If there is agreement to delete it, I'll fix up the PR, but I'm not going to lose more time on it if there isn't. I'll wait for more comment, or maybe it needs a TSC agenda label?

@cjihrig @bnoordhuis It wasn't my intention to give up sys, that was an oversight, if we want to remove sys I'll fix that.
@vsemozhetbyt The text and metadata of the deprecation docs is identical to that used in the _linklist module end-of-life. If this text needs changing, I'll update the other one, too.
We gave up _linklist to npm, oversight, or not considered a security issue?

core/node (remove-deprecated-sys $%) % cat node_modules/_linklist/index.js
console.log('_linklist')
core/node (remove-deprecated-sys $%) % node -p 'require("_linklist")'
_linklist
{}

vsemozhetbyt · 2019-02-25T19:26:14Z

The text and metadata of the deprecation docs is identical to that used in the _linklist module end-of-life. If this text needs changing, I'll update the other one, too.

No strong opinion. Please, do as you would consider being better.

bnoordhuis · 2019-02-26T03:21:39Z

We gave up _linklist to npm, oversight, or not considered a security issue?

Oversight, I'm afraid. We should probably claim it while we still can. :-)

targos · 2019-02-26T07:02:04Z

We gave up _linklist to npm, oversight, or not considered a security issue?

Oversight, I'm afraid. We should probably claim it while we still can. :-)

We can't. npm doesn't allow package names that start with an underscore.

sam-github · 2019-02-27T03:26:44Z

I think I'll just leave this alone.

lib: end-of-life the deprecated require('sys')

f24105e

See: nodejs#2405

sam-github added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 24, 2019

nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Feb 24, 2019

vsemozhetbyt added the deprecations Issues and PRs related to deprecations. label Feb 24, 2019

vsemozhetbyt reviewed Feb 24, 2019

View reviewed changes

devsnek approved these changes Feb 24, 2019

View reviewed changes

targos reviewed Feb 24, 2019

View reviewed changes

tniessen approved these changes Feb 24, 2019

View reviewed changes

lpinca approved these changes Feb 24, 2019

View reviewed changes

Trott requested changes Feb 25, 2019

View reviewed changes

bnoordhuis requested changes Feb 25, 2019

View reviewed changes

cjihrig mentioned this pull request Feb 25, 2019

Fully deprecate require('constants') #26012

Closed

jasnell requested changes Feb 25, 2019

View reviewed changes

BridgeAR requested changes Feb 25, 2019

View reviewed changes

sam-github closed this Feb 27, 2019

sam-github deleted the remove-deprecated-sys branch February 27, 2019 03:26

bnoordhuis mentioned this pull request Sep 29, 2020

lib: remove deprecated 'sys' #35407

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: end-of-life the deprecated require('sys') #26292

lib: end-of-life the deprecated require('sys') #26292

sam-github commented Feb 24, 2019

nodejs-github-bot commented Feb 24, 2019

vsemozhetbyt Feb 24, 2019

vsemozhetbyt Feb 24, 2019

targos Feb 24, 2019

Trott Feb 25, 2019

targos Feb 24, 2019

Fishrock123 commented Feb 24, 2019

Trott left a comment •

edited

Loading

Trott commented Feb 25, 2019

Fishrock123 commented Feb 25, 2019 •

edited

Loading

Trott commented Feb 25, 2019

Trott commented Feb 25, 2019

bnoordhuis left a comment

jasnell left a comment

BridgeAR left a comment

cjihrig commented Feb 25, 2019

sam-github commented Feb 25, 2019

vsemozhetbyt commented Feb 25, 2019

bnoordhuis commented Feb 26, 2019

targos commented Feb 26, 2019

sam-github commented Feb 27, 2019

	The `sys` module is deprecated. Please use the [`util`][] module instead.
	The `sys` module was removed. Please use the [`util`][] module instead.

	'no-restricted-modules': ['error'],
	'no-restricted-modules': 'error',

	pr-url: XXX
	pr-url: https://github.com/nodejs/node/pull/26292

lib: end-of-life the deprecated require('sys') #26292

lib: end-of-life the deprecated require('sys') #26292

Conversation

sam-github commented Feb 24, 2019

Checklist

nodejs-github-bot commented Feb 24, 2019

vsemozhetbyt Feb 24, 2019

Choose a reason for hiding this comment

vsemozhetbyt Feb 24, 2019

Choose a reason for hiding this comment

targos Feb 24, 2019

Choose a reason for hiding this comment

Trott Feb 25, 2019

Choose a reason for hiding this comment

targos Feb 24, 2019

Choose a reason for hiding this comment

Fishrock123 commented Feb 24, 2019

Trott left a comment • edited Loading

Choose a reason for hiding this comment

Trott commented Feb 25, 2019

Fishrock123 commented Feb 25, 2019 • edited Loading

Trott commented Feb 25, 2019

Trott commented Feb 25, 2019

bnoordhuis left a comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

cjihrig commented Feb 25, 2019

sam-github commented Feb 25, 2019

vsemozhetbyt commented Feb 25, 2019

bnoordhuis commented Feb 26, 2019

targos commented Feb 26, 2019

sam-github commented Feb 27, 2019

Trott left a comment •

edited

Loading

Fishrock123 commented Feb 25, 2019 •

edited

Loading