diff --git a/packages/react-reconciler/src/ReactFiberSyncTaskQueue.js b/packages/react-reconciler/src/ReactFiberSyncTaskQueue.js index 273de09874789..0ba06495a454f 100644 --- a/packages/react-reconciler/src/ReactFiberSyncTaskQueue.js +++ b/packages/react-reconciler/src/ReactFiberSyncTaskQueue.js @@ -52,37 +52,63 @@ export function flushSyncCallbacks(): null { if (!isFlushingSyncQueue && syncQueue !== null) { // Prevent re-entrance. isFlushingSyncQueue = true; - let i = 0; + + // Set the event priority to discrete + // TODO: Is this necessary anymore? The only user code that runs in this + // queue is in the render or commit phases, which already set the + // event priority. Should be able to remove. const previousUpdatePriority = getCurrentUpdatePriority(); - try { - const isSync = true; - const queue = syncQueue; - // TODO: Is this necessary anymore? The only user code that runs in this - // queue is in the render or commit phases. - setCurrentUpdatePriority(DiscreteEventPriority); + setCurrentUpdatePriority(DiscreteEventPriority); + + let errors: Array | null = null; + + const queue = syncQueue; + // $FlowFixMe[incompatible-use] found when upgrading Flow + for (let i = 0; i < queue.length; i++) { // $FlowFixMe[incompatible-use] found when upgrading Flow - for (; i < queue.length; i++) { - // $FlowFixMe[incompatible-use] found when upgrading Flow - let callback: SchedulerCallback = queue[i]; + let callback: SchedulerCallback = queue[i]; + try { do { + const isSync = true; // $FlowFixMe[incompatible-type] we bail out when we get a null callback = callback(isSync); } while (callback !== null); + } catch (error) { + // Collect errors so we can rethrow them at the end + if (errors === null) { + errors = [error]; + } else { + errors.push(error); + } } - syncQueue = null; - includesLegacySyncCallbacks = false; - } catch (error) { - // If something throws, leave the remaining callbacks on the queue. - if (syncQueue !== null) { - syncQueue = syncQueue.slice(i + 1); + } + + syncQueue = null; + includesLegacySyncCallbacks = false; + setCurrentUpdatePriority(previousUpdatePriority); + isFlushingSyncQueue = false; + + if (errors !== null) { + if (errors.length > 1) { + if (typeof AggregateError === 'function') { + // eslint-disable-next-line no-undef + throw new AggregateError(errors); + } else { + const error = errors[0]; + for (let i = 1; i < errors.length; i++) { + const nextError = errors[i]; + scheduleCallback(ImmediatePriority, () => { + throw nextError; + }); + } + throw error; + } + } else { + const error = errors[0]; + throw error; } - // Resume flushing in the next tick - scheduleCallback(ImmediatePriority, flushSyncCallbacks); - throw error; - } finally { - setCurrentUpdatePriority(previousUpdatePriority); - isFlushingSyncQueue = false; } } + return null; } diff --git a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js index 6ed8d95013947..e2d9ba76660f9 100644 --- a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js @@ -248,4 +248,50 @@ describe('ReactFlushSync', () => { // Now the effects have fired. assertLog(['Effect']); }); + + test('completely exhausts synchronous work queue even if something throws', async () => { + function Throws({error}) { + throw error; + } + + const root1 = ReactNoop.createRoot(); + const root2 = ReactNoop.createRoot(); + const root3 = ReactNoop.createRoot(); + + await act(async () => { + root1.render(); + root2.render(); + root3.render(); + }); + assertLog(['Hi', 'Andrew', '!']); + + const aahh = new Error('AAHH!'); + const nooo = new Error('Noooooooooo!'); + + let error; + try { + ReactNoop.flushSync(() => { + root1.render(); + root2.render(); + root3.render(); + }); + } catch (e) { + error = e; + } + + // The update to root 3 should have finished synchronously, even though the + // earlier updates errored. + assertLog(['aww']); + // Roots 1 and 2 were unmounted. + expect(root1).toMatchRenderedOutput(null); + expect(root2).toMatchRenderedOutput(null); + expect(root3).toMatchRenderedOutput('aww'); + + // Because there were multiple errors, React threw an AggregateError. + // eslint-disable-next-line no-undef + expect(error).toBeInstanceOf(AggregateError); + expect(error.errors.length).toBe(2); + expect(error.errors[0]).toBe(aahh); + expect(error.errors[1]).toBe(nooo); + }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactFlushSyncNoAggregateError-test.js b/packages/react-reconciler/src/__tests__/ReactFlushSyncNoAggregateError-test.js new file mode 100644 index 0000000000000..6cef94bbd35f5 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactFlushSyncNoAggregateError-test.js @@ -0,0 +1,79 @@ +let React; +let ReactNoop; +let Scheduler; +let act; +let assertLog; +let waitForThrow; + +// TODO: Migrate tests to React DOM instead of React Noop + +describe('ReactFlushSync (AggregateError not available)', () => { + beforeEach(() => { + jest.resetModules(); + + global.AggregateError = undefined; + + React = require('react'); + ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); + act = require('internal-test-utils').act; + + const InternalTestUtils = require('internal-test-utils'); + assertLog = InternalTestUtils.assertLog; + waitForThrow = InternalTestUtils.waitForThrow; + }); + + function Text({text}) { + Scheduler.log(text); + return text; + } + + test('completely exhausts synchronous work queue even if something throws', async () => { + function Throws({error}) { + throw error; + } + + const root1 = ReactNoop.createRoot(); + const root2 = ReactNoop.createRoot(); + const root3 = ReactNoop.createRoot(); + + await act(async () => { + root1.render(); + root2.render(); + root3.render(); + }); + assertLog(['Hi', 'Andrew', '!']); + + const aahh = new Error('AAHH!'); + const nooo = new Error('Noooooooooo!'); + + let error; + try { + ReactNoop.flushSync(() => { + root1.render(); + root2.render(); + root3.render(); + }); + } catch (e) { + error = e; + } + + // The update to root 3 should have finished synchronously, even though the + // earlier updates errored. + assertLog(['aww']); + // Roots 1 and 2 were unmounted. + expect(root1).toMatchRenderedOutput(null); + expect(root2).toMatchRenderedOutput(null); + expect(root3).toMatchRenderedOutput('aww'); + + // In modern environments, React would throw an AggregateError. Because + // AggregateError is not available, React throws the first error, then + // throws the remaining errors in separate tasks. + expect(error).toBe(aahh); + // TODO: Currently the remaining error is rethrown in an Immediate Scheduler + // task, but this may change to a timer or microtask in the future. The + // exact mechanism is an implementation detail; they just need to be logged + // in the order the occurred. + await waitForThrow(nooo); + }); +}); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index 285274fca0781..e97476b71b580 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -1367,17 +1367,14 @@ describe('ReactIncrementalErrorHandling', () => { ); await waitForAll([]); - expect(() => { - expect(() => { - ReactNoop.flushSync(() => { - inst.setState({fail: true}); - }); - }).toThrow('Hello.'); - - // The unmount is queued in a microtask. In order to capture the error - // that occurs during unmount, we can flush it early with `flushSync`. - ReactNoop.flushSync(); - }).toThrow('One does not simply unmount me.'); + let aggregateError; + try { + ReactNoop.flushSync(() => { + inst.setState({fail: true}); + }); + } catch (e) { + aggregateError = e; + } assertLog([ // Attempt to clean up. @@ -1387,6 +1384,12 @@ describe('ReactIncrementalErrorHandling', () => { 'BrokenRenderAndUnmount componentWillUnmount', ]); expect(ReactNoop).toMatchRenderedOutput(null); + + // React threw both errors as a single AggregateError + const errors = aggregateError.errors; + expect(errors.length).toBe(2); + expect(errors[0].message).toBe('Hello.'); + expect(errors[1].message).toBe('One does not simply unmount me.'); }); it('does not interrupt unmounting if detaching a ref throws', async () => { diff --git a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js index 44f58434c2b47..32118f5befede 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js @@ -31,18 +31,23 @@ describe('ReactTestRenderer', () => { it('should warn if used to render a ReactDOM portal', () => { const container = document.createElement('div'); expect(() => { + let error; try { ReactTestRenderer.create(ReactDOM.createPortal('foo', container)); } catch (e) { - // TODO: After the update throws, a subsequent render is scheduled to - // unmount the whole tree. This update also causes an error, and this - // happens in a separate task. Flush this error now and capture it, to - // prevent it from firing asynchronously and causing the Jest test - // to fail. - expect(() => Scheduler.unstable_flushAll()).toThrow( - '.children.indexOf is not a function', - ); + error = e; } + // After the update throws, a subsequent render is scheduled to + // unmount the whole tree. This update also causes an error, so React + // throws an AggregateError. + const errors = error.errors; + expect(errors.length).toBe(2); + expect(errors[0].message.includes('indexOf is not a function')).toBe( + true, + ); + expect(errors[1].message.includes('indexOf is not a function')).toBe( + true, + ); }).toErrorDev('An invalid container has been provided.', { withoutStack: true, });