ref(angular) Add error-like objects handling #6446
Conversation
packages/angular/src/errorhandler.ts
Outdated
|
|
||
| function isErrorOrErrorLikeObject(value: unknown): value is Error { | ||
| return ( | ||
| value instanceof Error || (isObject(value) && hasErrorName(value) && hasErrorMessage(value) && hasErrorStack(value)) |
There was a problem hiding this comment.
of course this could all be inlined, but I find a lot more readable that way
There was a problem hiding this comment.
It's very readable but we also have to consider the bundle size impact here. Hence my question if hasOwnProperty is strictly necessary.
Maybe, we could also inline the isObject check?
There was a problem hiding this comment.
I'm not very familiar with how the code impacts the bundle size, is there a way I could check the result so that I can check different things to see if there is still a way to keep it readable without increasing the size too much?
Closes getsentry#6332. As discussed in the linked issue, it is more beneficial to handle error shaped objects instead of just instances of `Error`s.
theofidry
force-pushed
the
feature/bugfix-error-like-objects
branch
from
4853d65 to
6c7d87f
Compare
packages/angular/src/errorhandler.ts
Outdated
| } | ||
|
|
||
| function hasErrorName(value: Record<string, unknown>): value is Pick<Error, 'name'> { | ||
| return hasOwnProperty(value, 'name') && typeof value.name === 'string'; |
There was a problem hiding this comment.
Not saying it's wrong but why do we need the hasOwnProperty check here? Wouldn't it be enough to check for typeof value.name === 'string'?
Or actually, for the string check, we could simply reuse our isString check from @sentry/utils.
packages/angular/src/errorhandler.ts
Outdated
|
|
||
| function isErrorOrErrorLikeObject(value: unknown): value is Error { | ||
| return ( | ||
| value instanceof Error || (isObject(value) && hasErrorName(value) && hasErrorMessage(value) && hasErrorStack(value)) |
There was a problem hiding this comment.
It's very readable but we also have to consider the bundle size impact here. Hence my question if hasOwnProperty is strictly necessary.
Maybe, we could also inline the isObject check?
|
@Lms24 here's a simplified version. I can't say I'm happy, and I feel the code could be simpler, but that's a case where TS is getting a bit in the way; I can't just check |
There was a problem hiding this comment.
Thanks for addressing my feedback! I see the TS problem and I think we just run with your solution for now.
This looks good to me now. Thanks so much for contributing!
Just a heads-up: We're not gonna make another release until after the holidays so I'll hold of from merging this PR until then.
|
👍 |
Closes #6332.
As discussed in the linked issue, it is more beneficial to handle error shaped objects instead of just instances of
Errors.