errors: add useOriginalName to internal/errors by joyeecheung · Pull Request #22556 · nodejs/node · GitHub

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

@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

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

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.

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.

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

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

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.

Changing user accessible error APIs is not the answer here.

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.

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.

@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.

this looks a-okay to me

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

So LGTM.

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

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

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.

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.

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.

Ping @joyeecheung

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

Addressed @refack 's comments.

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

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

Addressed @refack 's comments.

Thank you for following up.

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 <john.david.dalton@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com>

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

errors: add useOriginalName to internal/errors by joyeecheung · Pull Request #22556 · nodejs/node · GitHub

Uh oh!

Uh oh!

errors: add useOriginalName to internal/errors - #22556

errors: add useOriginalName to internal/errors#22556joyeecheung wants to merge 2 commits intonodejs:masternodejs/node:masterfrom joyeecheung:original-namejoyeecheung/node:original-nameCopy head branch name to clipboard

Conversation

joyeecheung commented Aug 27, 2018

Checklist

Uh oh!

nodejs-github-bot commented Aug 27, 2018

Uh oh!

joyeecheung commented Aug 27, 2018

Uh oh!

joyeecheung commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Aug 27, 2018

Uh oh!

joyeecheung commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdalton Aug 28, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung Aug 28, 2018

Choose a reason for hiding this comment

Uh oh!

jdalton Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Aug 28, 2018

Choose a reason for hiding this comment

Uh oh!

jdalton left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdalton commented Aug 28, 2018

Uh oh!

BridgeAR commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devsnek left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdalton left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 29, 2018

Uh oh!

refack commented Aug 29, 2018

Uh oh!

refack Aug 29, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BridgeAR commented Sep 5, 2018

Uh oh!

joyeecheung commented Sep 15, 2018

Uh oh!

joyeecheung commented Sep 16, 2018

Uh oh!

refack commented Sep 16, 2018

Uh oh!

joyeecheung commented Sep 17, 2018

Uh oh!

Reviewers

Assignees

errors: add useOriginalName to internal/errors#22556
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:original-name

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