Handle the case when a user throws somethign that isnt Error #191
Conversation
Codecov Report
@@ Coverage Diff @@
## master #191 +/- ##
==========================================
- Coverage 67% 66.25% -0.75%
==========================================
Files 79 79
Lines 1767 1787 +20
Branches 422 424 +2
==========================================
Hits 1184 1184
- Misses 455 473 +18
- Partials 128 130 +2
Continue to review full report at Codecov.
|
sdk-js/src/lib/transaction.js
Outdated
| this.set('error.exception.type', errorStack.exception.type); | ||
| this.set('error.exception.message', errorStack.exception.message); | ||
| this.set('error.exception.stacktrace', JSON.stringify(errorStack.exception.stacktrace)); | ||
| if (error instanceof Error) { |
There was a problem hiding this comment.
More a comment - it's not as bulletproof check as it could be:
instanceofis configurable in JS since ES2015+ (it can be overriden so it returns true for non error instances)- Native errors coming from other realms (e.g. if someone sneaks error from other VM context) will be rejected by
instanceofcheck.
The most solid check I know is: https://github.com/medikoo/type/blob/62c2e32a3568e12d93152d33c7cc882239d57f53/error/is.js#L9-L23
Still, I leave it just as a comment, as cases where above will produce a false positive (or positive false) I assume are highly unlikely in AWS Lambda context
There was a problem hiding this comment.
instanceof is configurable in JS since ES2015+ (it can be overriden so it returns true for non error instances)
IMO, if some one goes messing with that, they should know it can break just about anything.
Native errors coming from other realms (e.g. if someone sneaks error from other VM context) will be rejected by instanceof check.
is stackman able to process these native errors too?
There was a problem hiding this comment.
IMO, if some one goes messing with that, they should know it can break just about anything.
I agree that we shouldn't take into account cases when someone hacks the environment (by e.g. overriding some native methods or constructors).
But Symbol.hasInstance is a valid ES feature, and imo if something will fail on our side just because someone used it (of course that's extremely unlikely) - it's a bug on our side, no excuses :)
is stackman able to process these native errors too?
As I see it will (they do not seem to do any instanceof checks on input errors)
There was a problem hiding this comment.
is stackman able to process these native errors too?
As I see it will (they do not seem to do any instanceof checks on input errors)
That doesn't mean it'll work. It doesn't check for string either, but crashes if you pass it one.
There was a problem hiding this comment.
That doesn't mean it'll work
It will, as there's no difference on native error coming from different realm (it's just that it derives from other instance of Error.prototype), otherwise all internal slots (or even V8's specific Error.prepareStackTrace) are same among realms - It's why e.g. Array.isArray(arr) will return true for array coming from any realm but arr instanceof Array will not.
There was a problem hiding this comment.
ah. gotcha 👍
how would you recommend I use the code linked? should I just pull in your library? it seems like a lot to detangle to extract the same functionality.
There was a problem hiding this comment.
should I just pull in your library?
IMO it'll be a good move, it also has some other valuable type checking goodies, and it's very light (has no deps)
No description provided.