Skip to content
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.

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

Enable getDerivedStateFromError #13746

Merged
merged 10 commits into from
Sep 28, 2018
Merged

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Sep 27, 2018

Commits are split up for easier review:

  • 4ed84b0: Remove the enableGetDerivedStateFromCatch feature flag.
  • cd49672: Copy-paste ReactErrorBoundaries to ReactLegacyErrorBoundaries to preserve tests for legacy componentDidCatch recovery behavior. (We will eventually remove most of these tests when componentDidCatch no longer supports state updates.)
  • 56bbf1c: Update previous ReactErrorBoundaries tests to cover new getDerivedStateFromCatch render-phase behavior and fixed a minor reconciliation bug that testing uncovered.
  • f6fb111: Rename getDerivedStateFromCatch to getDerivedStateFromError.
  • 3df17ba: Add DEV warning if an error boundary specifies only the componentDidCatch lifecycle and does not trigger a state update (because this would swallow the error).
  • Add a test for re-throwing an error from getDerivedStateFromError to ensure that error bubbling works as expected.

(The only commits that aren't just rote find-and-replace are 56bbf1c and 3df17ba.)

@sizebot
Copy link

sizebot commented Sep 27, 2018

ReactDOM: size: 🔺+0.3%, gzip: 🔺+0.2%

Details of bundled changes.

Comparing: a0733fe...f1171b7

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.2% +0.3% 657.45 KB 659.01 KB 153.47 KB 153.9 KB UMD_DEV
react-dom.production.min.js 🔺+0.3% 🔺+0.2% 93.18 KB 93.44 KB 30.31 KB 30.37 KB UMD_PROD
react-dom.development.js +0.2% +0.3% 652.79 KB 654.34 KB 152.09 KB 152.52 KB NODE_DEV
react-dom.production.min.js 🔺+0.3% 🔺+0.2% 93.17 KB 93.44 KB 29.88 KB 29.93 KB NODE_PROD
react-dom-server.browser.development.js -0.1% -0.2% 106.22 KB 106.12 KB 28.26 KB 28.22 KB UMD_DEV
react-dom-server.browser.development.js -0.1% -0.2% 102.35 KB 102.25 KB 27.27 KB 27.22 KB NODE_DEV
react-dom-server.node.development.js -0.1% -0.2% 104.27 KB 104.18 KB 27.83 KB 27.78 KB NODE_DEV
ReactDOM-dev.js +0.3% +0.3% 669.84 KB 671.66 KB 152.73 KB 153.24 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.1% -0.0% 287.26 KB 287.49 KB 52.83 KB 52.81 KB FB_WWW_PROD
ReactDOMServer-dev.js -0.1% -0.1% 102.14 KB 102.06 KB 26.67 KB 26.64 KB FB_WWW_DEV
react-dom.profiling.min.js +0.3% +0.2% 96.16 KB 96.43 KB 30.55 KB 30.6 KB NODE_PROFILING
ReactDOM-profiling.js +0.1% -0.0% 294.1 KB 294.4 KB 54.29 KB 54.28 KB FB_WWW_PROFILING
react-dom.profiling.min.js +0.3% +0.2% 96.1 KB 96.37 KB 30.99 KB 31.05 KB UMD_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.3% +0.4% 447.4 KB 448.96 KB 99.87 KB 100.32 KB UMD_DEV
react-art.production.min.js 🔺+0.3% 🔺+0.1% 85.62 KB 85.88 KB 26.18 KB 26.21 KB UMD_PROD
react-art.development.js +0.4% +0.5% 379.17 KB 380.73 KB 82.7 KB 83.15 KB NODE_DEV
react-art.production.min.js 🔺+0.5% 🔺+0.4% 50.58 KB 50.85 KB 15.44 KB 15.5 KB NODE_PROD
ReactART-dev.js +0.5% +0.6% 382.99 KB 384.81 KB 81.16 KB 81.68 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.1% -0.0% 163.09 KB 163.3 KB 27.43 KB 27.42 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.4% +0.6% 391.18 KB 392.83 KB 85.25 KB 85.73 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.5% 🔺+0.3% 51.8 KB 52.06 KB 15.72 KB 15.77 KB UMD_PROD
react-test-renderer.development.js +0.4% +0.6% 386.75 KB 388.4 KB 84.13 KB 84.6 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.5% 🔺+0.1% 51.52 KB 51.78 KB 15.57 KB 15.59 KB NODE_PROD
ReactTestRenderer-dev.js +0.5% +0.6% 390.72 KB 392.58 KB 82.86 KB 83.39 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.4% +0.5% 375 KB 376.56 KB 80.81 KB 81.24 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.5% 🔺+0.4% 50.27 KB 50.54 KB 14.99 KB 15.05 KB NODE_PROD
react-reconciler-persistent.development.js +0.4% +0.6% 373.43 KB 375.09 KB 80.17 KB 80.64 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.5% 🔺+0.4% 50.29 KB 50.55 KB 15 KB 15.06 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.4% +0.5% 506.86 KB 508.68 KB 111.66 KB 112.19 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.1% -0.0% 218.19 KB 218.39 KB 37.85 KB 37.84 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.4% +0.5% 506.52 KB 508.37 KB 111.57 KB 112.1 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+0.4% 🔺+0.3% 207.87 KB 208.68 KB 36.21 KB 36.3 KB RN_OSS_PROD
ReactFabric-dev.js +0.4% +0.5% 496.97 KB 498.82 KB 109.22 KB 109.75 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.4% 🔺+0.2% 200.29 KB 201.09 KB 34.74 KB 34.83 KB RN_FB_PROD
ReactFabric-dev.js +0.4% +0.5% 497 KB 498.86 KB 109.23 KB 109.77 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.4% 🔺+0.2% 200.32 KB 201.13 KB 34.76 KB 34.85 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.3% +0.2% 215.52 KB 216.27 KB 37.68 KB 37.75 KB RN_OSS_PROFILING
ReactFabric-profiling.js +0.4% +0.2% 207.43 KB 208.17 KB 36.23 KB 36.29 KB RN_OSS_PROFILING
ReactNativeRenderer-profiling.js +0.1% -0.1% 224.15 KB 224.43 KB 39.2 KB 39.18 KB RN_FB_PROFILING
ReactFabric-profiling.js +0.4% +0.2% 207.39 KB 208.13 KB 36.22 KB 36.28 KB RN_FB_PROFILING

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD
scheduler-tracing.development.js -0.9% -1.8% 10.25 KB 10.16 KB 2.35 KB 2.31 KB NODE_DEV
SchedulerTracing-dev.js -0.7% -1.2% 10.34 KB 10.27 KB 2.27 KB 2.25 KB FB_WWW_DEV

Generated by 🚫 dangerJS

@bvaughn bvaughn changed the title Enable getDerivedStateFromCatch [WIP] Enable getDerivedStateFromError Sep 27, 2018
expect(renderer).toHaveRenderedChildren(
React.createElement(fallbackTagName, {prop: 'ErrorBoundary'}),
);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not super confident about this fix ^

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 @@
/**
Copy link
Contributor Author

@bvaughn bvaughn Sep 27, 2018

Choose a reason for hiding this comment

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

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.

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 27, 2018

Rebased and added the DEV warning we talked about.

Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

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

Let's pair on the "force remount children" thing tomorrow

workInProgress.child = null;
} else {
current.child = null;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 27, 2018

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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, thanks!

Brian Vaughn added 3 commits September 28, 2018 08:30
…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.
Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

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

Yay

@bvaughn bvaughn merged commit 806eebd into facebook:master Sep 28, 2018
@bvaughn bvaughn deleted the getDerivedStateFromCatch branch September 28, 2018 20:05
acdlite pushed a commit to plievone/react that referenced this pull request Oct 5, 2018
* 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
@gaearon gaearon mentioned this pull request Oct 23, 2018
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
* 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
@bisubus
Copy link

bisubus commented Dec 18, 2019

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 getDerivedStateFromError(error, errorInfo, prevState) compatible with componentDidCatch(error, errorInfo) signature. @bvaughn @acdlite What do you think?

@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 18, 2019

@bisubus How is this PR blocking anything? 😄 It was merged four months ago.

@bisubus
Copy link

bisubus commented Jan 15, 2020

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants