adjust checking for internal and public errors

[CatchMe2]

Changes

Please describe

Checklist

• Apply one of following labels; major, minor, patch or skip-release
• I've updated the documentation, or no changes were necessary
• I've updated the tests, or no changes were necessary

[kibertoad]

on a second thought, symbols might not be the best fit here. will write a comment to explain

[kibertoad]

Problem with using a symbol here is that it might make the implementation more fragile than intended.

Imagine that node-core 11 throws an error somewhere deep in your dependencies, and your app is using node-core 12. those will be two separate imports, and each will have their own Symbol instance, so errors from one node-core version will no longer be recognized as errors from another node-core version.

plain string field would work better here. it will have worse encapsulation, but it's unlikely to cause any real issues

[CatchMe2]

I don't feel like having a plain text field would change anything about backward compatibility. This new field would be only available in node-code v12 so the same problem as for symbol.

The only difference between the Symbol and plain text is that Symbol won't be serialized in the case of JSON.stringify(err) or it won't be iterable with Object.entries(err) so that it will behave the same as node-core v11.

I think that instead, we should adjust how we check if the error is internal or not so we might still support the old way of checking for it like

 return isError(error) && (error[Symbol.for(INTERNAL_ERROR_SYMBOL_KEY)] === true || error.name === 'InternalError')

[kibertoad]

I've done a naive test, and within same realm, indeed, Symbol.for is using a global registry, so we receive same symbol for the same string, and everything works as expected.

Tomorrow I'll setup a test with separate realms and see if it still doesn't break.

[CatchMe2]

I've already tested it:

import { createContext, runInContext } from 'node:vm';
import { isNativeError } from 'node:util/types'

const errContext = {};
createContext(errContext);

const errResult = runInContext(`new Error('Unknown')`, errContext);

console.log(errResult instanceof Error) // false
console.log(isNativeError(errResult)) // true


const symbolContext = {};
createContext(symbolContext);

const symbolResult = runInContext(`Symbol.for('ANY')`, errContext);

console.log(symbolResult === Symbol.for('ANY')) // true

[kibertoad]

reproduction for the problem: lokalise/node-service-template#761

[kibertoad]

will test more later, but adjusting this later should be a pretty minor non-breaking change