Don't Rethrow Errors at the Root (#28627)

Summary:
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.

---------

DiffTrain build for commit https://github.com/facebook/react/commit/6786563f3cbbc9b16d5a8187207b5bd904386e53.

Changelog:
[Internal]

Reviewed By: kassens

Differential Revision: D55408481

Pulled By: yungsters

fbshipit-source-id: 598aa306369e21cb3e93ad6041a87bfbaa9eef9e

Co-authored-by: Ricky Hanlon <rickhanlonii@gmail.com>
This commit is contained in:
sebmarkbage
2024-03-29 01:42:24 -07:00
committed by Facebook GitHub Bot
co-authored by Ricky Hanlon
parent e175912e7d
commit 78d6873b04
2 changed files with 220 additions and 220 deletions
@@ -25,7 +25,7 @@ jest
import {format} from 'node:util';
import * as React from 'react';
import TestRenderer from 'react-test-renderer';
import {act, create} from 'react-test-renderer';
const Animated = require('../Animated').default;
const NativeAnimatedHelper = require('../NativeAnimatedHelper').default;
@@ -67,7 +67,7 @@ describe('Native Animated', () => {
useNativeDriver: true,
}).start();
TestRenderer.create(<Animated.View ref={ref} style={{opacity}} />);
create(<Animated.View ref={ref} style={{opacity}} />);
expect(ref.current).not.toBeNull();
jest.spyOn(ref.current, 'setNativeProps');
@@ -86,7 +86,7 @@ describe('Native Animated', () => {
opacity.setOffset(10);
opacity.__makeNative();
TestRenderer.create(<Animated.View style={{opacity}} />);
create(<Animated.View style={{opacity}} />);
expect(NativeAnimatedModule.createAnimatedNode).toBeCalledWith(
expect.any(Number),
@@ -103,7 +103,7 @@ describe('Native Animated', () => {
const opacity = new Animated.Value(0);
opacity.__makeNative();
TestRenderer.create(<Animated.View style={{opacity}} />);
create(<Animated.View style={{opacity}} />);
expect(NativeAnimatedModule.createAnimatedNode).toBeCalledWith(
expect.any(Number),
@@ -123,7 +123,7 @@ describe('Native Animated', () => {
opacity.__makeNative();
const root = TestRenderer.create(<Animated.View style={{opacity}} />);
const root = create(<Animated.View style={{opacity}} />);
const tag = opacity.__getNativeTag();
root.unmount();
@@ -145,7 +145,7 @@ describe('Native Animated', () => {
opacity.setOffset(0.5);
opacity.__makeNative();
const root = TestRenderer.create(<Animated.View style={{opacity}} />);
const root = create(<Animated.View style={{opacity}} />);
const tag = opacity.__getNativeTag();
root.unmount();
@@ -161,7 +161,7 @@ describe('Native Animated', () => {
const opacity = new Animated.Value(0);
opacity.__makeNative();
TestRenderer.create(<Animated.View style={{opacity}} />);
create(<Animated.View style={{opacity}} />);
expect(NativeAnimatedModule.createAnimatedNode).toBeCalledWith(
expect.any(Number),
@@ -250,7 +250,7 @@ describe('Native Animated', () => {
useNativeDriver: true,
});
const root = TestRenderer.create(<Animated.View onTouchMove={event} />);
const root = create(<Animated.View onTouchMove={event} />);
expect(NativeAnimatedModule.addAnimatedEventToView).toBeCalledWith(
expect.any(Number),
'onTouchMove',
@@ -278,7 +278,7 @@ describe('Native Animated', () => {
useNativeDriver: true,
});
TestRenderer.create(<Animated.View onTouchMove={event} />);
create(<Animated.View onTouchMove={event} />);
['x', 'y'].forEach((key, idx) =>
expect(
NativeAnimatedModule.addAnimatedEventToView,
@@ -289,7 +289,7 @@ describe('Native Animated', () => {
);
});
it('should throw on invalid event path', () => {
it('should throw on invalid event path', async () => {
const value = new Animated.Value(0);
value.__makeNative();
const event = Animated.event([{notNativeEvent: {foo: value}}], {
@@ -305,9 +305,11 @@ describe('Native Animated', () => {
consoleError(...args);
});
expect(() => {
TestRenderer.create(<Animated.View onTouchMove={event} />);
}).toThrowError(/nativeEvent/);
await expect(async () => {
await act(() => {
create(<Animated.View onTouchMove={event} />);
});
}).rejects.toThrowError(/nativeEvent/);
expect(NativeAnimatedModule.addAnimatedEventToView).not.toBeCalled();
console.error.mockRestore();
@@ -331,7 +333,7 @@ describe('Native Animated', () => {
describe('Animated Graph', () => {
it('creates and detaches nodes', () => {
const opacity = new Animated.Value(0);
const root = TestRenderer.create(<Animated.View style={{opacity}} />);
const root = create(<Animated.View style={{opacity}} />);
Animated.timing(opacity, {
toValue: 10,
@@ -371,7 +373,7 @@ describe('Native Animated', () => {
it('sends a valid description for value, style and props nodes', () => {
const opacity = new Animated.Value(0);
TestRenderer.create(<Animated.View style={{opacity}} />);
create(<Animated.View style={{opacity}} />);
Animated.timing(opacity, {
toValue: 10,
@@ -399,9 +401,7 @@ describe('Native Animated', () => {
first.__makeNative();
second.__makeNative();
TestRenderer.create(
<Animated.View style={{opacity: Animated.add(first, second)}} />,
);
create(<Animated.View style={{opacity: Animated.add(first, second)}} />);
expect(NativeAnimatedModule.createAnimatedNode).toBeCalledWith(
expect.any(Number),
@@ -443,7 +443,7 @@ describe('Native Animated', () => {
first.__makeNative();
second.__makeNative();
TestRenderer.create(
create(
<Animated.View style={{opacity: Animated.subtract(first, second)}} />,
);
@@ -487,7 +487,7 @@ describe('Native Animated', () => {
first.__makeNative();
second.__makeNative();
TestRenderer.create(
create(
<Animated.View style={{opacity: Animated.multiply(first, second)}} />,
);
@@ -531,7 +531,7 @@ describe('Native Animated', () => {
first.__makeNative();
second.__makeNative();
TestRenderer.create(
create(
<Animated.View style={{opacity: Animated.divide(first, second)}} />,
);
@@ -573,9 +573,7 @@ describe('Native Animated', () => {
const value = new Animated.Value(4);
value.__makeNative();
TestRenderer.create(
<Animated.View style={{opacity: Animated.modulo(value, 4)}} />,
);
create(<Animated.View style={{opacity: Animated.modulo(value, 4)}} />);
expect(NativeAnimatedModule.createAnimatedNode).toBeCalledWith(
expect.any(Number),
@@ -607,7 +605,7 @@ describe('Native Animated', () => {
const value = new Animated.Value(10);
value.__makeNative();
TestRenderer.create(
create(
<Animated.View
style={{
opacity: value.interpolate({
@@ -651,9 +649,7 @@ describe('Native Animated', () => {
const translateX = new Animated.Value(0);
translateX.__makeNative();
TestRenderer.create(
<Animated.View style={{transform: [{translateX}, {scale: 2}]}} />,
);
create(<Animated.View style={{transform: [{translateX}, {scale: 2}]}} />);
expect(NativeAnimatedModule.createAnimatedNode).toBeCalledWith(
expect.any(Number),
@@ -678,7 +674,7 @@ describe('Native Animated', () => {
it('sends create operations before connect operations for multiple animated style props', () => {
const opacity = new Animated.Value(0);
const borderRadius = new Animated.Value(0);
TestRenderer.create(<Animated.View style={{borderRadius, opacity}} />);
create(<Animated.View style={{borderRadius, opacity}} />);
Animated.timing(opacity, {
toValue: 10,
@@ -772,7 +768,7 @@ describe('Native Animated', () => {
it('sends create operations before connect operations for multiple animated transform props', () => {
const translateX = new Animated.Value(0);
const translateY = new Animated.Value(0);
TestRenderer.create(
create(
<Animated.View
style={{
transform: [{translateX: translateX}, {translateY: translateY}],
@@ -894,7 +890,7 @@ describe('Native Animated', () => {
it('sends create operations before connect operations for multiple animated props', () => {
const propA = new Animated.Value(0);
const propB = new Animated.Value(0);
TestRenderer.create(<Animated.View propA={propA} propB={propB} />);
create(<Animated.View propA={propA} propB={propB} />);
Animated.timing(propA, {
toValue: 10,
@@ -967,7 +963,7 @@ describe('Native Animated', () => {
const value = new Animated.Value(2);
value.__makeNative();
TestRenderer.create(
create(
<Animated.View style={{opacity: Animated.diffClamp(value, 0, 20)}} />,
);
@@ -1000,7 +996,7 @@ describe('Native Animated', () => {
it("doesn't call into native API if useNativeDriver is set to false", () => {
const opacity = new Animated.Value(0);
const root = TestRenderer.create(<Animated.View style={{opacity}} />);
const root = create(<Animated.View style={{opacity}} />);
Animated.timing(opacity, {
toValue: 10,
@@ -1017,7 +1013,7 @@ describe('Native Animated', () => {
const opacity = new Animated.Value(0);
const ref = React.createRef(null);
TestRenderer.create(<Animated.View ref={ref} style={{opacity}} />);
create(<Animated.View ref={ref} style={{opacity}} />);
// Necessary to simulate the native animation.
expect(ref.current).not.toBeNull();
@@ -1046,7 +1042,7 @@ describe('Native Animated', () => {
it('fails for unsupported styles', () => {
const left = new Animated.Value(0);
TestRenderer.create(<Animated.View style={{left}} />);
create(<Animated.View style={{left}} />);
const animation = Animated.timing(left, {
toValue: 10,
@@ -1061,7 +1057,7 @@ describe('Native Animated', () => {
const opacity = new Animated.Value(0);
opacity.__makeNative();
TestRenderer.create(
create(
<Animated.View
removeClippedSubviews={true}
style={{left: 10, opacity, top: 20}}
@@ -1269,7 +1265,7 @@ describe('Native Animated', () => {
const opacity = new Animated.Value(0);
opacity.__makeNative();
const root = TestRenderer.create(<Animated.View style={{opacity}} />);
const root = create(<Animated.View style={{opacity}} />);
expect(NativeAnimatedModule.restoreDefaultValues).not.toHaveBeenCalled();
root.update(<Animated.View style={{opacity}} />);
File diff suppressed because it is too large Load Diff