errors: move error creation helpers to internal/errors.js #18546

joyeecheung · 2018-02-03T10:00:32Z

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.

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.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

errors, util

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.

joyeecheung · 2018-02-03T10:02:13Z

lib/internal/errors.js

+ * The goal is to migrate them to ERR_* errors later when compatibility is
+ * not a concern.
+ *
+ * @param {Object} ctx


I went with the JSDoc-style annotations here to make it clearer about what these helpers take and return

joyeecheung · 2018-02-03T10:07:32Z

lib/internal/errors.js

+
+  const ex = new Error(message);
+  // TODO(joyeecheung): errno is supposed to err, like in uvException
+  ex.code = ex.errno = code;


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.

joyeecheung · 2018-02-03T10:09:25Z

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

cc @jasnell

joyeecheung · 2018-02-07T03:23:13Z

CI before landing https://ci.nodejs.org/job/node-test-pull-request/12989/

joyeecheung · 2018-02-07T10:56:01Z

Landed in bff5d5b...c0762c2, thanks!

PR-URL: #18546 Reviewed-By: James M Snell <jasnell@gmail.com>

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 <jasnell@gmail.com>

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 <jasnell@gmail.com>

MylesBorins · 2018-02-21T00:26:25Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

PR-URL: nodejs#18546 Reviewed-By: James M Snell <jasnell@gmail.com>

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 <jasnell@gmail.com>

Backport-PR-URL: #18916 PR-URL: #18546 Reviewed-By: James M Snell <jasnell@gmail.com>

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 <jasnell@gmail.com>

Backport-PR-URL: #18916 PR-URL: #18546 Reviewed-By: James M Snell <jasnell@gmail.com>

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 <jasnell@gmail.com>

PR-URL: nodejs#18546 Reviewed-By: James M Snell <jasnell@gmail.com>

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 <jasnell@gmail.com>

PR-URL: nodejs#18546 Reviewed-By: James M Snell <jasnell@gmail.com>

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 <jasnell@gmail.com>

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 <jasnell@gmail.com>

Backport-PR-URL: #19191 PR-URL: #18546 Reviewed-By: James M Snell <jasnell@gmail.com>

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 <jasnell@gmail.com>

Backport-PR-URL: #19191 PR-URL: #18546 Reviewed-By: James M Snell <jasnell@gmail.com>

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 <jasnell@gmail.com>

Backport-PR-URL: #19191 PR-URL: #18546 Reviewed-By: James M Snell <jasnell@gmail.com>

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 <jasnell@gmail.com>

joyeecheung added 3 commits February 3, 2018 17:32

util: skip type checks in internal getSystemErrorName

d3671ae

fs: do not call new when creating uvException

7cb2e97

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.

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 3, 2018

joyeecheung added errors Issues and PRs related to JavaScript errors originated in Node.js core. util Issues and PRs related to the built-in util module. labels Feb 3, 2018

joyeecheung commented Feb 3, 2018

View reviewed changes

jasnell approved these changes Feb 5, 2018

View reviewed changes

joyeecheung closed this Feb 7, 2018

joyeecheung added a commit that referenced this pull request Feb 7, 2018

util: skip type checks in internal getSystemErrorName

12ae334

PR-URL: #18546 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added the backport-requested-v9.x label Feb 21, 2018

joyeecheung mentioned this pull request Feb 21, 2018

[v9.x] backport 17338, 18186, 18358, 18546 #18916

Closed

joyeecheung added a commit to joyeecheung/node that referenced this pull request Feb 22, 2018

util: skip type checks in internal getSystemErrorName

8006e0b

PR-URL: nodejs#18546 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 26, 2018

util: skip type checks in internal getSystemErrorName

f1f180d

Backport-PR-URL: #18916 PR-URL: #18546 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 26, 2018

util: skip type checks in internal getSystemErrorName

8e31bf4

Backport-PR-URL: #18916 PR-URL: #18546 Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax mentioned this pull request Feb 27, 2018

v9.7.0 proposal #19040

Closed

joyeecheung added backported-to-v9.x and removed backport-requested-v9.x labels Mar 6, 2018

joyeecheung mentioned this pull request Mar 7, 2018

[v8.x backport] backport 17338, 18186, 18358, 18546 #19191

Closed

joyeecheung added a commit to joyeecheung/node that referenced this pull request May 2, 2018

util: skip type checks in internal getSystemErrorName

2179547

PR-URL: nodejs#18546 Reviewed-By: James M Snell <jasnell@gmail.com>

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

util: skip type checks in internal getSystemErrorName

94f0f11

PR-URL: nodejs#18546 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request May 22, 2018

util: skip type checks in internal getSystemErrorName

de86203

Backport-PR-URL: #19191 PR-URL: #18546 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Jun 14, 2018

util: skip type checks in internal getSystemErrorName

ae81a04

Backport-PR-URL: #19191 PR-URL: #18546 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

MylesBorins added the backported-to-v8.x label Aug 7, 2018

rvagg pushed a commit that referenced this pull request Aug 16, 2018

util: skip type checks in internal getSystemErrorName

66520af

Backport-PR-URL: #19191 PR-URL: #18546 Reviewed-By: James M Snell <jasnell@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: move error creation helpers to internal/errors.js #18546

errors: move error creation helpers to internal/errors.js #18546

joyeecheung commented Feb 3, 2018

joyeecheung Feb 3, 2018 •

edited

Loading

joyeecheung Feb 3, 2018 •

edited

Loading

joyeecheung commented Feb 3, 2018

joyeecheung commented Feb 7, 2018

joyeecheung commented Feb 7, 2018

MylesBorins commented Feb 21, 2018

errors: move error creation helpers to internal/errors.js #18546

errors: move error creation helpers to internal/errors.js #18546

Conversation

joyeecheung commented Feb 3, 2018

Checklist

Affected core subsystem(s)

joyeecheung Feb 3, 2018 • edited Loading

Choose a reason for hiding this comment

joyeecheung Feb 3, 2018 • edited Loading

Choose a reason for hiding this comment

joyeecheung commented Feb 3, 2018

joyeecheung commented Feb 7, 2018

joyeecheung commented Feb 7, 2018

MylesBorins commented Feb 21, 2018

joyeecheung Feb 3, 2018 •

edited

Loading

joyeecheung Feb 3, 2018 •

edited

Loading