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

util: improve internal `isError()` validation #24746

Closed
wants to merge 5 commits into from

Conversation

@BridgeAR
Copy link
Member

commented Nov 30, 2018

The current internal isError function checked the toString value
instead of using the more precise util.types.isNativeError() check.
The instanceof check is not removed due to possible errors that
are not native but still an instance of Error.

The internal isError function is only used in util.inspect() and the repl.

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
util: improve internal `isError()` validation
The current internal isError function checked the toString value
instead of using the more precise `util.types.isNativeError()` check.
The `instanceof` check is not removed due to possible errors that
are not native but still an instance of Error.
@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

@sam-github
Copy link
Member

left a comment

I think this should have some unit tests, specifically, some that failed before the change and succeed afterwards. My js isn't good enough, I don't understand the subtleties of how it is becoming more precise, and was hoping to learn what corner cases are better covered from the tests.

lib/util.js Show resolved Hide resolved
@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

I just pushed some tests as well.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Dec 2, 2018

@targos
targos approved these changes Dec 2, 2018
BridgeAR added 2 commits Dec 2, 2018
@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Dec 2, 2018

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 3, 2018
util: improve internal `isError()` validation
The current internal isError function checked the toString value
instead of using the more precise `util.types.isNativeError()` check.
The `instanceof` check is not removed due to possible errors that
are not native but still an instance of Error.

PR-URL: nodejs#24746
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2018

Landed in 2b5f2bc

@BridgeAR BridgeAR closed this Dec 3, 2018

BridgeAR added a commit that referenced this pull request Dec 5, 2018
util: improve internal `isError()` validation
The current internal isError function checked the toString value
instead of using the more precise `util.types.isNativeError()` check.
The `instanceof` check is not removed due to possible errors that
are not native but still an instance of Error.

PR-URL: #24746
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
BridgeAR added a commit that referenced this pull request Dec 5, 2018
util: improve internal `isError()` validation
The current internal isError function checked the toString value
instead of using the more precise `util.types.isNativeError()` check.
The `instanceof` check is not removed due to possible errors that
are not native but still an instance of Error.

PR-URL: #24746
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@BridgeAR BridgeAR referenced this pull request Dec 5, 2018
4 of 4 tasks complete
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
util: improve internal `isError()` validation
The current internal isError function checked the toString value
instead of using the more precise `util.types.isNativeError()` check.
The `instanceof` check is not removed due to possible errors that
are not native but still an instance of Error.

PR-URL: nodejs#24746
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.