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

[8.x] assert: revert breaking change #24786

Closed
wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 2, 2018

It was not intended to change the assert.doesNotThrow() message
with #23223. This reverts the
breaking behavior to the one before.

Refs: #23223 (comment)

I guess it won't hurt much if we keep this change in 8.x but it was not meant to be
in 8 and therefore I opened this revert.

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

It was not intended to change the `assert.doesNotThrow()` message
with nodejs#23223. This reverts the
breaking behavior to the one before.
@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. v8.x labels Dec 2, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 2, 2018

FYI @nodejs/release

@BethGriggs
Copy link
Member

I'd be +1 on reverting

@rvagg
Copy link
Member

rvagg commented Dec 3, 2018

This is in 8.x releases now? For how long? Fixing it is going to be another breaking change for folks who have adapted to the change. Since we've also done a security release I would hope the majority of users are on the latest version so have already adapted.
I'm inclined to leave it. Maybe even go back to the release blog post and put an update note about it (just a line in italics with the of the note).

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 3, 2018

It was released in 8.13 which is not yet two weeks old. I honestly have no strong opinion either way. So maybe we should just vote?

🎉 revert
👍 keep the breaking change

@sam-github
Copy link
Contributor

The actual change: https://github.com/nodejs/node/pull/24786/files

Summary: it appends more info to the .message property of the error thrown from an assert.

A nice change, but changes to .message are defined as semver-major by policy. Should revert, IMO.

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 4, 2018

Let's wait 24h more hours but I suggest to have a new patch release very soon in case we decide to revert.

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 5, 2018

There are 4 collaborators who would like to revert and 1 who would like to keep the current change. Since it's out there for two weeks only, I think it's best to revert this.

@BethGriggs AFAIK you'll prepare the next 8.x.x release: would you be so kind and include this PR in that case as well?

@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
@MylesBorins
Copy link
Contributor

@BridgeAR I think what @rvagg meant was that it went out with the v8.14.0 release, which as a security release likely had more upgrades then a normal release. As such people may have already adapted.

Alternatively, unexpected breaking changes could stop people from adopting a security release.

I genuinely doubt that this is breaking lots of production stuff, although I imagine it could break some tests in an annoying way.

I've kicked off a CI: https://ci.nodejs.org/job/node-test-pull-request/192561

If it is green and there are no objections I think we should consider landing it. It would be nice to have more explicit sign-off before doing so.

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 6, 2018

CI is green.

BethGriggs pushed a commit that referenced this pull request Dec 11, 2018
It was not intended to change the `assert.doesNotThrow()` message
with #23223. This reverts the
breaking behavior to the one before.

PR-URL: #24786
Refs: #23223
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@BethGriggs
Copy link
Member

Landed in 62fb5db

@BethGriggs BethGriggs closed this Dec 11, 2018
BethGriggs added a commit that referenced this pull request Dec 11, 2018
Notable changes:

* **assert**:
  - revert breaking change (Ruben Bridgewater)
    [#24786](#24786)
* **http2**:
  - fix sequence of error/close events (Gerhard Stoebich)
    [#24789](#24789)

PR-URL: #24832
MylesBorins pushed a commit that referenced this pull request Dec 18, 2018
Notable changes:

* **assert**:
  - revert breaking change (Ruben Bridgewater)
    [#24786](#24786)
* **http2**:
  - fix sequence of error/close events (Gerhard Stoebich)
    [#24789](#24789)

PR-URL: #24832
MylesBorins pushed a commit that referenced this pull request Dec 18, 2018
Notable changes:

* **assert**:
  - revert breaking change (Ruben Bridgewater)
    [#24786](#24786)
* **http2**:
  - fix sequence of error/close events (Gerhard Stoebich)
    [#24789](#24789)

PR-URL: #24832
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Notable changes:

* **assert**:
  - revert breaking change (Ruben Bridgewater)
    [nodejs#24786](nodejs#24786)
* **http2**:
  - fix sequence of error/close events (Gerhard Stoebich)
    [nodejs#24789](nodejs#24789)

PR-URL: nodejs#24832
@BridgeAR BridgeAR deleted the revert-major-change branch January 20, 2020 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants