errors: add useOriginalName to internal/errors #22556

joyeecheung · 2018-08-27T20:49:15Z

This allows us to tell the type of the errors without using
instanceof, which is necessary in WPT harness.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2018-08-27T20:49:16Z

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/702/pipeline

joyeecheung · 2018-08-27T20:51:42Z

Also for context on why we use our internal error system for the URL errors: #11299

joyeecheung · 2018-08-27T20:55:18Z

There was also a discussion about restoring the behavior of error.name (couldn't find the link...cc @BridgeAR ?) I think we decided to keep them as-is. I haven't dug into the spec to see if we have to leave error.name alone, but I am guessing this is unspecified, so this patch is just a hack for the WPT harness.

joyeecheung · 2018-08-27T20:59:03Z

or...maybe we have to leave the error names alone, at least for errors thrown from Web APIs :/

Refs: https://heycam.github.io/webidl/#dfn-exception-error-name

For simple exceptions, the error name is the type of the exception.

jasnell · 2018-08-27T21:00:02Z

Not really a fan of this and really prefer to leave the internal/error names as they are currently. However, if this is just for testing, then definitely won't block

joyeecheung · 2018-08-27T21:03:47Z

Not really a fan of this and really prefer to leave the internal/error names as they are currently. However, if this is just for testing, then definitely won't block

@jasnell Yeah this is just a hack for testing with the WPT harness directly instead of using our mocks. I am not entirely sure if it's technically a bug in our implementation (which means we need to fix it for users), or just a quirk that we need to deal with if we want to use the WPT harness in our own tests though, but it doesn't harm to have a workaround before we figure that out

jdalton · 2018-08-28T05:50:17Z

lib/internal/errors.js

+  // These are exported only to facilitate testing.
+  E,
+  get useOriginalName() { return useOriginalName; },
+  set useOriginalName(value) { useOriginalName = value; }
 };


Instead of adding the useOriginalName couldn't you create a new error helper for more Web Platform related errors separate from the makeNodeErrorWithCode one?

@jdalton I would if we decide to leave the names alone for those errors...but doesn't seem to worth it if it's just to work around the WPT harness and it's only for testing

@joyeecheung Is it just a test harness thing are is the WPT fail hinting that URL errors should not have custom name's other than those of the builtin errors? If it means URL is not spec compliant then it's an issue for Node. If it is unspec'd then a bug should be filed for WPT.

See #11299 If you feel strongly about this feel free to reopen that issue or open a new one. If we were to fix that, it has to be semver-major. This PR is just a work around so that we can update the tests without a semver-major change - I would rather not label a bunch of upcoming test update with semver-major and make them a backport burden.

jdalton

Changing user accessible error APIs is not the answer here.

joyeecheung · 2018-08-28T17:01:42Z

Changing user accessible error APIs is not the answer here.

@jdalton Can you elaborate on your reasoning behind this? We have been exposing user-accessible APIs - albeit internals that are behind flags - to facilitate testing, like the errors.E that have already existed for quite some time, which is exported together with errors.useOriginalName. It seems out of scope to debate whether we should make internal testing utilities (behind flags) accessible to users here.

jdalton · 2018-08-28T17:06:35Z

This is a test harness concern and not worth moving into exposed APIs. I've given you a path forward that doesn't involving touching Node error APIs. Depending on where the fault is you can file a bug on WPT or work with Node to correct its errors. If it requires a major bump for Node then so be it. The WPT tests can be held until then.

BridgeAR · 2018-08-28T17:08:31Z

@joyeecheung the issue is #20253.

I only found a single way to consistently adding the error code to the stack trace while keeping the name as is: directly after creating the error (or on creation) the name is set to err.name + ' [ERROR_CODE]', then the stack is accessed, the error name is reset to the original name and the error is returned... this however is a significant performance issue in case the error stack would not be accessed (but accessing the stack is normal when receiving an error, so that should be rare) and the prepareStackTrace function could not be used on received errors (this is something the user can not always expect though as the stack might already have been accessed somewhere in-between).

Other than that, the current implementation is AFAIK the only one possible.

devsnek

this looks a-okay to me

BridgeAR

Since this is internal, I am fine with it for now. We can still revise the solution if we find something better.

So LGTM.

joyeecheung · 2018-08-28T17:32:33Z

This is a test harness concern and not worth moving into exposed APIs.

I would agree if this is a public API, but it is an internal API that is only accessible with the --expose-internals flag. As unfortunate as it is, altering internal APIs for test harness concerns is an established practice - searching for // Flags: --expose-internals under test yields 118 results on master.

EDIT: it appears I was on a outdated branch...the current number of Flags: --expose-internals under test is 156

jdalton

Oh I see. I misunderstood where the useOriginalName getter/setters lived.

refack · 2018-08-29T16:53:44Z

As unfortunate as it is, altering internal APIs for test harness concerns is an established practice - searching for // Flags: --expose-internals under test yields 118 results on master.

IMHO that's A Ok. At least for unit testing. Being able to "mock out" parts of the system is very important for predictable testing.

refack · 2018-08-29T16:56:40Z

~~P.S. I know I'm late to tha party, but what is the purpose of this change?~~
NM found it This allows us to tell the type of the errors without using instanceof, which is necessary in WPT harness.

refack · 2018-08-29T16:57:27Z

lib/internal/errors.js

