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

errors: multiple updates by jasnell · Pull Request #15002 · nodejs/node · GitHub

/ node Public

errors: multiple updates - #15002

Closed
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:improve-errors-internals
ConversationCommits (2)ChecksFiles changed
Closed

errors: multiple updates#15002
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:improve-errors-internals

Conversation

jasnell commented Aug 24, 2017

Copy link
Copy Markdown
Member

An alternative to #14350 ... eliminates the circular dependency with the assert module.

Also, export the defined error keys as constants. Some have said this makes it easier but I'm not fully convinced.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

errors

nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Aug 24, 2017
lpinca reviewed Aug 24, 2017
View reviewed changes
Comment thread lib/internal/errors.js Outdated

lpinca Aug 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This should be ===.

evanlucas Aug 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Actually, since this is loose equality, shouldn't it stay as ==? The current assert() function uses == as well (https://github.com/nodejs/node/blob/master/lib/assert.js#L78)

lpinca Aug 24, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@evanlucas if I'm not wrong there only assert.strictEqual() is currently used in this file.

Edit: nvm didn't see https://github.com/nodejs/node/pull/15002/files#diff-5a3344c263a73c663dd1cfcb91880fd7R70. Yes it's better to keep ==, ignore my comment.

refack Aug 29, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

IMHO since this is an internal module assertions should be made in /test/, so there's no need for runtime assertions at all.

jasnell commented Aug 29, 2017

Copy link
Copy Markdown
Member Author

@nodejs/collaborators ... ping!

lpinca approved these changes Aug 29, 2017
View reviewed changes

lpinca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. I'm only a bit hesitant about exporting the constants.

jasnell commented Aug 29, 2017

Copy link
Copy Markdown
Member Author

As am I, to be honest, I'm not convinced that it will be all that useful.

benjamingr approved these changes Aug 29, 2017
View reviewed changes

benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Not too worried about exporting the constants.

refack suggested changes Aug 29, 2017
View reviewed changes

refack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

IMHO assert should be removed for this module

Comment thread lib/internal/errors.js Outdated

refack Aug 29, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

IMHO since this is an internal module assertions should be made in /test/, so there's no need for runtime assertions at all.

jasnell commented Aug 29, 2017

Copy link
Copy Markdown
Member Author

@refack... that's certainly a valid argument also :-)

BridgeAR mentioned this pull request Aug 31, 2017
Closed
4 tasks

BridgeAR commented Aug 31, 2017

Copy link
Copy Markdown
Member

I agree with @refack that using AssertionErrors here is not required and I would actually like to get rid of all asserts in /lib.

I am also not convinced about the export of the constants.

jasnell commented Aug 31, 2017

Copy link
Copy Markdown
Member Author

Looking at it further, I don't think we should remove the asserts. In particular, if someone ends up passing something like new errors.Error(), the result is a really odd code. Just as we have CHECKs at the C/C++ layer, we should have some basic checks at the js layer to keep from footgunning ourselves.

I will drop the commit that exports the constants tho.

@refack ... let's look at the possibility of pulling the asserts separately.

jasnell force-pushed the improve-errors-internals branch from 7f85568 to 6208225 Compare August 31, 2017 17:29

BridgeAR commented Aug 31, 2017

Copy link
Copy Markdown
Member

Well, I did not mean to get rid of the checks but I think it is good enough if we use if (truthy) { fail }. But probably it is fine to just stick to the asserts as they are.

Another note - as I pointed out in #14350, this is not really a alternative to it as it tries to solve something different.

refack commented Aug 31, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Looking at it further, I don't think we should remove the asserts. In particular, if someone ends up passing something like new errors.Error(), the result is a really odd code. Just as we have CHECKs at the C/C++ layer, we should have some basic checks at the js layer to keep from footgunning ourselves.

IMHO assertions are valid when dealing with userland inputs. For an internal (and fairly simple) construct, coverage by unit tests should be sufficient.
Also there's the matter of disparity of those two cases (ERR_INVALID_ARG_TYPE and ERR_INVALID_ARRAY_LENGTH) VS the other 10s of error message formatters.

But anyway I'm removing my objection.

jasnell commented Sep 1, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/9918/

BridgeAR commented Sep 3, 2017

Copy link
Copy Markdown
Member

@jasnell do you mind changing the assert name to something else e.g. "internalAssert"? Otherwise I think it is a bit confusing that it is not the "real" assert module and no other functions are available.

refack commented Sep 4, 2017

Copy link
Copy Markdown
Contributor

@jasnell do you mind changing the assert name to something else e.g. "internalAssert"? Otherwise I think it is a bit confusing that it is not the "real" assert module and no other functions are available.

I just had the same though 🤔

jasnell force-pushed the improve-errors-internals branch from ec87cad to 2e894b3 Compare September 5, 2017 18:09

jasnell commented Sep 5, 2017

Copy link
Copy Markdown
Member Author

Updated with the function renamed and a test fixed.

jasnell commented Sep 5, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/9956/

jasnell added a commit that referenced this pull request Sep 12, 2017
errors: eliminate circular dependency on assert
6ff521b 
PR-URL: #15002
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

jasnell commented Sep 12, 2017

Copy link
Copy Markdown
Member Author

Landed in 6ff521b

jasnell closed this Sep 12, 2017
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
errors: eliminate circular dependency on assert
581be7a 
PR-URL: nodejs#15002
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

jasnell commented Sep 20, 2017

Copy link
Copy Markdown
Member Author

This will need to be backported for 8.x

jasnell added a commit to jasnell/node that referenced this pull request Sep 25, 2017
errors: eliminate circular dependency on assert
4df100f 
PR-URL: nodejs#15002
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
jasnell mentioned this pull request Sep 25, 2017
Closed
4 tasks
jasnell added a commit that referenced this pull request Sep 25, 2017
errors: eliminate circular dependency on assert
a14b252 
PR-URL: #15002
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes
lpinca lpinca approved these changes
BridgeAR BridgeAR approved these changes
+2 more reviewers
evanlucas evanlucas left review comments
refack refack approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

errors Issues and PRs related to JavaScript errors originated in Node.js core.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL