Construct Error at invariant call site for clearer stack traces #15877
Conversation
|
ReactDOM: size: 🔺+0.5%, gzip: 🔺+0.3% Details of bundled changes.Comparing: cd98e63...765e068 react-dom
react-art
react-test-renderer
react-native-renderer
react-reconciler
Generated by 🚫 dangerJS |
|
throw ReactError();So there's no extra frame that needs popping. |
|
Closing. If I'm mistaken in my reasoning, please let me know with additional details and I'll reopen. |
|
@acdlite, I'm not at my laptop now but I'm quite certain that's incorrect. They are functions that create an Error instance (which is where the stack gets captured) and return it, so there is in fact an extra frame there. I'm seeing this with the error reporting for FB's RN apps - all the renderer invariants currently get aggregated by that first frame inside ReactError. |
Ah right. Ugh... Maybe we should make them actual constructors. Lemme think about this before merging |
|
What if we changed the error transform so that the error is passed to throw ReactError(Error(msg)); |
|
I'd be happy with changing the transform - that sounds like the best way forward for now. A few thoughts:
That gets tricky depending on the execution environment. In a native ES2015 environment like Chrome, extending Error works as you'd expect, but under Babel there are some additional frames on the stack. Currently dealing with this internally :)
Just for full context - the Overall, |
|
The bundle size increase is not great. :-( Can we make this FB-only or something? |
motiz88
changed the title
|
Maybe we can compensate for the increase by making |
|
Hmm, that didn't work the way I wanted it to. I've also tried moving Options I see:
I think 4 is my favourite option now. Any objections? EDIT: Doesn't look like there's currently a way to override specific files with EDIT 2: Found |
|
Open questions:
|
|
Re: the version where you construct the error at the callsite, I inspected the build output and it looks like it only goes up because - throw z(85)
+ throw z(Error(85))This doesn't seem like a huge deal to me. Bundle size as a metric is mostly a proxy for parse time, which I wouldn't expect this to contribute to significantly. However, I wonder what the increase would look like if you alias
It used to work like that, like |
|
Also I would still like to find a solution that works for people outside Facebook and React Native. |
|
If we find the bundle size increase to be acceptable, we have a couple of general solutions ready to go right here in the PR. Aliasing I'll implement any changes you folks think are necessary. |
Meh, don't care :D
Import a module that exports Error? :D I will stamp a version that passes the error in, preferably with Error minified but without it is also fine. |
|
@acdlite: Rolled back to 1488297 and then rebased on master as 765e068. I have a local version that aliases |
Summary
Currently, when a React invariant fails, it throws an
Errorobject constructed inside theReactError(orReactErrorProd) function. Because the stack trace is captured at the point of Error's construction, it always begins with the same frame for ReactError, no matter specific invariant actually failed.This PR moves the Error constructor call to the location of the invariant, removing the stack frame inside ReactError.
Test plan
Added a test in
ReactError-test.internal.js.NOTE: The PR summary has been significantly edited a couple of times; see edit history for more context.