feat: Use framesToPop for InvaliantViolations in React errors#2204
feat: Use framesToPop for InvaliantViolations in React errors#2204kamilogorek merged 1 commit intomasterfrom
Conversation
kamilogorek
force-pushed
the
react-framestopop
branch
from
9c2f513 to
07d8341
Compare
packages/browser/src/tracekit.ts
Outdated
| } | ||
|
|
||
| function popFrames(stacktrace: any, popSize: number): any { | ||
| if (typeof popSize !== 'number' || popSize === 0) { |
There was a problem hiding this comment.
My personal habit is using isNaN to detect number in JavaScript since typeof NaN also returns number, feel free if it's over-defend :)
There was a problem hiding this comment.
Good catch. Will change that, thanks! :)
| }; | ||
| } | ||
|
|
||
| function popFrames(stacktrace: any, popSize: number): any { |
There was a problem hiding this comment.
Is there a reason not to use default value, then you could remove the number type check?!
| function popFrames(stacktrace: any, popSize: number): any { | |
| function popFrames(stacktrace: any, popSize: number = 0): any { |
There was a problem hiding this comment.
It's Tracekit, so we have to be very defensive in here. It's guarding us in case React breaks something, or someone decides to put framesToPop property on their error objects.
|
Ahh and changelog plz! |
kamilogorek
force-pushed
the
react-framestopop
branch
from
07d8341 to
dbdfa83
Compare
| } | ||
|
|
||
| function popFrames(stacktrace: any, popSize: number): any { | ||
| if (Number.isNaN(popSize)) { |
There was a problem hiding this comment.
Number.isNaN is totally different than isNaN, since Number.isNaN detects if it's NaN or not, isNaN detects if it is Not A Number, you should also check value 0 as your previous logic: if (isNaN(popSize) || popSize === 0)
Number.isNaN('sentry'); // false
isNaN('sentry'); // trueBTW, Number.isNaN seems not work so good on IE browsers, haven't checked yet. Would be nice if I can get some information abt it from yours XD
There was a problem hiding this comment.
@jiananshi I know how they work, thanks for pointing that out anyway :)
I can skip 0 logic, as slice(0) returns the same array, thus passing popSize === 0 will be effectively no-op.
Yes, Number.isNaN is not IE friendly, but we already use it in our codebase. It's mentioned here https://docs.sentry.io/platforms/javascript/#browser-table
There was a problem hiding this comment.
Fine, let's take a step back and compare with the previous code:
// previous
if (typeof popSize !== 'number' || popSize === 0)
// current
if (Number.isNaN(popSize))In the previous logic you do want a NUMBER but currently anythings besides NaN is acceptable, not sure if that's what you want.
btw the browser-table is a nice design, just learned that.
There was a problem hiding this comment.
Thankfully it will work just fine anyway, as slice can accept anything and it'll disregard anything other than a number. Thus this check is completely redundant anyway. Will remove it in the next patch release. Thanks and excuse my stubbornness 😅
There was a problem hiding this comment.
It's ok, as english is not my mother language previous comment may feel little bit annoying, if it is, pls don't mind.
There was a problem hiding this comment.
It's totally fine, my bad :)
There was a problem hiding this comment.
Will remove it in the next patch release.
Removed (will be commited soon) :)
Thanks again!
4 participants
Fixes #2202