fix(node/browser): Always use constructor to get error type

[lobsterkatie]

For built-in subclasses of Error, JavaScript adjusts the name property to match the name of the class, but it does not do the same for user-defined Error subclasses. This therefore switches to always using constructor.name, as it works for both built-in and custom subclasses of Error.

Note: Our tracekit tests rely on fake error objects, rather than real instances of Error or a subclass, in order to be able to control the stacktrace value. In order to get those tests to pass, I added a helper to convert the mock errors into ones which also have the correct constructor.name property. I also had to hack react error boundary error creation just a tiny bit. LMK if you think there's a better way to do that.

[github-actions]

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	21.04 KB (+0.11% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)	65.69 KB (+0.11% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	19.59 KB (+0.12% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)	58.17 KB (+0.14% 🔺)
@sentry/browser - Webpack (gzipped + minified)	21.21 KB (+0.19% 🔺)
@sentry/browser - Webpack (minified)	69.08 KB (+0.06% 🔺)
@sentry/react - Webpack (gzipped + minified)	21.23 KB (+0.19% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified)	49.16 KB (+0.09% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	28.67 KB (+0.09% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	26.9 KB (+0.09% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified)	48.24 KB (+1.32% 🔺)
@sentry/replay - Webpack (gzipped + minified)	42.1 KB (+1.53% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	67.21 KB (+1.07% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	60.11 KB (+1.23% 🔺)

[Lms24]

I guess the tradeoff here is that if we go through with that change, manually set error.name properties wouldn't be respected anymore?

class CustomError extends Error {
    constructor(msg) {
        super(msg);
        this.name = 'MyCustomError';
    }
}

or fwiw

const err = new Error();
err.name = 'whatever'

I'm not sure yet if this is an improvement tbh

[lobsterkatie]

I'm a little confused by this example:

class CustomError extends Error {
    constructor(msg) {
        super(msg);
        this.name = 'MyCustomError';
    }
}

If you name your class CustomError, why would you want the name to be MyCustomError? Or, put another way, if you want the name to be MyCustomError, why wouldn't you just give that name to the class?

That said, I just played around a bit, and I think there's another way to solve this problem. Using the same example as above:

(The first class and globalThis assignment is what the SDK would do. The second class is what the user would do, when defining a custom error type.)

If we do it that way, we can keep pulling the error type from name when building the event. WDYT?

[Lms24]

If you name your class CustomError, why would you want the name to be MyCustomError? Or, put another way, if you want the name to be MyCustomError, why wouldn't you just give that name to the class?

Good questions - I wouldn't do this myself 😅 It's just something that's possible so I guess we have to find a way around it.

I tried your alternative suggestion in the browser console and got the following:

Seems like your example worked because of TypeError but if you directly extend from Error it'll use the patched classname

[Lms24]

Oh nevermind, we can get around this:

[lobsterkatie]

Seems like your example worked because of TypeError but if you directly extend from Error it'll use the patched classname

Sorry, I should have taken the TypeError out. It was there in the original example (in the PR description) to show that using the constructor name didn't break built-in subclasses. But using this new approach means we'd still be using name rather than constructor.name, so the TypeError example becomes superfluous.

Oh nevermind, we can get around this

class Error extends globalThis.Error

Clever!

All right, lemme give this a go in another branch.

P.S. I stuck this in a replit so I wouldn't have to retype it. Sorry, should have shared that from the get-go: https://replit.com/@lobsterkatie/RewardingSlowVideogame#index.js

[lforst]

Hey, quick heads up here: Cramer discovered that when doing this.name = this.constructor.name for errors, errors will appear minified in Sentry if the code is minified. I asked Kamil if there is a way for us to reliably unminify the errors in the backend but sadly we can't :/

Example: https://cramer.sentry.io/issues/4203128674/events/5f03c3b3b8604d7a8ef611ef5c6a701a/

Given the above, should we still go forward with the PR?

[lobsterkatie]

Hey, quick heads up here: Cramer discovered that when doing this.name = this.constructor.name for errors, errors will appear minified in Sentry if the code is minified.

Huh. Interesting. I suppose that's perhaps because the minifier happily mangles the names of classes but won't minify certain reserved tokens, inlcuding Error, TypeErrror, and the like? And then on our end, we don't apply deminification to the error type? If so, I'd buy that theory - it does make a certain amount of sense. And I'll admit I'm trying but failing to think of a workaround which doesn't require action on the user's part (in which case we should just update the troubleshooting docs).

I'm happy to close this for now. If I wake up in the middle of the night with a brilliant revelation, I can always re-open it.

[lforst]

If I wake up in the middle of the night with a brilliant revelation, I can always re-open it.

That's the way :D