@@ -437,7 +442,10 @@ module.exports = {
  getMessage,
  SystemError,
  codes,
-  E // This is exported only to facilitate testing.
+  // These are exported only to facilitate testing.


Could you add this This allows us to tell the type of the errors without using instanceof, which is necessary in WPT harness. to the comment.

test/parallel/test-internal-error-original-names.js

BridgeAR · 2018-09-05T13:55:15Z

Ping @joyeecheung

This allows us to tell the type of the errors without using instanceof, which is necessary in WPT harness.

joyeecheung · 2018-09-15T05:45:45Z

Addressed @refack 's comments.

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

joyeecheung · 2018-09-16T22:03:00Z

I am going to land this tomorrow if there are no more comments etc.

refack · 2018-09-16T22:30:31Z

Addressed @refack 's comments.

Thank you for following up.

joyeecheung · 2018-09-17T18:47:13Z

Landed in e692a09, thanks!

This allows us to tell the type of the errors without using instanceof, which is necessary in WPT harness. PR-URL: #22556 Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Aug 27, 2018

jdalton reviewed Aug 28, 2018

View reviewed changes

jdalton suggested changes Aug 28, 2018

View reviewed changes

devsnek approved these changes Aug 28, 2018

View reviewed changes

BridgeAR approved these changes Aug 28, 2018

View reviewed changes

jdalton approved these changes Aug 28, 2018

View reviewed changes

refack reviewed Aug 29, 2018

View reviewed changes

joyeecheung added 2 commits September 14, 2018 22:19

errors: add useOriginalName to internal/errors

dd18873

This allows us to tell the type of the errors without using instanceof, which is necessary in WPT harness.

fixup! errors: add useOriginalName to internal/errors

2742d39

joyeecheung force-pushed the original-name branch from a831e50 to 2742d39 Compare September 15, 2018 05:45

refack approved these changes Sep 16, 2018

View reviewed changes

joyeecheung closed this Sep 17, 2018

targos mentioned this pull request Sep 18, 2018

Release proposal: v10.11.0 #22932

Merged

joyeecheung mentioned this pull request Oct 3, 2018

Error.name should not contain display elements #20253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: add useOriginalName to internal/errors #22556

errors: add useOriginalName to internal/errors #22556

joyeecheung commented Aug 27, 2018

nodejs-github-bot commented Aug 27, 2018

joyeecheung commented Aug 27, 2018

joyeecheung commented Aug 27, 2018 •

edited

Loading

joyeecheung commented Aug 27, 2018 •

edited

Loading

jasnell commented Aug 27, 2018

joyeecheung commented Aug 27, 2018 •

edited

Loading

jdalton Aug 28, 2018

joyeecheung Aug 28, 2018

jdalton Aug 28, 2018 •

edited

Loading

joyeecheung Aug 28, 2018

jdalton left a comment

joyeecheung commented Aug 28, 2018 •

edited

Loading

jdalton commented Aug 28, 2018

BridgeAR commented Aug 28, 2018 •

edited

Loading

devsnek left a comment

BridgeAR left a comment

joyeecheung commented Aug 28, 2018 •

edited

Loading

jdalton left a comment

refack commented Aug 29, 2018

refack commented Aug 29, 2018

refack Aug 29, 2018

BridgeAR commented Sep 5, 2018

joyeecheung commented Sep 15, 2018

joyeecheung commented Sep 16, 2018

refack commented Sep 16, 2018

joyeecheung commented Sep 17, 2018

errors: add useOriginalName to internal/errors #22556

errors: add useOriginalName to internal/errors #22556

Conversation

joyeecheung commented Aug 27, 2018

Checklist

nodejs-github-bot commented Aug 27, 2018

joyeecheung commented Aug 27, 2018

joyeecheung commented Aug 27, 2018 • edited Loading

joyeecheung commented Aug 27, 2018 • edited Loading

jasnell commented Aug 27, 2018

joyeecheung commented Aug 27, 2018 • edited Loading

jdalton Aug 28, 2018

Choose a reason for hiding this comment

joyeecheung Aug 28, 2018

Choose a reason for hiding this comment

jdalton Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

joyeecheung Aug 28, 2018

Choose a reason for hiding this comment

jdalton left a comment

Choose a reason for hiding this comment

joyeecheung commented Aug 28, 2018 • edited Loading

jdalton commented Aug 28, 2018

BridgeAR commented Aug 28, 2018 • edited Loading

devsnek left a comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

joyeecheung commented Aug 28, 2018 • edited Loading

jdalton left a comment

Choose a reason for hiding this comment

refack commented Aug 29, 2018

refack commented Aug 29, 2018

refack Aug 29, 2018

Choose a reason for hiding this comment

BridgeAR commented Sep 5, 2018

joyeecheung commented Sep 15, 2018

joyeecheung commented Sep 16, 2018

refack commented Sep 16, 2018

joyeecheung commented Sep 17, 2018

joyeecheung commented Aug 27, 2018 •

edited

Loading

joyeecheung commented Aug 27, 2018 •

edited

Loading

joyeecheung commented Aug 27, 2018 •

edited

Loading

jdalton Aug 28, 2018 •

edited

Loading

joyeecheung commented Aug 28, 2018 •

edited

Loading

BridgeAR commented Aug 28, 2018 •

edited

Loading

joyeecheung commented Aug 28, 2018 •

edited

Loading