errors: move error creation helpers to internal/errors.js#18546
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
We cannot make uvException a proper class due to compatibility reasons for now, so there is no need to call new since it only returns a newly-created Error.
This commit moves error creation helpers scattered around under lib/ into lib/internal/errors.js in the hope of being clearer about the differences of errors that we throw into the user land. - Move util._errnoException and util._exceptionWithHostPort into internal/errors.js and simplify their logic so it's clearer what the properties these helpers create. - Move the errnoException helper in dns.js to internal/errors.js into internal/errors.js and rename it to dnsException. Simplify it's logic so it no longer calls errnoException and skips the unnecessary argument checks.
nodejs-github-bot added the lib / src 
joyeecheung added errors 
joyeecheung commented Feb 3, 2018
| * The goal is to migrate them to ERR_* errors later when compatibility is | ||
| * not a concern. | ||
| * | ||
| * @param{Object} ctx |
MemberAuthor
There was a problem hiding this comment.
I went with the JSDoc-style annotations here to make it clearer about what these helpers take and return
joyeecheung commented Feb 3, 2018
| constex=newError(message); | ||
| // TODO(joyeecheung): errno is supposed to err, like in uvException | ||
| ex.code=ex.errno=code; |
MemberAuthor
There was a problem hiding this comment.
In general, errors thrown by net and dns present this weird behavior (both err.code and err.errno are the string name of the error), while errors thrown by fs have a numeric err.errno and a string err.code, which is more correct IMO (otherwise why bother duplicating the info in another property? Also the name errno means it's supposed to be a number) We might want to revisit this and see how big the breakage would be if we make err.errno numeric.
MemberAuthor
joyeecheung commented Feb 3, 2018
jasnell approved these changes Feb 5, 2018
MemberAuthor
joyeecheung commented Feb 7, 2018
CI before landing https://ci.nodejs.org/job/node-test-pull-request/12989/ |
MemberAuthor
joyeecheung commented Feb 7, 2018
Landed in bff5d5b...c0762c2, thanks! |
joyeecheung added a commit that referenced this pull request Feb 7, 2018
PR-URL: #18546 Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this pull request Feb 7, 2018
We cannot make uvException a proper class due to compatibility reasons for now, so there is no need to call new since it only returns a newly-created Error. PR-URL: #18546 Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this pull request Feb 7, 2018
This commit moves error creation helpers scattered around under lib/ into lib/internal/errors.js in the hope of being clearer about the differences of errors that we throw into the user land. - Move util._errnoException and util._exceptionWithHostPort into internal/errors.js and simplify their logic so it's clearer what the properties these helpers create. - Move the errnoException helper in dns.js to internal/errors.js into internal/errors.js and rename it to dnsException. Simplify it's logic so it no longer calls errnoException and skips the unnecessary argument checks. PR-URL: #18546 Reviewed-By: James M Snell <[email protected]>
Contributor
MylesBorins commented Feb 21, 2018
Should this be backported to |
joyeecheung added a commit to joyeecheung/node that referenced this pull request Feb 22, 2018
PR-URL: nodejs#18546 Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Feb 22, 2018
This commit moves error creation helpers scattered around under lib/ into lib/internal/errors.js in the hope of being clearer about the differences of errors that we throw into the user land. - Move util._errnoException and util._exceptionWithHostPort into internal/errors.js and simplify their logic so it's clearer what the properties these helpers create. - Move the errnoException helper in dns.js to internal/errors.js into internal/errors.js and rename it to dnsException. Simplify it's logic so it no longer calls errnoException and skips the unnecessary argument checks. PR-URL: nodejs#18546 Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
Backport-PR-URL: #18916 PR-URL: #18546 Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
This commit moves error creation helpers scattered around under lib/ into lib/internal/errors.js in the hope of being clearer about the differences of errors that we throw into the user land. - Move util._errnoException and util._exceptionWithHostPort into internal/errors.js and simplify their logic so it's clearer what the properties these helpers create. - Move the errnoException helper in dns.js to internal/errors.js into internal/errors.js and rename it to dnsException. Simplify it's logic so it no longer calls errnoException and skips the unnecessary argument checks. Backport-PR-URL: #18916 PR-URL: #18546 Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
Backport-PR-URL: #18916 PR-URL: #18546 Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
This commit moves error creation helpers scattered around under lib/ into lib/internal/errors.js in the hope of being clearer about the differences of errors that we throw into the user land. - Move util._errnoException and util._exceptionWithHostPort into internal/errors.js and simplify their logic so it's clearer what the properties these helpers create. - Move the errnoException helper in dns.js to internal/errors.js into internal/errors.js and rename it to dnsException. Simplify it's logic so it no longer calls errnoException and skips the unnecessary argument checks. Backport-PR-URL: #18916 PR-URL: #18546 Reviewed-By: James M Snell <[email protected]>
Closed
joyeecheung added a commit to joyeecheung/node that referenced this pull request May 2, 2018
PR-URL: nodejs#18546 Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this pull request May 2, 2018
This commit moves error creation helpers scattered around under lib/ into lib/internal/errors.js in the hope of being clearer about the differences of errors that we throw into the user land. - Move util._errnoException and util._exceptionWithHostPort into internal/errors.js and simplify their logic so it's clearer what the properties these helpers create. - Move the errnoException helper in dns.js to internal/errors.js into internal/errors.js and rename it to dnsException. Simplify it's logic so it no longer calls errnoException and skips the unnecessary argument checks. PR-URL: nodejs#18546 Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#18546 Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
We cannot make uvException a proper class due to compatibility reasons for now, so there is no need to call new since it only returns a newly-created Error. PR-URL: nodejs#18546 Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
This commit moves error creation helpers scattered around under lib/ into lib/internal/errors.js in the hope of being clearer about the differences of errors that we throw into the user land. - Move util._errnoException and util._exceptionWithHostPort into internal/errors.js and simplify their logic so it's clearer what the properties these helpers create. - Move the errnoException helper in dns.js to internal/errors.js into internal/errors.js and rename it to dnsException. Simplify it's logic so it no longer calls errnoException and skips the unnecessary argument checks. PR-URL: nodejs#18546 Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
Backport-PR-URL: #19191 PR-URL: #18546 Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
This commit moves error creation helpers scattered around under lib/ into lib/internal/errors.js in the hope of being clearer about the differences of errors that we throw into the user land. - Move util._errnoException and util._exceptionWithHostPort into internal/errors.js and simplify their logic so it's clearer what the properties these helpers create. - Move the errnoException helper in dns.js to internal/errors.js into internal/errors.js and rename it to dnsException. Simplify it's logic so it no longer calls errnoException and skips the unnecessary argument checks. Backport-PR-URL: #19191 PR-URL: #18546 Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
Backport-PR-URL: #19191 PR-URL: #18546 Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
This commit moves error creation helpers scattered around under lib/ into lib/internal/errors.js in the hope of being clearer about the differences of errors that we throw into the user land. - Move util._errnoException and util._exceptionWithHostPort into internal/errors.js and simplify their logic so it's clearer what the properties these helpers create. - Move the errnoException helper in dns.js to internal/errors.js into internal/errors.js and rename it to dnsException. Simplify it's logic so it no longer calls errnoException and skips the unnecessary argument checks. Backport-PR-URL: #19191 PR-URL: #18546 Reviewed-By: James M Snell <[email protected]>
Merged
rvagg pushed a commit that referenced this pull request Aug 16, 2018
Backport-PR-URL: #19191 PR-URL: #18546 Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
This commit moves error creation helpers scattered around under lib/ into lib/internal/errors.js in the hope of being clearer about the differences of errors that we throw into the user land. - Move util._errnoException and util._exceptionWithHostPort into internal/errors.js and simplify their logic so it's clearer what the properties these helpers create. - Move the errnoException helper in dns.js to internal/errors.js into internal/errors.js and rename it to dnsException. Simplify it's logic so it no longer calls errnoException and skips the unnecessary argument checks. Backport-PR-URL: #19191 PR-URL: #18546 Reviewed-By: James M Snell <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
util: skip type checks in internal getSystemErrorName
fs: do not call new when creating uvException
We cannot make uvException a proper class due to compatibility
reasons for now, so there is no need to call new since
it only returns a newly-created Error.
errors: move error creation helpers to errors.js
This commit moves error creation helpers scattered around
under lib/ into lib/internal/errors.js in the hope of being clearer
about the differences of errors that we throw into the user land.
into internal/errors.js and simplify their logic so it's
clearer what the properties these helpers create.
into internal/errors.js and rename it to dnsException. Simplify
it's logic so it no longer calls errnoException and skips
the unnecessary argument checks.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
errors, util