Skip to content

Commit 33898db

Browse files
sebmarkbagerickhanlonii
authored and
facebook-github-bot
committed
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 facebook/react@6786563. Changelog: [Internal] Reviewed By: kassens Differential Revision: D55408481 Pulled By: yungsters fbshipit-source-id: 598aa306369e21cb3e93ad6041a87bfbaa9eef9e Co-authored-by: Ricky Hanlon <[email protected]>
1 parent cc691d6 commit 33898db

File tree

2 files changed

+100
-86
lines changed

2 files changed

+100
-86
lines changed

‎packages/react-relay/__tests__/LiveResolvers-test.js

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -881,7 +881,7 @@ describe.each([['New', useFragment]])(
881881
});
882882
});
883883

884-
test('Live Resolver with Missing Data and @required', () => {
884+
test('Live Resolver with Missing Data and @required', async () => {
885885
function InnerTestComponent({id}: {id: string}) {
886886
const data = useLazyLoadQuery(
887887
graphql`
@@ -955,9 +955,13 @@ describe.each([['New', useFragment]])(
955955
const environment = createEnvironment(source);
956956

957957
// First, render with missing data
958-
expect(() => {
959-
TestRenderer.create(<TestComponent environment={environment} id="1" />);
960-
}).toThrow(
958+
await expect(async () => {
959+
await TestRenderer.act(() => {
960+
TestRenderer.create(
961+
<TestComponent environment={environment} id="1" />,
962+
);
963+
});
964+
}).rejects.toThrow(
961965
"Relay: Missing @required value at path 'username' in 'ResolverThatThrows'.",
962966
);
963967
expect(relayFieldLogger.mock.calls).toEqual([

‎packages/react-relay/relay-hooks/__tests__/MatchContainer-test.js

Lines changed: 92 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -73,14 +73,16 @@ describe('MatchContainer', () => {
7373
));
7474
});
7575

76-
it('throws when match prop is null', () => {
76+
it('throws when match prop is null', async () => {
7777
// This prevents console.error output in the test, which is expected
7878
jest.spyOn(console, 'error').mockImplementationOnce(() => {});
79-
expect(() => {
80-
TestRenderer.create(
81-
<MatchContainer loader={loader} match={(42: $FlowFixMe)} />,
82-
);
83-
}).toThrow(
79+
await expect(async () => {
80+
await TestRenderer.act(() => {
81+
TestRenderer.create(
82+
<MatchContainer loader={loader} match={(42: $FlowFixMe)} />,
83+
);
84+
});
85+
}).rejects.toThrow(
8486
'MatchContainer: Expected `match` value to be an object or null/undefined.',
8587
);
8688
});
@@ -242,110 +244,118 @@ describe('MatchContainer', () => {
242244
expect(Fallback).toBeCalledTimes(1);
243245
});
244246

245-
it('throws if the match object is invalid (__id)', () => {
247+
it('throws if the match object is invalid (__id)', async () => {
246248
jest.spyOn(console, 'error').mockImplementationOnce(() => {});
247249
loader.mockReturnValue(React.memo((UserComponent: $FlowFixMe)));
248250
const otherProps = {otherProp: 'hello!'};
249251
const Fallback: $FlowFixMe = jest.fn(() => <div>fallback</div>);
250-
expect(() => {
251-
TestRenderer.create(
252-
<MatchContainer
253-
loader={loader}
254-
match={
255-
({
256-
__id: 42, // not a string
257-
__fragments: null,
258-
__fragmentPropName: null,
259-
__fragmentOwner: null,
260-
__module_component: null,
261-
}: $FlowFixMe)
262-
} // intentionally all null
263-
props={otherProps}
264-
fallback={(<Fallback />: $FlowFixMe)}
265-
/>,
266-
);
267-
}).toThrow(
252+
await expect(async () => {
253+
await TestRenderer.act(() => {
254+
TestRenderer.create(
255+
<MatchContainer
256+
loader={loader}
257+
match={
258+
({
259+
__id: 42, // not a string
260+
__fragments: null,
261+
__fragmentPropName: null,
262+
__fragmentOwner: null,
263+
__module_component: null,
264+
}: $FlowFixMe)
265+
} // intentionally all null
266+
props={otherProps}
267+
fallback={(<Fallback />: $FlowFixMe)}
268+
/>,
269+
);
270+
});
271+
}).rejects.toThrow(
268272
"MatchContainer: Invalid 'match' value, expected an object that has a '...SomeFragment' spread.",
269273
);
270274
});
271275

272-
it('throws if the match object is invalid (__fragments)', () => {
276+
it('throws if the match object is invalid (__fragments)', async () => {
273277
jest.spyOn(console, 'error').mockImplementationOnce(() => {});
274278
loader.mockReturnValue(React.memo((UserComponent: $FlowFixMe)));
275279
const otherProps = {otherProp: 'hello!'};
276280
const Fallback: $FlowFixMe = jest.fn(() => <div>fallback</div>);
277-
expect(() => {
278-
TestRenderer.create(
279-
<MatchContainer
280-
loader={loader}
281-
match={
282-
({
283-
__id: null,
284-
__fragments: 42, // not an object
285-
__fragmentPropName: null,
286-
__fragmentOwner: null,
287-
__module_component: null,
288-
}: $FlowFixMe)
289-
} // intentionally all null
290-
props={otherProps}
291-
fallback={(<Fallback />: $FlowFixMe)}
292-
/>,
293-
);
294-
}).toThrow(
281+
await expect(async () => {
282+
await TestRenderer.act(() => {
283+
TestRenderer.create(
284+
<MatchContainer
285+
loader={loader}
286+
match={
287+
({
288+
__id: null,
289+
__fragments: 42, // not an object
290+
__fragmentPropName: null,
291+
__fragmentOwner: null,
292+
__module_component: null,
293+
}: $FlowFixMe)
294+
} // intentionally all null
295+
props={otherProps}
296+
fallback={(<Fallback />: $FlowFixMe)}
297+
/>,
298+
);
299+
});
300+
}).rejects.toThrow(
295301
"MatchContainer: Invalid 'match' value, expected an object that has a '...SomeFragment' spread.",
296302
);
297303
});
298304

299-
it('throws if the match object is invalid (__fragmentOwner)', () => {
305+
it('throws if the match object is invalid (__fragmentOwner)', async () => {
300306
jest.spyOn(console, 'error').mockImplementationOnce(() => {});
301307
loader.mockReturnValue(React.memo((UserComponent: $FlowFixMe)));
302308
const otherProps = {otherProp: 'hello!'};
303309
const Fallback: $FlowFixMe = jest.fn(() => <div>fallback</div>);
304-
expect(() => {
305-
TestRenderer.create(
306-
<MatchContainer
307-
loader={loader}
308-
match={
309-
({
310-
__id: null,
311-
__fragments: null,
312-
__fragmentPropName: null,
313-
__fragmentOwner: 42, // not an object
314-
__module_component: null,
315-
}: $FlowFixMe)
316-
} // intentionally all null
317-
props={otherProps}
318-
fallback={(<Fallback />: $FlowFixMe)}
319-
/>,
320-
);
321-
}).toThrow(
310+
await expect(async () => {
311+
await TestRenderer.act(() => {
312+
TestRenderer.create(
313+
<MatchContainer
314+
loader={loader}
315+
match={
316+
({
317+
__id: null,
318+
__fragments: null,
319+
__fragmentPropName: null,
320+
__fragmentOwner: 42, // not an object
321+
__module_component: null,
322+
}: $FlowFixMe)
323+
} // intentionally all null
324+
props={otherProps}
325+
fallback={(<Fallback />: $FlowFixMe)}
326+
/>,
327+
);
328+
});
329+
}).rejects.toThrow(
322330
"MatchContainer: Invalid 'match' value, expected an object that has a '...SomeFragment' spread.",
323331
);
324332
});
325333

326-
it('throws if the match object is invalid (__fragmentPropName)', () => {
334+
it('throws if the match object is invalid (__fragmentPropName)', async () => {
327335
jest.spyOn(console, 'error').mockImplementationOnce(() => {});
328336
loader.mockReturnValue(React.memo((UserComponent: $FlowFixMe)));
329337
const otherProps = {otherProp: 'hello!'};
330338
const Fallback: $FlowFixMe = jest.fn(() => <div>fallback</div>);
331-
expect(() => {
332-
TestRenderer.create(
333-
<MatchContainer
334-
loader={loader}
335-
match={
336-
({
337-
__id: null,
338-
__fragments: null,
339-
__fragmentPropName: 42, // not a string
340-
__fragmentOwner: null,
341-
__module_component: null,
342-
}: $FlowFixMe)
343-
} // intentionally all null
344-
props={otherProps}
345-
fallback={(<Fallback />: $FlowFixMe)}
346-
/>,
347-
);
348-
}).toThrow(
339+
await expect(async () => {
340+
await TestRenderer.act(() => {
341+
TestRenderer.create(
342+
<MatchContainer
343+
loader={loader}
344+
match={
345+
({
346+
__id: null,
347+
__fragments: null,
348+
__fragmentPropName: 42, // not a string
349+
__fragmentOwner: null,
350+
__module_component: null,
351+
}: $FlowFixMe)
352+
} // intentionally all null
353+
props={otherProps}
354+
fallback={(<Fallback />: $FlowFixMe)}
355+
/>,
356+
);
357+
});
358+
}).rejects.toThrow(
349359
"MatchContainer: Invalid 'match' value, expected an object that has a '...SomeFragment' spread.",
350360
);
351361
});

0 commit comments

Comments
 (0)