From 8528f6ac78ec7730ede6f707ef46acbfd1ea612b Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 8 Apr 2020 00:46:22 +0100 Subject: [PATCH 1/2] Add repro case for #18486 --- ...tSuspenseWithNoopRenderer-test.internal.js | 133 ++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 966fc653ca1..3728e4c4b9f 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -3645,4 +3645,137 @@ describe('ReactSuspenseWithNoopRenderer', () => { , ); }); + + // Regression: https://github.com/facebook/react/issues/18486 + it.experimental( + 'does not get stuck in pending state with render phase updates', + async () => { + let setTextWithTransition; + + function App() { + const [startTransition, isPending] = React.useTransition({ + timeoutMs: 30000, + }); + const [text, setText] = React.useState(''); + const [mirror, setMirror] = React.useState(''); + + if (text !== mirror) { + // Render phase update was needed to repro the bug. + setMirror(text); + } + + if (text !== '') { + // Inlined so that it's in the same component. + try { + Scheduler.unstable_yieldValue(text); + readText(text); + } catch (e) { + if (e && typeof e.then === 'function') { + Scheduler.unstable_yieldValue('Suspend! [' + text + ']'); + } + throw e; + } + } + + setTextWithTransition = value => { + startTransition(() => { + setText(value); + }); + }; + + return ( + <> + {isPending ? : null} + + + ); + } + + function Root() { + return ( + }> + + + ); + } + + const root = ReactNoop.createRoot(); + await ReactNoop.act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['']); + expect(root).toMatchRenderedOutput(); + + // Update to "a". That will suspend. + await ReactNoop.act(async () => { + setTextWithTransition('a'); + // Let it expire. This is important for the repro. + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYield([ + 'Pending...', + '', + 'a', + 'Suspend! [a]', + 'Loading...', + ]); + }); + expect(Scheduler).toHaveYielded([]); + expect(root).toMatchRenderedOutput( + <> + + + , + ); + + // Update to "b". That will suspend, too. + await ReactNoop.act(async () => { + setTextWithTransition('b'); + expect(Scheduler).toFlushAndYield([ + // Neither is resolved yet. + 'a', + 'Suspend! [a]', + 'Loading...', + 'b', + 'Suspend! [b]', + 'Loading...', + ]); + }); + expect(Scheduler).toHaveYielded([]); + expect(root).toMatchRenderedOutput( + <> + + + , + ); + + // Resolve "a". But "b" is still pending. + await ReactNoop.act(async () => { + await resolveText('a'); + }); + expect(Scheduler).toHaveYielded([ + 'Promise resolved [a]', + 'a', + 'a', + 'Pending...', + 'a', + ]); + expect(root).toMatchRenderedOutput( + <> + + + , + ); + + // Resolve "b". This should remove the pending state. + await ReactNoop.act(async () => { + await resolveText('b'); + }); + expect(Scheduler).toHaveYielded([ + 'Promise resolved [b]', + // TODO: "b" + ]); + // The bug was that the pending state got stuck forever. + expect(root).toMatchRenderedOutput(); + }, + ); }); From 3236038c55de5fc4edda958b87d8d9d301a68c9e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 8 Apr 2020 01:38:32 +0100 Subject: [PATCH 2/2] Simplify repro case --- ...tSuspenseWithNoopRenderer-test.internal.js | 21 ++----------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 3728e4c4b9f..b07ebad475a 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -3664,19 +3664,6 @@ describe('ReactSuspenseWithNoopRenderer', () => { setMirror(text); } - if (text !== '') { - // Inlined so that it's in the same component. - try { - Scheduler.unstable_yieldValue(text); - readText(text); - } catch (e) { - if (e && typeof e.then === 'function') { - Scheduler.unstable_yieldValue('Suspend! [' + text + ']'); - } - throw e; - } - } - setTextWithTransition = value => { startTransition(() => { setText(value); @@ -3686,7 +3673,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { return ( <> {isPending ? : null} - + {text !== '' ? : } ); } @@ -3714,7 +3701,6 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(Scheduler).toFlushAndYield([ 'Pending...', '', - 'a', 'Suspend! [a]', 'Loading...', ]); @@ -3732,10 +3718,9 @@ describe('ReactSuspenseWithNoopRenderer', () => { setTextWithTransition('b'); expect(Scheduler).toFlushAndYield([ // Neither is resolved yet. - 'a', + 'Pending...', 'Suspend! [a]', 'Loading...', - 'b', 'Suspend! [b]', 'Loading...', ]); @@ -3754,8 +3739,6 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); expect(Scheduler).toHaveYielded([ 'Promise resolved [a]', - 'a', - 'a', 'Pending...', 'a', ]);