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

sys: Removing after many years of deprecation #2405

Closed
wants to merge 2 commits into from
Closed

Conversation

geek
Copy link
Member

@geek geek commented Aug 17, 2015

Relates to nodejs/node-v0.x-archive#8880

Before we release a major version of node we should remove sys, which has been deprecated and undocumented for a long time.

@mscdex
Copy link
Contributor

mscdex commented Aug 17, 2015

+1

@Trott Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 17, 2015
@Trott
Copy link
Member

Trott commented Aug 17, 2015

Was the disagreement (about what is necessary and sufficient to finally do this) at #1704 ever resolved?

@ChALkeR
Copy link
Member

ChALkeR commented Aug 17, 2015

This will break some modules.
By grepping in my 2-month-old collection of current versions of «active» modules (the modules that were uploaded using npm2+, that's about 1/3 of all npm modules), I found a little more than 100 modules that directly require sys.

And not all of that are unmaintained and forgotten modules, for example, see pm2 (one, two).

Personally, I'm +1 for the removal. There is a sys module as the final work-around, and one could install that if he or she uses some unmaintained module that won't be updated in time.

I think I'll prepare an updated list of modules in some time, when I resync my local npm packages.

@ronkorving
Copy link
Contributor

Find all cases of require('sys') on GitHub and leave an issue? I for one am really +1 on this one.

@bnoordhuis
Copy link
Member

See #182. The TC resolution for that pull request was that deprecating 'sys' is okay (and that happened in #317) but removing it is not. It's not defective and it's not a maintenance burden.

@jaw187
Copy link

jaw187 commented Aug 17, 2015

Does it make sense to deprecate a module ..... forever?

@geek
Copy link
Member Author

geek commented Aug 17, 2015

@bnoordhuis Anything in the nodejs org will have a maintenance cost.

@ChALkeR thanks for pointing those cases out, I will submit a PR to pm2

@ChALkeR
Copy link
Member

ChALkeR commented Aug 17, 2015

Partial (and outdated) list of other modules: https://gist.github.com/ChALkeR/60c95b60bbfa3296c2f8

An updated list wil follow, it this won't be rejected by that time =).

@jasnell
Copy link
Member

jasnell commented Aug 22, 2015

@nodejs/tsc ... any objections to closing this based on the prior decision?

@geek
Copy link
Member Author

geek commented Aug 24, 2015

@jasnell I object.

We should do the right thing... @ChALkeR already submitted PRs to modules found to use sys. This module is safe to remove and should be removed... if we don't then we will end up having to submit more PRs to modules that end up finding and using sys as well as having conversations like this in the future.

@ChALkeR
Copy link
Member

ChALkeR commented Aug 24, 2015

@geek Correction: I did not submit PR to all modules found to use sys, but only to 6 of them.
Even the above-published list is not complete. I will post an updated list today.

@Fishrock123 Fishrock123 added this to the 6.0.0 milestone Aug 24, 2015
@Fishrock123
Copy link
Contributor

We need at least one node release with it issuing the runtime deprecation warning.

You can blame this commit for not re-deprecating it as a runtime warning.

@geek
Copy link
Member Author

geek commented Aug 24, 2015

@Fishrock123 it was released with a deprecated warning previously: dc42e1f

$ iojs -v
v3.1.0
$ iojs
> var sys = require('sys')
(node) sys is deprecated. Use util instead.

@Fishrock123
Copy link
Contributor

For the purposes of this, io.js does not count as a "node version", let's rephrase: an LTS cycle because past mistakes were stupid and we don't have control of that.

@bnoordhuis
Copy link
Member

A decision was made and I don't see a reason to revisit that. I'm okay with closing this.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 24, 2015

I notice this has been added to the 6.0.0 milestone. Assuming that this could actually be removed in v6, and @geek wanted to do the work, what is the best way to track long running issues? Closing it will likely lead to it being forgotten about, but keeping it open for over a year leads to a cluttered issue tracker.

@geek
Copy link
Member Author

geek commented Aug 24, 2015

@bnoordhuis we should listen to the decision of the community to not use sys. People want a clean codebase, not something with broken modules.

@trevnorris
Copy link
Contributor

@geek That's a bit extreme. Have you looked at lib/sys.js lately?

'use strict';
const util = require('internal/util');
module.exports = require('util');
util.printDeprecationMessage('sys is deprecated. Use util instead.');

That's the entire thing. It's a namespace change is all. We don't use sys internally, and I'm not sure what your comment about listening to the community is about. In short, it's not a broken module. It's an extra few lines of code to forward the namespace to the current module so we stay backwards compatible.

@geek
Copy link
Member Author

geek commented Aug 24, 2015

@trevnorris the community isn't using sys. Its a broke module in that when you try to use it you get a warning.

What is extreme about stating that?

@geek
Copy link
Member Author

geek commented Aug 24, 2015

@trevnorris removing this implicit API should be safe to do in v4, as it was part of our v1 release. These are the guidelines for the deprecation policy:

Backwards incompatible changes to Explicit APIs that have previously been included in a Release must follow a clear and predictable deprecation process in which the existing documented API: (a) is clearly marked for deprecation and (b) can change only after the next semver-major version increase.

Backwards incompatible changes to Implicit APIs that have been included in a Release should be handled the same as Explicit API changes.

It has now existed in an undocumented state for years as well as in an explicitly deprecated state for 3 major releases.

@Qard
Copy link
Member

Qard commented Aug 24, 2015

It's been in an explicitly deprecated state for io.js. There has never been a node release in which it was deprecated. First, we need to get it out as deprecated in v4, then we can consider removing it in v6.

@geek
Copy link
Member Author

geek commented Aug 24, 2015

@Qard sounds like a plan

@geek geek closed this Aug 24, 2015
sam-github added a commit to sam-github/node that referenced this pull request Feb 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.