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

console: make .assert standard compliant #17706

Closed

Conversation

BridgeAR
Copy link
Member

The standard does not throw and has no stack trace.
See https://console.spec.whatwg.org/#assert

This was mentioned before and the "documentation fix" in #6169 was not seen as a good solution by everyone and I would also prefer the way the standard behaves.

This is clearly a BC but I doubt that this is really a troublemaker.

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)

console

@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 16, 2017
@nodejs-github-bot nodejs-github-bot added the console Issues and PRs related to the console subsystem. label Dec 16, 2017
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Will definitely need some good citgm runs on this.

@BridgeAR BridgeAR requested a review from a team December 18, 2017 16:35
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.

LGTM

* `message` {any}
* `...args` {any}
* `value` {any} The value tested for being truthy.
* `...message` {any} All arguments besides the first are used as error message.
Copy link
Member

Choose a reason for hiding this comment

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

This feels a tiny bit confusing just because it says "all arguments besides the first." Is there a way we could reword?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am open for suggestions ;-)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like "All arguments besides value are used as error message."? Just to make it clear that first = value not the first argument within message.

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

@BridgeAR
Copy link
Member Author

New CI before landing due to weird build failures from before
https://ci.nodejs.org/job/node-test-commit-light/49/

@BridgeAR
Copy link
Member Author

@BridgeAR
Copy link
Member Author

Landed in 15d880b

@BridgeAR BridgeAR closed this Dec 26, 2017
BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 26, 2017
The standard does not throw and has no stack trace.
See https://console.spec.whatwg.org/#assert

PR-URL: nodejs#17706
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BridgeAR BridgeAR deleted the console-assert-spec-compliant 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
console Issues and PRs related to the console 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.

5 participants