Node: C++ errors and JS errors format mismatch

[mscdex]

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

[addaleax]

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]

Possibly a duplicate of https://github.com/nodejs/node/issues/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]

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: https://github.com/nodejs/node/issues/20253#issuecomment-426568019

An alternative would be to move it in the message property as suggested in https://github.com/nodejs/node/issues/20253#issuecomment-426574237

[joyeecheung]

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]

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

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

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

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]

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

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

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

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]

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

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

[BridgeAR]

Yes, we should improve the rule to detect that.

[BridgeAR]

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.