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

C++ errors and JS errors format mismatch #26669

Closed
mscdex opened this issue Mar 14, 2019 · 16 comments
Closed

C++ errors and JS errors format mismatch #26669

mscdex opened this issue Mar 14, 2019 · 16 comments
Assignees
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core.

Comments

@mscdex
Copy link
Contributor

mscdex commented Mar 14, 2019

  • Version: all
  • Platform: n/a
  • Subsystem: errors

The format of the name property differs between errors generated by C++ and those generated by JS.

For example, JS errors will contain a name like 'TypeError [ERR_FOO_BAR_BAZ]', while C++ errors will contain a name with just 'TypeError'.

It's probably a good idea to have these two match.

@mscdex mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Mar 14, 2019
@addaleax
Copy link
Member

I think there was some previous discussion (somewhere?) that it’s a bad idea to modify the .name property of errors like we do. I would agree, and if we do something, prefer to change it to the C++-style formatting.

@Trott
Copy link
Member

Trott commented Mar 14, 2019

Possibly a duplicate of #20253?

I'd be in favor of a breaking change that moves the codes out of the name property but keeps them in the display when the error is printed.

@BridgeAR
Copy link
Member

It indeed looks like a duplicate of #20253. I personally would also like to change it to only contain the class name.

One way that work relatively well is: #20253 (comment)

An alternative would be to move it in the message property as suggested in #20253 (comment)

@joyeecheung
Copy link
Member

joyeecheung commented Mar 14, 2019

I tried restoring the name property and moving the code out of them once, but there were too many tests failing because we explicitly test for the name property with this format (there are still 77 tests on the current master if you search for Error [ERR), and I did not really want to change them in bulk.

Maybe we should start migrating those tests? We already disable this format for WPT with internalErrors.useOriginalName = true, since WPT uses the name property instead of instanceof (because of realms) to verify the error types.

@BridgeAR
Copy link
Member

@joyeecheung I would be rigorous in this case and use regular expressions to replace these cases. It would otherwise indeed be a lot of work to change all these tests. I am not sure how you would like to migrate the tests without having to change all at once (if we want to keep on using assert.throws() and check the name property in these tests).

@joyeecheung
Copy link
Member

joyeecheung commented Mar 15, 2019

@BridgeAR Pretty sure for most cases (errors that are not thrown from a different vm context), we could just replace assert.throws options like

{ name: 'TypeError [ERR_SOMETHING]', code: 'ERR_SOMETHING' }

with

{ type: TypeError, code: 'ERR_SOMETHING' }

For errors thrown from another context, we could switch to regular expressions, but those are rare.

No matter how we change those tests, there are still going to be 70+ files to be changed so I don't see a huge difference.

@BridgeAR
Copy link
Member

@joyeecheung I don't think it is a good idea to use common.expectsError() at all anymore for these cases (and I originally changed the function to support this use case in the first place). I definitely think we should use assert.throws.

The only reason why I originally added this functionality was that assert.throws() at that point did not support anything the like but that is not the case anymore.

@joyeecheung
Copy link
Member

Right, type seems to be a common.expectsError option, an easy way to do this is to simply replace all assert.throws with common.expectsError, and in the implementation of common.expectsError, transform the options passed to assert.throws as mentioned above.
Then when we finish the migration and change the implementation of the name properties, we just need to go back to strict matching in common.expectsError and the test will be stricter.

@joyeecheung
Copy link
Member

@BridgeAR The issue with assert.throws is that it's a public API, but common.expectsError is not, so whenever we make a decision like this, if we continue using assert.throws, all the tests need to be migrated again before the implementation can change, whereas it would easier if we just use common.expectsError all the time and tweak it to our liking.

@BridgeAR
Copy link
Member

@joyeecheung this is the only special case and we'd run into the same issue with expectsError if we planned on changing anything else. I doubt that if we switch back to the having the "regular" name in the name property that we'd ever change that again.

@joyeecheung
Copy link
Member

joyeecheung commented Mar 15, 2019

@BridgeAR But we could extend common.expectsError with more features easily in the future if we decide to make any more changes to our errors: e.g. testing cross-realm errors, or actually providing names (e.g. NetworkError) for different categories of errors with a hierarchy. In those cases, it will be easier to change common.expectsError than to change assert.throws, and since common.expectsError already uses assert.throws, if we decide to surface those changes to assert.throws we just need to move the logic. IMO that's a healthier development flow than having to rely on assert.throws directly to make any changes for internal testing purposes.

@sam-github
Copy link
Contributor

I like the common. APIs. Every time I type assert.strict... I wish we didn't use assert at all in our unit tests, and just had an internal assertion package that did exactly what we want and that could be changed at our convenience, rather than having eslint rules that force us to use the long cumbersome names.

@BridgeAR
Copy link
Member

@sam-github you could use const assert = require('assert').strict;. All assert functions are going to behave strict afterwards (so you can use the short names). But this has never become a convention in core.

@sam-github
Copy link
Contributor

@BridgeAR Nice idea, I just tried, but it triggers our eslint error.

@BridgeAR
Copy link
Member

Yes, we should improve the rule to detect that.

@BridgeAR
Copy link
Member

I have a fix with which the errors will always print the same as they do right now but with which the name is set to the class name. I'll open a PR for it later on.

@BridgeAR BridgeAR self-assigned this Mar 17, 2019
@BridgeAR BridgeAR mentioned this issue Mar 18, 2019
4 tasks
BridgeAR added a commit to BridgeAR/node that referenced this issue Mar 23, 2019
When using `Errors.captureStackFrames` the error's stack property
is set again. This adds a helper function that wraps this functionality
in a simple API that does not only set the stack including the `code`
property but it also improves the performance to create the error.
The helper works for thrown errors and errors returned from wrapped
functions in case they are Node.js core errors.

PR-URL: nodejs#26738
Fixes: nodejs#26669
Fixes: nodejs#20253
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this issue Mar 23, 2019
This is a first step to align the n-api errors towards errors created
in JS. The stack still has to be updated to add the error code.

PR-URL: nodejs#26738
Fixes: nodejs#26669
Fixes: nodejs#20253
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Development

No branches or pull requests

6 participants