Skip to content

feat: Use framesToPop for InvaliantViolations in React errors #2204

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kamilogorek merged 1 commit into from
Aug 19, 2019
Merged

feat: Use framesToPop for InvaliantViolations in React errors #2204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog

## Unreleased

- [browser] feat: Use framesToPop for InvaliantViolations in React errors (#2204)

## 5.6.1

- [core] fix: Correctly detect when client is enabled before installing integrations (#2193)
Expand Down
24 changes: 20 additions & 4 deletions packages/browser/src/tracekit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,7 @@ TraceKit._computeStackTrace = (function _computeStackTraceWrapper() {
*/
function computeStackTrace(ex: any, depth: any) {
var stack = null;
var popSize = ex && ex.framesToPop;
depth = depth == null ? 0 : +depth;

try {
Expand All @@ -878,28 +879,28 @@ TraceKit._computeStackTrace = (function _computeStackTraceWrapper() {
// property first!!
stack = _computeStackTraceFromStacktraceProp(ex);
if (stack) {
return stack;
return popFrames(stack, popSize);
}
} catch (e) {}

try {
stack = _computeStackTraceFromStackProp(ex);
if (stack) {
return stack;
return popFrames(stack, popSize);
}
} catch (e) {}

try {
stack = _computeStackTraceFromOperaMultiLineMessage(ex);
if (stack) {
return stack;
return popFrames(stack, popSize);
}
} catch (e) {}

try {
stack = _computeStackTraceByWalkingCallerChain(ex, depth + 1);
if (stack) {
return stack;
return popFrames(stack, popSize);
}
} catch (e) {}

Expand All @@ -911,6 +912,21 @@ TraceKit._computeStackTrace = (function _computeStackTraceWrapper() {
};
}

function popFrames(stacktrace: any, popSize: number): any {

@HazAT HazAT Aug 19, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason not to use default value, then you could remove the number type check?!

Suggested change
function popFrames(stacktrace: any, popSize: number): any {
function popFrames(stacktrace: any, popSize: number = 0): any {

@kamilogorek kamilogorek Aug 19, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (Number.isNaN(popSize)) {

@jiananshi jiananshi Aug 20, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'); // true

BTW, 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

@kamilogorek kamilogorek Aug 20, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

@jiananshi jiananshi Aug 20, 2019 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kamilogorek kamilogorek Aug 20, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jiananshi

image

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 😅

@jiananshi jiananshi Aug 21, 2019 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok, as english is not my mother language previous comment may feel little bit annoying, if it is, pls don't mind.

@kamilogorek kamilogorek Aug 21, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's totally fine, my bad :)

@kamilogorek kamilogorek Sep 16, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove it in the next patch release.

Removed (will be commited soon) :)
Thanks again!

return stacktrace;
}

try {
return {
...stacktrace,
stack: stacktrace.stack.slice(popSize),
};
} catch (e) {
return stacktrace;
}
}

(computeStackTrace as any)._augmentStackTraceWithInitialElement = _augmentStackTraceWithInitialElement;
(computeStackTrace as any)._computeStackTraceFromStackProp = _computeStackTraceFromStackProp;

Expand Down
53 changes: 53 additions & 0 deletions packages/browser/test/unit/tracekit/custom.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,4 +284,57 @@ describe('Tracekit - Custom Tests', () => {
]);
});
});

describe('React', () => {
it('should correctly parse Invariant Violation errors and use framesToPop to drop info message', () => {
const REACT_INVARIANT_VIOLATION_EXCEPTION = {
framesToPop: 1,
message:
'Minified React error #31; visit https://reactjs.org/docs/error-decoder.html?invariant=31&args[]=object%20with%20keys%20%7B%7D&args[]= for the full message or use the non-minified dev environment for full errors and additional helpful warnings. ',
name: 'Invariant Violation',
stack: `Invariant Violation: Minified React error #31; visit https://reactjs.org/docs/error-decoder.html?invariant=31&args[]=object%20with%20keys%20%7B%7D&args[]= for the full message or use the non-minified dev environment for full errors and additional helpful warnings.
at http://localhost:5000/static/js/foo.chunk.js:1:21738
at a (http://localhost:5000/static/js/foo.chunk.js:1:21841)
at ho (http://localhost:5000/static/js/foo.chunk.js:1:68735)
at f (http://localhost:5000/:1:980)`,
};

const stacktrace = _computeStackTrace(REACT_INVARIANT_VIOLATION_EXCEPTION);

expect(stacktrace.stack).deep.equal([
{
args: [],
column: 21738,
context: null,
func: '?',
line: 1,
url: 'http://localhost:5000/static/js/foo.chunk.js',
},
{
args: [],
column: 21841,
context: null,
func: 'a',
line: 1,
url: 'http://localhost:5000/static/js/foo.chunk.js',
},
{
args: [],
column: 68735,
context: null,
func: 'ho',
line: 1,
url: 'http://localhost:5000/static/js/foo.chunk.js',
},
{
args: [],
column: 980,
context: null,
func: 'f',
line: 1,
url: 'http://localhost:5000/',
},
]);
});
});
});