From e7b255341b059b4e2a109847395d0d0ba2633999 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 8 Sep 2020 23:55:23 -0500 Subject: [PATCH] Internal `act`: Flush timers at end of scope (#19788) If there are any suspended fallbacks at the end of the `act` scope, force them to display by running the pending timers (i.e. `setTimeout`). The public implementation of `act` achieves the same behavior with an extra check in the work loop (`shouldForceFlushFallbacks`). Since our internal `act` needs to work in both development and production, without additional runtime checks, we instead rely on Jest's mock timers. This doesn't not affect refresh transitions, which are meant to delay indefinitely, because in that case we exit the work loop without posting a timer. --- .../src/test-utils/ReactTestUtilsAct.js | 5 ++- .../src/createReactNoop.js | 5 ++- .../src/__tests__/ReactBlocks-test.js | 18 ++++++--- .../ReactSuspenseWithNoopRenderer-test.js | 39 +++++++------------ .../src/ReactTestRenderer.js | 5 ++- scripts/rollup/validate/eslintrc.cjs.js | 1 + scripts/rollup/validate/eslintrc.cjs2015.js | 1 + scripts/rollup/validate/eslintrc.fb.js | 3 ++ scripts/rollup/validate/eslintrc.rn.js | 3 ++ scripts/rollup/validate/eslintrc.umd.js | 3 ++ 10 files changed, 48 insertions(+), 35 deletions(-) diff --git a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js index 369981f18a074..77fd1779ee56a 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js +++ b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js @@ -126,8 +126,9 @@ export function unstable_concurrentAct(scope: () => Thenable | void) { } function flushActWork(resolve, reject) { - // TODO: Run timers to flush suspended fallbacks - // jest.runOnlyPendingTimers(); + // Flush suspended fallbacks + // $FlowFixMe: Flow doesn't know about global Jest object + jest.runOnlyPendingTimers(); enqueueTask(() => { try { const didFlushWork = Scheduler.unstable_flushAllWithoutAsserting(); diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index f04c5479daa19..386d040e90757 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -1175,8 +1175,9 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { } function flushActWork(resolve, reject) { - // TODO: Run timers to flush suspended fallbacks - // jest.runOnlyPendingTimers(); + // Flush suspended fallbacks + // $FlowFixMe: Flow doesn't know about global Jest object + jest.runOnlyPendingTimers(); enqueueTask(() => { try { const didFlushWork = Scheduler.unstable_flushAllWithoutAsserting(); diff --git a/packages/react-reconciler/src/__tests__/ReactBlocks-test.js b/packages/react-reconciler/src/__tests__/ReactBlocks-test.js index 0d28a3359967d..18971c18a0c4a 100644 --- a/packages/react-reconciler/src/__tests__/ReactBlocks-test.js +++ b/packages/react-reconciler/src/__tests__/ReactBlocks-test.js @@ -14,6 +14,7 @@ let useState; let Suspense; let block; let readString; +let resolvePromises; let Scheduler; describe('ReactBlocks', () => { @@ -28,15 +29,16 @@ describe('ReactBlocks', () => { useState = React.useState; Suspense = React.Suspense; const cache = new Map(); + let unresolved = []; readString = function(text) { let entry = cache.get(text); if (!entry) { entry = { promise: new Promise(resolve => { - setTimeout(() => { + unresolved.push(() => { entry.resolved = true; resolve(); - }, 100); + }); }), resolved: false, }; @@ -47,6 +49,12 @@ describe('ReactBlocks', () => { } return text; }; + + resolvePromises = () => { + const res = unresolved; + unresolved = []; + res.forEach(r => r()); + }; }); // @gate experimental @@ -144,7 +152,7 @@ describe('ReactBlocks', () => { expect(ReactNoop).toMatchRenderedOutput('Loading...'); await ReactNoop.act(async () => { - jest.advanceTimersByTime(1000); + resolvePromises(); }); expect(ReactNoop).toMatchRenderedOutput(Name: Sebastian); @@ -291,7 +299,7 @@ describe('ReactBlocks', () => { ReactNoop.render(); }); await ReactNoop.act(async () => { - jest.advanceTimersByTime(1000); + resolvePromises(); }); expect(ReactNoop).toMatchRenderedOutput(Name: Sebastian); }); @@ -336,7 +344,7 @@ describe('ReactBlocks', () => { }); await ReactNoop.act(async () => { _setSuspend(false); - jest.advanceTimersByTime(1000); + resolvePromises(); }); expect(ReactNoop).toMatchRenderedOutput(Sebastian); }); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js index 13d38e992c5e0..cbcdf8b4f6f57 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js @@ -575,7 +575,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { {shouldSuspend ? ( - + ) : ( )} @@ -2595,7 +2595,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { } return ( }> - + ); } @@ -2616,8 +2616,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); // Later we load the data. - Scheduler.unstable_advanceTime(5000); - await advanceTimers(5000); + await resolveText('A'); expect(Scheduler).toHaveYielded(['Promise resolved [A]']); expect(Scheduler).toFlushAndYield(['A']); expect(ReactNoop.getChildren()).toEqual([span('A')]); @@ -2635,8 +2634,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); // Later we load the data. - Scheduler.unstable_advanceTime(3000); - await advanceTimers(3000); + await resolveText('B'); expect(Scheduler).toHaveYielded(['Promise resolved [B]']); expect(Scheduler).toFlushAndYield(['B']); expect(ReactNoop.getChildren()).toEqual([span('B')]); @@ -2754,12 +2752,14 @@ describe('ReactSuspenseWithNoopRenderer', () => { ); }); + // TODO: This test is specifically about avoided commits that suspend for a + // JND. We may remove this behavior. it("suspended commit remains suspended even if there's another update at same expiration", async () => { // Regression test function App({text}) { return ( - + ); } @@ -2768,34 +2768,28 @@ describe('ReactSuspenseWithNoopRenderer', () => { await ReactNoop.act(async () => { root.render(); }); + expect(Scheduler).toHaveYielded(['Suspend! [Initial]']); // Resolve initial render await ReactNoop.act(async () => { - Scheduler.unstable_advanceTime(2000); - await advanceTimers(2000); + await resolveText('Initial'); }); - expect(Scheduler).toHaveYielded([ - 'Suspend! [Initial]', - 'Promise resolved [Initial]', - 'Initial', - ]); + expect(Scheduler).toHaveYielded(['Promise resolved [Initial]', 'Initial']); expect(root).toMatchRenderedOutput(); - // Update. Since showing a fallback would hide content that's already - // visible, it should suspend for a bit without committing. await ReactNoop.act(async () => { + // Update. Since showing a fallback would hide content that's already + // visible, it should suspend for a JND without committing. root.render(); - expect(Scheduler).toFlushAndYield(['Suspend! [First update]']); + // Should not display a fallback expect(root).toMatchRenderedOutput(); - }); - // Update again. This should also suspend for a bit. - await ReactNoop.act(async () => { + // Update again. This should also suspend for a JND. root.render(); - expect(Scheduler).toFlushAndYield(['Suspend! [Second update]']); + // Should not display a fallback expect(root).toMatchRenderedOutput(); }); @@ -3989,9 +3983,6 @@ describe('ReactSuspenseWithNoopRenderer', () => { await ReactNoop.act(async () => { root.render(); }); - // TODO: `act` should have already flushed the placeholder, so this - // runAllTimers call should be unnecessary. - jest.runAllTimers(); expect(Scheduler).toHaveYielded(['Suspend! [Async]', 'Loading...']); expect(root).toMatchRenderedOutput( <> diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index 616e38faaabae..899f4c6542886 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -684,8 +684,9 @@ function unstable_concurrentAct(scope: () => Thenable | void) { } function flushActWork(resolve, reject) { - // TODO: Run timers to flush suspended fallbacks - // jest.runOnlyPendingTimers(); + // Flush suspended fallbacks + // $FlowFixMe: Flow doesn't know about global Jest object + jest.runOnlyPendingTimers(); enqueueTask(() => { try { const didFlushWork = Scheduler.unstable_flushAllWithoutAsserting(); diff --git a/scripts/rollup/validate/eslintrc.cjs.js b/scripts/rollup/validate/eslintrc.cjs.js index 4eae89839ae16..9f1d74176a507 100644 --- a/scripts/rollup/validate/eslintrc.cjs.js +++ b/scripts/rollup/validate/eslintrc.cjs.js @@ -42,6 +42,7 @@ module.exports = { // jest expect: true, + jest: true, }, parserOptions: { ecmaVersion: 5, diff --git a/scripts/rollup/validate/eslintrc.cjs2015.js b/scripts/rollup/validate/eslintrc.cjs2015.js index 81383a59643e9..d13058a61cc1d 100644 --- a/scripts/rollup/validate/eslintrc.cjs2015.js +++ b/scripts/rollup/validate/eslintrc.cjs2015.js @@ -42,6 +42,7 @@ module.exports = { // jest expect: true, + jest: true, }, parserOptions: { ecmaVersion: 2015, diff --git a/scripts/rollup/validate/eslintrc.fb.js b/scripts/rollup/validate/eslintrc.fb.js index 9c9f6b780a935..36f4877efdb43 100644 --- a/scripts/rollup/validate/eslintrc.fb.js +++ b/scripts/rollup/validate/eslintrc.fb.js @@ -36,6 +36,9 @@ module.exports = { // Flight Uint8Array: true, Promise: true, + + // jest + jest: true, }, parserOptions: { ecmaVersion: 5, diff --git a/scripts/rollup/validate/eslintrc.rn.js b/scripts/rollup/validate/eslintrc.rn.js index 4fb8181ae7096..3e7027f3510e2 100644 --- a/scripts/rollup/validate/eslintrc.rn.js +++ b/scripts/rollup/validate/eslintrc.rn.js @@ -31,6 +31,9 @@ module.exports = { ArrayBuffer: true, TaskController: true, + + // jest + jest: true, }, parserOptions: { ecmaVersion: 5, diff --git a/scripts/rollup/validate/eslintrc.umd.js b/scripts/rollup/validate/eslintrc.umd.js index e3786ffd3b6c2..933c1c9d73496 100644 --- a/scripts/rollup/validate/eslintrc.umd.js +++ b/scripts/rollup/validate/eslintrc.umd.js @@ -43,6 +43,9 @@ module.exports = { // Flight Webpack __webpack_chunk_load__: true, __webpack_require__: true, + + // jest + jest: true, }, parserOptions: { ecmaVersion: 5,