[Fiber] Don't Rethrow Errors at the Root #28627
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sebmarkbage
commented
Mar 25, 2024
| logCapturedError( | ||
| root.current, | ||
| createCapturedValueAtFiber(error, root.current), | ||
| ); |
There was a problem hiding this comment.
Instead of stashing this and then logging, I just log it directly. I think that's fine since nothing else is really supposed to be happening in between but not sure. We can do that now since it's not rethrowing so we still will finish the commit after the log.
|
Comparing: 5910eb3...0928d91 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show |
d6a24f1 to
084ab0d
Compare
sebmarkbage
commented
Mar 25, 2024
| event.preventDefault(); | ||
| caughtErrors.push(event.error); | ||
| } | ||
| window.addEventListener('error', errorHandler); |
There was a problem hiding this comment.
We don't have "act" in the TypeScript environment.
| @@ -140,6 +142,6 @@ describe('ReactFlushSync (AggregateError not available)', () => { | |||
| // AggregateError is not available, React throws the first error, then | |||
| // throws the remaining errors in separate tasks. | |||
| expect(error).toBe(aahh); | |||
| expect(flushFakeMicrotasks).toThrow(nooo); | |||
| await flushFakeMicrotasks(); | |||
There was a problem hiding this comment.
Should we assert somewhere that this was thrown to reportError or whatever?
There was a problem hiding this comment.
So it's a bit weird about what we should be testing here. Because this is to test that something works even if aggregated errors is not supported by the platform. However, we only do aggregation at all in act() now. So we're testing act() and if you use act in an environment that doesn't support aggregated errors, then this does get dropped. Not logged somewhere else.
Which I guess is mostly if you use act in a non-JSDOM environment.
There was a problem hiding this comment.
Makes sense, we should probably update the comment on 143 then
packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js
Show resolved
Hide resolved
| expect.objectContaining({ | ||
| message: 'Boom', | ||
| }), | ||
| ], | ||
| ]); | ||
| expect(console.warn.mock.calls).toEqual([ |
There was a problem hiding this comment.
We're moving some logs from console.error to console.warn right? Can we add that to the "Breaking changes" in the description?
There was a problem hiding this comment.
So it used to be that we had two logs. One for the error and then one for the suffix. In my previous PR removing invokeGuardedCallback I was able to unify them into one but what this is doing is basically restoring it into two because we can't pass a component stack to reportGlobalError for uncaught but we can for caught which just goes to console.error.
It really isn't great to have the two logs, especially if the former gets called with preventDefault which used to silence the second one but we can't do that anymore.
I'm kind of inclined to say that we shouldn't have this second log at all and let uncaught not have component stacks by default and instead if you want that you implement it yourself using onUncaughtError.
There was a problem hiding this comment.
I like it. But why switch from error to warn?
Would using onUncaughtError result in double logging or would we not use reportGlobalError if that's provided?
There was a problem hiding this comment.
The warn is mainly about telling you to add an error boundary. It's not really so much about the component stack anymore although it includes it. Where as before it was more like two console.error as a pair. The console.warn can stand on its own.
You should really always have a root error boundary and if you don't want to you should at least implement onUncaughtError. So this shouldn't really be a case you need to hit beyond seeing the warning.
onUncaughtError takes control so this doesn't get logged then.
|
[WIP comment] I'm going to update this as I go through it Making some notes as I fix the tests, it looks like these are some changes included: Internal Tests
Single error // TODO: why do some of my assertions require the extra array and not others?
await expect(async () => {
await act(() => {
root.render(<WithError />);
});
}).rejects.toThrow(
expect.objectContaining({
message: "Boom",
}),
);Multiple errors with asymmetric matchers await expect(async () => {
await act(() => {
root.render(<WithError />);
});
}).rejects.toThrow(
expect.objectContaining({
errors: [
expect.objectContaining({
message: "Boom",
}),
expect.objectContaining({
message: "Boom",
}),
],
}),
);Multiple errors with error constructors await expect(async () => {
await act(() => {
root.render(<WithError />);
});
}).rejects.toThrow(
// eslint-disable-next-line no-undef
new AggregateError([
new Error('Boom'),
new Error('Boom'),
]),
);Multiple errors with waitForThrow helper root.render(<WithError /).
const aggregateError = await waitForThrow();
expect(aggregateError.errors.length).toBe(2);
expect(aggregateError.errors[0].message).toContain('Boom');
expect(aggregateError.errors[0].message).toContain('Boom');Public builds
|
|
Things that suck about the
|
|
I fixed the legacy tests but there's still two non-legacy left: Remaining tests to fix
|
| 'Warning: React.jsx: type is invalid -- expected a string ' + | ||
| '(for built-in components) or a class/function ' + | ||
| '(for composite components) but got: null.', | ||
| {withoutStack: 1}, |
There was a problem hiding this comment.
I don't understand why this switched from React.createElement to React.jsx?
I assume that's also why there's no component stack either?
There was a problem hiding this comment.
It wasn't actually asserted before because the render threw which means the assertion wasn't run. The assertion before was asserting the wrong thing.
| }).toErrorDev( | ||
| 'Expected the last optional `callback` argument to be ' + | ||
| 'a function. Instead received: no.', | ||
| {withoutStack: 1}, |
There was a problem hiding this comment.
Many of the test changes here removed the component stacks, is that expected?
There was a problem hiding this comment.
These are the same. They weren't actually asserting before.
These should always be inside of DOM event listeners anyway. This lets us report more than one error. Not sure what to do about the React Native one.
This ensures that they can get logged properly through the regular onerror mechanism.
Also let waitForThrow listen to global errors too.
We try to throw the first error discovered even if other errors happen later which more closely models what used to happen when the act scope was aborted early.
This way you can nest internalAct/waitFor.
Since act now swallows the logging, we need to use a fakeAct to test these cases. Since there's no rethrow, only logging, this case doesn't throw the original error anymore.
…rors Such as errors logged in event handlers and onRecoverableErrors. This is different from the public act which doesn't catch those.
f9ac852 to
613ae34
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Mar 27, 2024
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 `act` we track the error into act in an aggregate error and then rethrow it at the root of `act`. 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 `flushSync` to `act` if you assert on throwing. However, in the React repo we also have `internalAct` and the `waitForThrow` helpers. 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 added `An error occurred`. ### Polyfill All browsers we support really supports `reportError` but not all test and server environments do, so I implemented a polyfill for browser and node in `shared/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. --------- Co-authored-by: Ricky Hanlon <[email protected]> DiffTrain build for [6786563](6786563)
sebmarkbage
added a commit
that referenced
this pull request
Mar 27, 2024
Stacked on #28627. This makes error logging configurable using these `createRoot`/`hydrateRoot` options: ``` onUncaughtError(error: mixed, errorInfo: {componentStack?: ?string}) => void onCaughtError(error: mixed, errorInfo: {componentStack?: ?string, errorBoundary?: ?React.Component<any, any>}) => void onRecoverableError(error: mixed, errorInfo: {digest?: ?string, componentStack?: ?string}) => void ``` We already have the `onRecoverableError` option since before. Overriding these can be used to implement custom error dialogs (with access to the `componentStack`). It can also be used to silence caught errors when testing an error boundary or if you prefer not getting logs for caught errors that you've already handled in an error boundary. I currently expose the error boundary instance but I think we should probably remove that since it doesn't make sense for non-class error boundaries and isn't very useful anyway. It's also unclear what it should do when an error is rethrown from one boundary to another. Since these are public APIs now we can implement the ReactFiberErrorDialog forks using these options at the roots of the builds. So I unforked those files and instead passed a custom option for the native and www builds. To do this I had to fork the ReactDOMLegacy file into ReactDOMRootFB which is a duplication but that will go away as soon as the FB fork is the only legacy root.
github-actions bot
pushed a commit
that referenced
this pull request
Mar 27, 2024
Stacked on #28627. This makes error logging configurable using these `createRoot`/`hydrateRoot` options: ``` onUncaughtError(error: mixed, errorInfo: {componentStack?: ?string}) => void onCaughtError(error: mixed, errorInfo: {componentStack?: ?string, errorBoundary?: ?React.Component<any, any>}) => void onRecoverableError(error: mixed, errorInfo: {digest?: ?string, componentStack?: ?string}) => void ``` We already have the `onRecoverableError` option since before. Overriding these can be used to implement custom error dialogs (with access to the `componentStack`). It can also be used to silence caught errors when testing an error boundary or if you prefer not getting logs for caught errors that you've already handled in an error boundary. I currently expose the error boundary instance but I think we should probably remove that since it doesn't make sense for non-class error boundaries and isn't very useful anyway. It's also unclear what it should do when an error is rethrown from one boundary to another. Since these are public APIs now we can implement the ReactFiberErrorDialog forks using these options at the roots of the builds. So I unforked those files and instead passed a custom option for the native and www builds. To do this I had to fork the ReactDOMLegacy file into ReactDOMRootFB which is a duplication but that will go away as soon as the FB fork is the only legacy root. DiffTrain build for [a053716](a053716)
5 tasks
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
Stacked on top of facebook#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 `act` we track the error into act in an aggregate error and then rethrow it at the root of `act`. 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 `flushSync` to `act` if you assert on throwing. However, in the React repo we also have `internalAct` and the `waitForThrow` helpers. 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 added `An error occurred`. ### Polyfill All browsers we support really supports `reportError` but not all test and server environments do, so I implemented a polyfill for browser and node in `shared/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. --------- Co-authored-by: Ricky Hanlon <[email protected]>
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
Stacked on facebook#28627. This makes error logging configurable using these `createRoot`/`hydrateRoot` options: ``` onUncaughtError(error: mixed, errorInfo: {componentStack?: ?string}) => void onCaughtError(error: mixed, errorInfo: {componentStack?: ?string, errorBoundary?: ?React.Component<any, any>}) => void onRecoverableError(error: mixed, errorInfo: {digest?: ?string, componentStack?: ?string}) => void ``` We already have the `onRecoverableError` option since before. Overriding these can be used to implement custom error dialogs (with access to the `componentStack`). It can also be used to silence caught errors when testing an error boundary or if you prefer not getting logs for caught errors that you've already handled in an error boundary. I currently expose the error boundary instance but I think we should probably remove that since it doesn't make sense for non-class error boundaries and isn't very useful anyway. It's also unclear what it should do when an error is rethrown from one boundary to another. Since these are public APIs now we can implement the ReactFiberErrorDialog forks using these options at the roots of the builds. So I unforked those files and instead passed a custom option for the native and www builds. To do this I had to fork the ReactDOMLegacy file into ReactDOMRootFB which is a duplication but that will go away as soon as the FB fork is the only legacy root.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
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 `act` we track the error into act in an aggregate error and then rethrow it at the root of `act`. 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 `flushSync` to `act` if you assert on throwing. However, in the React repo we also have `internalAct` and the `waitForThrow` helpers. 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 added `An error occurred`. ### Polyfill All browsers we support really supports `reportError` but not all test and server environments do, so I implemented a polyfill for browser and node in `shared/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. --------- Co-authored-by: Ricky Hanlon <[email protected]> DiffTrain build for commit 6786563.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
Stacked on #28627. This makes error logging configurable using these `createRoot`/`hydrateRoot` options: ``` onUncaughtError(error: mixed, errorInfo: {componentStack?: ?string}) => void onCaughtError(error: mixed, errorInfo: {componentStack?: ?string, errorBoundary?: ?React.Component<any, any>}) => void onRecoverableError(error: mixed, errorInfo: {digest?: ?string, componentStack?: ?string}) => void ``` We already have the `onRecoverableError` option since before. Overriding these can be used to implement custom error dialogs (with access to the `componentStack`). It can also be used to silence caught errors when testing an error boundary or if you prefer not getting logs for caught errors that you've already handled in an error boundary. I currently expose the error boundary instance but I think we should probably remove that since it doesn't make sense for non-class error boundaries and isn't very useful anyway. It's also unclear what it should do when an error is rethrown from one boundary to another. Since these are public APIs now we can implement the ReactFiberErrorDialog forks using these options at the roots of the builds. So I unforked those files and instead passed a custom option for the native and www builds. To do this I had to fork the ReactDOMLegacy file into ReactDOMRootFB which is a duplication but that will go away as soon as the FB fork is the only legacy root. DiffTrain build for commit a053716.
This was referenced Apr 24, 2024
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.