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

sys: Removing after many years of deprecation by geek · Pull Request #2405 · nodejs/node · GitHub

Repository navigation

sys: Removing after many years of deprecation - #2405

Closed
geek wants to merge 2 commits into
nodejs:masterfrom
geek:master
Closed

geek wants to merge 2 commits into
nodejs:masterfrom
geek:master

Conversation

geek commented Aug 17, 2015

Copy link
Copy Markdown
Member

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 commented Aug 17, 2015

Copy link
Copy Markdown
Contributor

+1

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

Trott commented Aug 17, 2015

Copy link
Copy Markdown
Member

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

ChALkeR commented Aug 17, 2015

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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 commented Aug 17, 2015

Copy link
Copy Markdown

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

geek commented Aug 17, 2015

Copy link
Copy Markdown
Member Author

@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 commented Aug 17, 2015

Copy link
Copy Markdown
Member

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 commented Aug 22, 2015

Copy link
Copy Markdown
Member

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

geek commented Aug 24, 2015

Copy link
Copy Markdown
Member Author

@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 commented Aug 24, 2015

Copy link
Copy Markdown
Member

@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 added this to the 6.0.0 milestone Aug 24, 2015

Copy link
Copy Markdown
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 commented Aug 24, 2015

Copy link
Copy Markdown
Member Author

@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.

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Member

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

cjihrig commented Aug 24, 2015

Copy link
Copy Markdown
Contributor

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 commented Aug 24, 2015

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
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 commented Aug 24, 2015

Copy link
Copy Markdown
Member Author

@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 commented Aug 24, 2015

Copy link
Copy Markdown
Member Author

@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 commented Aug 24, 2015

Copy link
Copy Markdown
Member

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 commented Aug 24, 2015

Copy link
Copy Markdown
Member Author

@Qard sounds like a plan

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
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.


Back | FazBrowse Home | New Git URL