Enable getDerivedStateFromError #13746
Conversation
|
ReactDOM: size: 🔺+0.3%, gzip: 🔺+0.2% Details of bundled changes.Comparing: a0733fe...f1171b7 react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
scheduler
Generated by 🚫 dangerJS |
bvaughn
force-pushed
the
getDerivedStateFromCatch
branch
from
d8700c1 to
a18bc9f
Compare
bvaughn
changed the title
| expect(renderer).toHaveRenderedChildren( | ||
| React.createElement(fallbackTagName, {prop: 'ErrorBoundary'}), | ||
| ); | ||
| } |
There was a problem hiding this comment.
This test was useful for me since my initial "fix" to sync mode broke concurrent mode. I'm not sure about this amount of abstraction though. On the one hand, it makes it very easy to spot what's different between these otherwise very similar tests. On the other hand, it may be harder to read.
| workInProgress.child = null; | ||
| } else { | ||
| current.child = null; | ||
| } |
There was a problem hiding this comment.
I'm not super confident about this fix ^
There was a problem hiding this comment.
Hmm, yeah I don't think this is right. I think I know how to fix it though. Let's pair on it tomorrow morning.
| @@ -0,0 +1,2125 @@ | |||
| /** | |||
There was a problem hiding this comment.
This file was literally copied from ReactErrorBoundaries-test.internal.js so it doesn't really need to be reviewed. Normally I would have git mved it, but in this case– I wanted the reviewer to be able to easily spot the places the new render-phase error boundary behavior differed from the old behavior– so I modified the existing file.
…tly enabled the feature)
…testing componentDidCatch
…nd fixed a minor reconciliation error in begin work.
bvaughn
force-pushed
the
getDerivedStateFromCatch
branch
from
a18bc9f to
3df17ba
Compare
|
Rebased and added the DEV warning we talked about. |
acdlite
left a comment
acdlite
left a comment
There was a problem hiding this comment.
Let's pair on the "force remount children" thing tomorrow
| workInProgress.child = null; | ||
| } else { | ||
| current.child = null; | ||
| } |
There was a problem hiding this comment.
Hmm, yeah I don't think this is right. I think I know how to fix it though. Let's pair on it tomorrow morning.
Sounds great! |
| // If no state update is scheduled then the boundary will swallow the error. | ||
| const updateQueue = fiber.updateQueue; | ||
| warningWithoutStack( | ||
| updateQueue !== null && updateQueue.firstUpdate !== null, |
There was a problem hiding this comment.
This check isn't sufficient because firstUpdate might be a low priority update. It needs to be sync. You can check fiber.expirationTime === Sync instead.
…ention of throw I think this is confusing because it implies that gDSFE can throw and cDC will still be called, when that is not the case (because cDC is a commit phase lifecycle- and if gDSFE rethrows, the boundary in question will never mount and the commit phase menthods it defines will not be called.
bvaughn
force-pushed
the
getDerivedStateFromCatch
branch
from
333d4e7 to
54f637f
Compare
* Removed the enableGetDerivedStateFromCatch feature flag (aka permanently enabled the feature) * Forked/copied ReactErrorBoundaries to ReactLegacyErrorBoundaries for testing componentDidCatch * Updated error boundaries tests to apply to getDerivedStateFromCatch * Renamed getDerivedStateFromCatch -> getDerivedStateFromError * Warn if boundary with only componentDidCatch swallows error * Fixed a subtle reconciliation bug with render phase error boundary
* Removed the enableGetDerivedStateFromCatch feature flag (aka permanently enabled the feature) * Forked/copied ReactErrorBoundaries to ReactLegacyErrorBoundaries for testing componentDidCatch * Updated error boundaries tests to apply to getDerivedStateFromCatch * Renamed getDerivedStateFromCatch -> getDerivedStateFromError * Warn if boundary with only componentDidCatch swallows error * Fixed a subtle reconciliation bug with render phase error boundary
|
Greetings, just noticed that this PR has been blocking mine, #13986 . Incidentally, second errorInfo param still isn't in use, https://github.com/facebook/react/pull/13746/files#diff-1996f2b11f9c68c0a81652e32be88ddbR109 . I could fix this in my PR and make |
|
@bisubus How is this PR blocking anything? 😄 It was merged four months ago. |
|
@bvaughn Bad wording, it's the merge that was blocked. And it was a year and a few months, to be precise, I apologize for a blast from the past. Any way, I think I addressed the problem with errorInfo from this PR. |
5 participants
Commits are split up for easier review:
enableGetDerivedStateFromCatchfeature flag.ReactErrorBoundariestoReactLegacyErrorBoundariesto preserve tests for legacycomponentDidCatchrecovery behavior. (We will eventually remove most of these tests whencomponentDidCatchno longer supports state updates.)ReactErrorBoundariestests to cover newgetDerivedStateFromCatchrender-phase behavior and fixed a minor reconciliation bug that testing uncovered.getDerivedStateFromCatchtogetDerivedStateFromError.3df17ba: Add DEV warning if an error boundary specifies only thecomponentDidCatchlifecycle and does not trigger a state update (because this would swallow the error).getDerivedStateFromErrorto ensure that error bubbling works as expected.(The only commits that aren't just rote find-and-replace are 56bbf1c and
3df17ba.)