[Fiber] Don't Rethrow Errors at the Root#28627
Merged
sebmarkbage merged 24 commits intofacebook:mainfrom Mar 27, 2024
Merged
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on top of #28498 for test fixes.
Don't Rethrow
When we started React it was 1:1 setState calls a series of renders and if they error, it errors where the setState was called. Simple. However, then batching came and the error actually got thrown somewhere else. With concurrent mode, it's not even possible to get setState itself to throw anymore.
In fact, all APIs that can rethrow out of React are executed either at the root of the scheduler or inside a DOM event handler.
If you throw inside a React.startTransition callback that's sync, then that will bubble out of the startTransition but if you throw inside an async callback or a useTransition we now need to handle it at the hook site. So in 19 we need to make all React.startTransition swallow the error (and report them to reportError).
The only one remaining that can throw is flushSync but it doesn't really make sense for it to throw at the callsite neither because batching. Just because something rendered in this flush doesn't mean it was rendered due to what was just scheduled and doesn't mean that it should abort any of the remaining code afterwards. setState is fire and forget. It's send an instruction elsewhere, it's not part of the current imperative code.
Error boundaries never rethrow. Since you should really always have error boundaries, most of the time, it wouldn't rethrow anyway.
Rethrowing also actually currently drops errors on the floor since we can only rethrow the first error, so to avoid that we'd need to call reportError anyway. This happens in RN events.
The other issue with rethrowing is that it logs an extra console.error. Since we're not sure that user code will actually log it anywhere we still log it too just like we do with errors inside error boundaries which leads all of these to log twice.
The goal of this PR is to never rethrow out of React instead, errors outside of error boundaries get logged to reportError. Event system errors too.
Breaking Changes
The main thing this affects is testing where you want to inspect the errors thrown. To make it easier to port, if you're inside
actwe track the error into act in an aggregate error and then rethrow it at the root ofact. Unlike before though, if you flush synchronously inside of act it'll still continue until the end of act before rethrowing.I expect most user code breakages would be to migrate from
flushSynctoactif you assert on throwing.However, in the React repo we also have
internalActand thewaitForThrowhelpers. Since these have to use public production implementations we track these using the global onerror or process uncaughtException. Unlike regular act, includes both event handler errors and onRecoverableError by default too. Not just render/commit errors. So I had to account for that in our tests.We restore logging an extra log for uncaught errors after the main log with the component stack in it. We use
console.warn. This is not yet ignorable if you preventDefault to the main error event. To avoid confusion if you don't end up logging the error to console I just addedAn error occurred.Polyfill
All browsers we support really supports
reportErrorbut not all test and server environments do, so I implemented a polyfill for browser and node inshared/reportGlobalError. I don't love that this is included in all builds and gets duplicated into isomorphic even though it's not actually needed in production. Maybe in the future we can require a polyfill for this.Follow Ups
In a follow up, I'll make caught vs uncaught error handling be configurable too.