From 42814ae52eb50c59512e82021a306554d5c4ddba Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 4 Oct 2018 11:08:19 -0700 Subject: [PATCH 1/2] Restart from root if promise pings before end of render phase --- .../src/ReactFiberScheduler.js | 10 +++- .../__tests__/ReactSuspense-test.internal.js | 53 +++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 69fc983fb13..bfb2d09a1d8 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -111,7 +111,7 @@ import { computeAsyncExpiration, computeInteractiveExpiration, } from './ReactFiberExpirationTime'; -import {ConcurrentMode, ProfileMode} from './ReactTypeOfMode'; +import {ConcurrentMode, ProfileMode, NoContext} from './ReactTypeOfMode'; import {enqueueUpdate, resetCurrentlyProcessingQueue} from './ReactUpdateQueue'; import {createCapturedValue} from './ReactCapturedValue'; import { @@ -1597,6 +1597,14 @@ function retrySuspendedRoot( markPendingPriorityLevel(root, retryTime); } + if ((fiber.mode & ConcurrentMode) !== NoContext) { + if (root === nextRoot && nextRenderExpirationTime === suspendedTime) { + // Received a ping at the same priority level at which we're currently + // rendering. Restart from the root. + nextRoot = null; + } + } + scheduleWorkToRoot(fiber, retryTime); const rootExpirationTime = root.expirationTime; if (rootExpirationTime !== NoWork) { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 3d66b81c8b6..80bd4efc756 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -189,4 +189,57 @@ describe('ReactSuspense', () => { expect(root).toFlushAndYield(['B']); expect(root).toMatchRenderedOutput('AB'); }); + + it('interrupts current render if promise resolves before current render phase', () => { + let didResolve = false; + let listeners = []; + + const thenable = { + then(resolve) { + if (!didResolve) { + listeners.push(resolve); + } else { + resolve(); + } + }, + }; + + function resolveThenable() { + didResolve = true; + listeners.forEach(l => l()); + } + + function Async() { + if (!didResolve) { + ReactTestRenderer.unstable_yield('Suspend!'); + throw thenable; + } + ReactTestRenderer.unstable_yield('Async'); + return 'Async'; + } + + const root = ReactTestRenderer.create( + }> + + + , + { + unstable_isConcurrent: true, + }, + ); + + expect(root).toFlushAndYieldThrough(['Suspend!']); + + // The promise resolves before the current render phase has completed + resolveThenable(); + expect(ReactTestRenderer).toHaveYielded([]); + + // Start over from the root, instead of continuing. + expect(root).toFlushAndYield([ + // Async renders again *before* Sibling + 'Async', + 'Sibling', + ]); + expect(root).toMatchRenderedOutput('AsyncSibling'); + }); }); From b48edffacbc531e4e25a015afd9247064e985ae4 Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Thu, 4 Oct 2018 11:35:26 -0700 Subject: [PATCH 2/2] Test that placeholder resolves successfully even if fallback render is pending --- ...eactSuspenseWithNoopRenderer-test.internal.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 017817fd621..6259d798f09 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -649,6 +649,22 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('Async'), span('Sync')]); }); + it('resolves successfully even if fallback render is pending', async () => { + ReactNoop.render( + }> + + , + ); + expect(ReactNoop.flushNextYield()).toEqual(['Suspend! [Async]']); + await advanceTimers(1500); + expect(ReactNoop.expire(1500)).toEqual([]); + // Before we have a chance to flush, the promise resolves. + await advanceTimers(2000); + expect(ReactNoop.clearYields()).toEqual(['Promise resolved [Async]']); + expect(ReactNoop.flush()).toEqual(['Async']); + expect(ReactNoop.getChildren()).toEqual([span('Async')]); + }); + it('throws a helpful error when an update is suspends without a placeholder', () => { expect(() => { ReactNoop.flushSync(() =>