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

process: deprecate process assert #18666

Closed
wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Feb 9, 2018

This was never documented and the assert module should be used instead. So let us just deprecate this.

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
Affected core subsystem(s)

process

@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Feb 9, 2018
@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 9, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 9, 2018

@cjihrig
Copy link
Contributor

cjihrig commented Feb 9, 2018

If this is going to be removed, I don't think it makes sense to also swap out the underlying assertion function.

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 9, 2018

@cjihrig the reason why I did this is because there are still two assert calls in there and it is semver-major anyway. But I can keep the function as it was before if you prefer that.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 9, 2018

I'd prefer leaving the implementation as is, but I'm fine with deprecating. Should probably run CITGM just in case.

@addaleax
Copy link
Member

addaleax commented Feb 9, 2018

So let us just deprecate this.

But why? Continuing to support it indefinitely is basically free.

@bnoordhuis
Copy link
Member

It's also used quite a bit out in the wild. I don't feel strongly about it but I don't see a compelling reason to deprecate it, except maybe for OOWTDI (Only One Way To Do It.)

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 9, 2018

I rebased due to conflicts and addressed the comment to keep the old function in place.

I personally definitely prefer OOWTDI, especially because this is much more limited than the assert module.

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 9, 2018

@BridgeAR BridgeAR requested a review from a team February 9, 2018 16:00
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@addaleax
Copy link
Member

addaleax commented Feb 9, 2018

I personally definitely prefer OOWTDI, especially because this is much more limited than the assert module.

I remember @bnoordhuis saying some time that that’s what documentation deprecations are there for, and I still agree with that. ;)

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 9, 2018

Light-CI https://ci.nodejs.org/job/node-test-commit-light/244/ (enough to test this change)
CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1270/

@addaleax I personally think it would be better to get the code ported to newer one and if it is once implemented, people will probably not realize that it is there.
I personally also feel like doc only deprecations only make sense in case the deprecated code was documented before and that is not the case here. Almost no one is ever going to check the deprecation list for such things.

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favour of this. I don't really like having a random function on the process object that seems to do a fraction of what the actual assert.ok does. Esp since it's not even documented...

@addaleax
Copy link
Member

addaleax commented Feb 9, 2018

Taking the more general discussion here to #18682.

Fwiw, my personal preference would be process.assert = assert.ok;, or as an non-enumerable property if we do want to hide it.

@apapirovski
Copy link
Member

Just to be clear, process.assert was deprecated back in 0.3.7. That's 7 years ago. I see no harm in a runtime deprecation and eventual removal. It's been long enough. Pretty sure the only reason it's still here is because most maintainers don't even know it exists. That's not, in any way, a good thing.

@BridgeAR BridgeAR requested a review from a team February 10, 2018 01:41
Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Usage is really low.

@ChALkeR
Copy link
Member

ChALkeR commented Feb 10, 2018

Grep for process\.assert (data from 2017-08-02 😢):

chrome-js-0.2.0.tgz/tests/src/test-NodeWebKit-Service.coffee:37:      nodeWebKit.process.assert_Is_Object()
nwr-0.1.8.tgz/tests/src/test-NodeWebKit-Service.coffee:37:      nodeWebKit.process.assert_Is_Object()
binomial-hash-list-4.1.4.tgz/ranges.js:4:var assert = process.assert
biojs-vis-blast-0.1.5.tgz/node/src/node.js:261:    assert = process.assert = function(x, msg) {
cessnalibjs-0.0.30.tgz/node.js:298:    assert = process.assert = function(x, msg) {
changesets-1.0.2.tgz/test.js:20:process.assert(textFinal_2 === 'Good day treasured adventurers, y\'all!')
flush-all-0.1.1.tgz/node-v0.13/src/node.js:277:    assert = process.assert = function(x, msg) {
gzip-stack-1.0.0.tgz/gzip-stack.js:17:    process.assert(compression >= 1 && compression <= 9);
kinto-9.0.2.tgz/blah/union/examples/simple/middleware/gzip-encode.js:14:    process.assert(compression >= 1 && compression <= 9);
nltk-0.0.1.tgz/test/ngram.js:5:process.assert(tri.enum(str).length === str.length - 2 + 1);
persha-0.2.3.tgz/node-lib/node.js:263:    assert = process.assert = function(x, msg) {
pezhu-0.0.0.tgz/Downloads/node-v0.9.11/src/node.js:285:    assert = process.assert = function(x, msg) {
pn-1.0.0.tgz/process.js:27:  assert: { enumerable: true, value: bind(process, process.assert) },
portable-js-0.0.3.tgz/misc/node/node.js:264:        assert = process.assert = function(x, msg) {
postgres-0.0.1.tgz/postgres.js:32:    process.assert(c.currentQuery);
tempisfugit-0.0.1.tgz/lib/types/commit.js:2:    assert = process.assert;
union-0.4.6.tgz/examples/simple/middleware/gzip-encode.js:14:    process.assert(compression >= 1 && compression <= 9);

And half of those is from Node.js copies, tests, or examples, others are abadoned and unused.

I don't see a reason for not runtime-deprecating it.
Runtime-deprecating is positive not only because it is a step forward removing obsolete code, but also because we better do that before someone accidentially starts to use it in a popular package and then we break/change/remove it (as it's an undocumented feature).

@devsnek
Copy link
Member

devsnek commented Feb 10, 2018

@ChALkeR unrelated to this pr, how do you grep packages like that

@ChALkeR
Copy link
Member

ChALkeR commented Feb 10, 2018

@devsnek Gzemnid. I intend to move it to the org (#7935), but it is a bit stuck there…

@ChALkeR
Copy link
Member

ChALkeR commented Feb 10, 2018

@devsnek I also upload grep-able datasets to http://oserv.org/npm/Gzemnid/, but that server is out of free space currently…

@BridgeAR
Copy link
Member Author

Rebased due to conflicts.

@addaleax is it OK for you if this lands as is or do you want to give a explicit -1?

@addaleax
Copy link
Member

addaleax commented Feb 10, 2018

@BridgeAR I would prefer to wait until #18682 is resolved, yes.

(Edit: I’m not putting a real -1 on this because of @ChALkeR’s Gzemnid run.)

@BridgeAR BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Feb 10, 2018
@BridgeAR
Copy link
Member Author

Sure :-)

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with moving forward if the analysis is that usage is very low.

@BridgeAR
Copy link
Member Author

@addaleax if I understand your update correct, you are now fine if this lands?

@addaleax
Copy link
Member

@BridgeAR Well, I’m not a fan, but I’m not going to block this either…

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels Feb 15, 2018
This was never documented and the `assert` module should be used
instead.
@BridgeAR
Copy link
Member Author

New CI before landing https://ci.nodejs.org/job/node-test-pull-request/13204/ (I rebased due to deprecations.md conflicts).

BridgeAR added a commit to BridgeAR/node that referenced this pull request Feb 16, 2018
This was never documented and the `assert` module should be used
instead.

PR-URL: nodejs#18666
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member Author

Landed in 703e37c

@BridgeAR BridgeAR closed this Feb 16, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
This was never documented and the `assert` module should be used
instead.

PR-URL: nodejs#18666
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR deleted the deprecate-process-assert branch April 1, 2019 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. process Issues and PRs related to the process subsystem. 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.