From 8ced545e3df95afab6fa35bc29f9320bafbcef26 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 18 Oct 2018 16:07:22 -0700 Subject: [PATCH] Suspense component does not capture if `fallback` is not defined (#13879) * Suspense component does not capture if `fallback` is not defined A missing fallback prop means the exception should propagate to the next parent (like a rethrow). That way a Suspense component can specify other props like maxDuration without needing to provide a fallback, too. Closes #13864 * Change order of checks --- .../src/ReactFiberUnwindWork.js | 6 +-- .../src/__tests__/ReactPure-test.internal.js | 18 +++---- .../__tests__/ReactSuspense-test.internal.js | 48 ++++++++++++++++++- ...tSuspenseWithNoopRenderer-test.internal.js | 44 ++++++++++------- 4 files changed, 87 insertions(+), 29 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index 2f0b87b89b..afec1bc821 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -205,9 +205,9 @@ function throwException( workInProgress = returnFiber; do { if (workInProgress.tag === SuspenseComponent) { - const state = workInProgress.memoizedState; - const didTimeout = state !== null && state.didTimeout; - if (!didTimeout) { + const fallback = workInProgress.memoizedProps.fallback; + const didTimeout = workInProgress.memoizedState; + if (!didTimeout && workInProgress.memoizedProps.fallback !== undefined) { // Found the nearest boundary. // If the boundary is not in concurrent mode, we should not suspend, and diff --git a/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js b/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js index 62787a3474..ca1bb36851 100644 --- a/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js @@ -57,11 +57,11 @@ describe('pure', () => { Counter = pure(Counter); ReactNoop.render( - + }> , ); - expect(ReactNoop.flush()).toEqual([]); + expect(ReactNoop.flush()).toEqual(['Loading...']); await Promise.resolve(); expect(ReactNoop.flush()).toEqual([0]); expect(ReactNoop.getChildren()).toEqual([span(0)]); @@ -107,7 +107,7 @@ describe('pure', () => { state = {count: 0}; render() { return ( - + }> @@ -118,7 +118,7 @@ describe('pure', () => { const parent = React.createRef(null); ReactNoop.render(); - expect(ReactNoop.flush()).toEqual([]); + expect(ReactNoop.flush()).toEqual(['Loading...']); await Promise.resolve(); expect(ReactNoop.flush()).toEqual(['Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); @@ -148,11 +148,11 @@ describe('pure', () => { }); ReactNoop.render( - + }> , ); - expect(ReactNoop.flush()).toEqual([]); + expect(ReactNoop.flush()).toEqual(['Loading...']); await Promise.resolve(); expect(ReactNoop.flush()).toEqual([0]); expect(ReactNoop.getChildren()).toEqual([span(0)]); @@ -204,7 +204,7 @@ describe('pure', () => { const divRef = React.createRef(); ReactNoop.render( - + }> , ); @@ -224,11 +224,11 @@ describe('pure', () => { const divRef2 = React.createRef(); ReactNoop.render( - + }> , ); - expect(ReactNoop.flush()).toEqual([]); + expect(ReactNoop.flush()).toEqual(['Loading...']); await Promise.resolve(); expect(ReactNoop.flush()).toEqual(['Text']); expect(divRef.current.type).toBe('div'); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 34818bd684..5ce5318412 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -106,7 +106,7 @@ describe('ReactSuspense', () => { function Foo() { ReactTestRenderer.unstable_yield('Foo'); return ( - + }> @@ -126,6 +126,7 @@ describe('ReactSuspense', () => { 'Suspend! [A]', // But we keep rendering the siblings 'B', + 'Loading...', ]); expect(root).toMatchRenderedOutput(null); @@ -271,4 +272,49 @@ describe('ReactSuspense', () => { expect(ReactTestRenderer).toHaveYielded(['Hi', 'Did mount: Hi']); expect(root).toMatchRenderedOutput('Hi'); }); + + it('only captures if `fallback` is defined', () => { + const root = ReactTestRenderer.create( + }> + + + + , + { + unstable_isConcurrent: true, + }, + ); + + expect(root).toFlushAndYield([ + 'Suspend! [Hi]', + // The outer fallback should be rendered, because the inner one does not + // have a `fallback` prop + 'Loading...', + ]); + jest.advanceTimersByTime(1000); + expect(ReactTestRenderer).toHaveYielded([]); + expect(root).toFlushAndYield([]); + expect(root).toMatchRenderedOutput('Loading...'); + + jest.advanceTimersByTime(5000); + expect(ReactTestRenderer).toHaveYielded(['Promise resolved [Hi]']); + expect(root).toFlushAndYield(['Hi']); + expect(root).toMatchRenderedOutput('Hi'); + }); + + it('throws if tree suspends and none of the Suspense ancestors have a fallback', () => { + const root = ReactTestRenderer.create( + + + , + { + unstable_isConcurrent: true, + }, + ); + + expect(root).toFlushAndThrow( + 'An update was suspended, but no placeholder UI was provided.', + ); + expect(ReactTestRenderer).toHaveYielded(['Suspend! [Hi]', 'Suspend! [Hi]']); + }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index de5f3662ff..3fbd5e4e77 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -104,7 +104,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { function Foo() { ReactNoop.yield('Foo'); return ( - + }> @@ -121,6 +121,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'Suspend! [A]', // But we keep rendering the siblings 'B', + 'Loading...', ]); expect(ReactNoop.getChildren()).toEqual([]); @@ -193,7 +194,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { it('continues rendering siblings after suspending', async () => { ReactNoop.render( - + }> @@ -201,7 +202,13 @@ describe('ReactSuspenseWithNoopRenderer', () => { , ); // B suspends. Continue rendering the remaining siblings. - expect(ReactNoop.flush()).toEqual(['A', 'Suspend! [B]', 'C', 'D']); + expect(ReactNoop.flush()).toEqual([ + 'A', + 'Suspend! [B]', + 'C', + 'D', + 'Loading...', + ]); // Did not commit yet. expect(ReactNoop.getChildren()).toEqual([]); @@ -243,7 +250,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { const errorBoundary = React.createRef(); function App() { return ( - + }> @@ -252,7 +259,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { } ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['Suspend! [Result]']); + expect(ReactNoop.flush()).toEqual(['Suspend! [Result]', 'Loading...']); expect(ReactNoop.getChildren()).toEqual([]); textResourceShouldFail = true; @@ -278,7 +285,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { errorBoundary.current.reset(); cache.invalidate(); - expect(ReactNoop.flush()).toEqual(['Suspend! [Result]']); + expect(ReactNoop.flush()).toEqual(['Suspend! [Result]', 'Loading...']); ReactNoop.expire(1000); await advanceTimers(1000); expect(ReactNoop.flush()).toEqual(['Promise resolved [Result]', 'Result']); @@ -356,7 +363,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { it('can update at a higher priority while in a suspended state', async () => { function App(props) { return ( - + }> @@ -376,6 +383,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'A', // Suspends 'Suspend! [2]', + 'Loading...', ]); // While we're still waiting for the low-pri update to complete, update the @@ -395,7 +403,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { it('keeps working on lower priority work after being pinged', async () => { function App(props) { return ( - + }> {props.showB && } @@ -403,13 +411,13 @@ describe('ReactSuspenseWithNoopRenderer', () => { } ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['Suspend! [A]']); + expect(ReactNoop.flush()).toEqual(['Suspend! [A]', 'Loading...']); expect(ReactNoop.getChildren()).toEqual([]); // Advance React's virtual time by enough to fall into a new async bucket. ReactNoop.expire(1200); ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['Suspend! [A]', 'B']); + expect(ReactNoop.flush()).toEqual(['Suspend! [A]', 'B', 'Loading...']); expect(ReactNoop.getChildren()).toEqual([]); await advanceTimers(0); @@ -676,13 +684,17 @@ describe('ReactSuspenseWithNoopRenderer', () => { it('a Suspense component correctly handles more than one suspended child', async () => { ReactNoop.render( - + }> , ); - expect(ReactNoop.expire(10000)).toEqual(['Suspend! [A]', 'Suspend! [B]']); - expect(ReactNoop.getChildren()).toEqual([]); + expect(ReactNoop.expire(10000)).toEqual([ + 'Suspend! [A]', + 'Suspend! [B]', + 'Loading...', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); await advanceTimers(100); @@ -722,7 +734,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { it('starts working on an update even if its priority falls between two suspended levels', async () => { function App(props) { return ( - + } maxDuration={10000}> {props.text === 'C' ? ( ) : ( @@ -735,7 +747,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { // Schedule an update ReactNoop.render(); // The update should suspend. - expect(ReactNoop.flush()).toEqual(['Suspend! [A]']); + expect(ReactNoop.flush()).toEqual(['Suspend! [A]', 'Loading...']); expect(ReactNoop.getChildren()).toEqual([]); // Advance time until right before it expires. This number may need to @@ -748,7 +760,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { // Schedule another low priority update. ReactNoop.render(); // This update should also suspend. - expect(ReactNoop.flush()).toEqual(['Suspend! [B]']); + expect(ReactNoop.flush()).toEqual(['Suspend! [B]', 'Loading...']); expect(ReactNoop.getChildren()).toEqual([]); // Schedule a high priority update. Its expiration time will fall between