[Fiber] Preserve "Break on all/uncaught exceptions" behavior in DEV mode #8961
Conversation
| }; | ||
| var evtType = `react-${name}`; | ||
| const evtType = 'react-' + (name ? name : 'invokeguardedcallback'); |
There was a problem hiding this comment.
Is the name argument worth keeping?
| } catch (error) { | ||
| let error; | ||
| if (__DEV__) { | ||
| error = invokeGuardedCallback('commitphase1', commitAllHostEffects, null, finishedWork); |
There was a problem hiding this comment.
I'm guessing the name parameter is only important for ErrorUtils. I'll come back to this before landing.
There was a problem hiding this comment.
yea but I'm not sure we need this feature from ErrorUtils neither. Maybe for event handlers I guess it is pretty useful.
acdlite
force-pushed
the
fiberbreakonuncaught
branch
from
7d52f8c
to
66bd8fd
Compare
acdlite
changed the title
|
@sebmarkbage Ready for review |
acdlite
force-pushed
the
fiberbreakonuncaught
branch
from
66bd8fd
to
92d7497
Compare
There was a problem hiding this comment.
I'm not too happy with how invokeGuardedCallback gets optimized, even with a compiler. It probably won't be able to strip out the array slicing etc.
Can we reorganize our callers so that they use try/catch locally in a prod mode so that the local stack frame optimizations can do the best they can? Even if try/catch deopts the function in some VMs.
And then we only invoke invokeGuardedCallback if we're in DEV mode.
acdlite
force-pushed
the
fiberbreakonuncaught
branch
from
92d7497
to
5202ef3
Compare
acdlite
dismissed
sebmarkbage’s stale review
Updated to use a local try-catch when not in DEV
| } | ||
| } | ||
| if (refError !== null) { | ||
| captureError(current, refError); |
There was a problem hiding this comment.
Can we move this into the try/catch and only do the if (ref !== null) for the __DEV__ case?
acdlite
force-pushed
the
fiberbreakonuncaught
branch
from
5202ef3
to
2ed6229
Compare
| } | ||
|
|
||
| var ReactErrorUtils = { | ||
| invokeGuardedCallback: invokeGuardedCallback, | ||
| invokeGuardedCallback, | ||
| invokeGuardedCallbackProd: invokeGuardedCallback, |
There was a problem hiding this comment.
Can we avoid exposing this since we're not using it? Waste of bytes.
We can use this more flexible version for error handling in Fiber. The DEV mode version uses same fake event trick as before to preserve "break on uncaught exception" behavior when debugging.
Added a guard in the global error event listener to prevent nested errors from being captured higher up the stack. Also found a bug where the DEV version of invokeGuardedCallback would infinite loop if there were nested invocations with the same name. Fixed by appending the depth to the fake event name. (I'm actually not sure why this is necessary.)
Only in DEV. In production, continue using a local try-catch.
We weren't using it anywhere except for the test suite. Instead, we can change the environment flag and run the tests in both environments.
acdlite
force-pushed
the
fiberbreakonuncaught
branch
from
2ed6229
to
2c59713
Compare
invokeGuardedCallbackso that it returns a thrown error. Need this for Fiber's error handling.invokeGuardedCallbackin place of Fiber's try/catch blocks.