From ac3ca097aeecae8fe3ec7f9b286307a923676518 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Tue, 5 Nov 2024 16:58:16 -0500 Subject: [PATCH 1/6] Fix continuation bug --- packages/react-reconciler/src/ReactFiberRootScheduler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js index 9f267e8345e39..9eb2a327608c5 100644 --- a/packages/react-reconciler/src/ReactFiberRootScheduler.js +++ b/packages/react-reconciler/src/ReactFiberRootScheduler.js @@ -470,7 +470,7 @@ function performWorkOnRootViaSchedulerTask( // only safe to do because we know we're at the end of the browser task. // So although it's not an actual microtask, it might as well be. scheduleTaskForRootDuringMicrotask(root, now()); - if (root.callbackNode === originalCallbackNode) { + if (root.callbackNode != null && root.callbackNode === originalCallbackNode) { // The task node scheduled for this root is the same one that's // currently executed. Need to return a continuation. return performWorkOnRootViaSchedulerTask.bind(null, root); From 80d3cdfa32d08ce65a1079c8ab4fbe96cc3600ea Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Mon, 11 Nov 2024 15:38:03 -0500 Subject: [PATCH 2/6] Rename ActivitySuspense to ActivityLegacySuspense --- .../{ActivitySuspense-test.js => ActivityLegacySuspense-test.js} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename packages/react-reconciler/src/__tests__/{ActivitySuspense-test.js => ActivityLegacySuspense-test.js} (100%) diff --git a/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js b/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js similarity index 100% rename from packages/react-reconciler/src/__tests__/ActivitySuspense-test.js rename to packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js From 9e793a99fb8a50c96dd533ac8e1f2e5b45a3d112 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Mon, 11 Nov 2024 16:00:22 -0500 Subject: [PATCH 3/6] Add tests Co-authored-by: Joe Savona --- .../__tests__/ActivityLegacySuspense-test.js | 50 ++ .../src/__tests__/ActivitySuspense-test.js | 612 ++++++++++++++++++ 2 files changed, 662 insertions(+) create mode 100644 packages/react-reconciler/src/__tests__/ActivitySuspense-test.js diff --git a/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js b/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js index d02ae70e523ca..ddeda70f77fc1 100644 --- a/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js +++ b/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js @@ -1,3 +1,5 @@ +import {enableSiblingPrerendering} from 'shared/ReactFeatureFlags'; + let React; let ReactNoop; let Scheduler; @@ -174,6 +176,54 @@ describe('Activity Suspense', () => { ); }); + // @gate enableActivity + test('suspend, resolve, hide (forces refresh), suspend, reveal', async () => { + const root = ReactNoop.createRoot(); + + let setMode; + function Container({text}) { + const [mode, _setMode] = React.useState('visible'); + setMode = _setMode; + useEffect(() => { + return () => { + Scheduler.log(`Clear [${text}]`); + textCache.delete(text); + }; + }); + return ( + //$FlowFixMe + + + + + + ); + } + + await act(() => { + root.render(); + }); + assertLog([ + 'Suspend! [hello]', + ...(gate(flags => flags.enableSiblingPrerendering) + ? ['Suspend! [hello]'] + : []), + ]); + expect(root).toMatchRenderedOutput('Loading'); + + await act(async () => { + await resolveText('hello'); + }); + assertLog(['hello']); + expect(root).toMatchRenderedOutput('hello'); + + await act(async () => { + setMode('hidden'); + }); + assertLog(['Clear [hello]', 'Suspend! [hello]']); + expect(root).toMatchRenderedOutput(''); + }); + // @gate enableActivity test("suspending inside currently hidden tree that's switching to visible", async () => { const root = ReactNoop.createRoot(); diff --git a/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js b/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js new file mode 100644 index 0000000000000..3679ddd5e8296 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js @@ -0,0 +1,612 @@ +let React; +let ReactNoop; +let Scheduler; +let act; +let LegacyHidden; +let Activity; +let Suspense; +let useState; +let useEffect; +let startTransition; +let textCache; +let waitFor; +let waitForPaint; +let assertLog; +let use; + +describe('Activity Suspense', () => { + beforeEach(() => { + jest.resetModules(); + + React = require('react'); + ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); + act = require('internal-test-utils').act; + LegacyHidden = React.unstable_LegacyHidden; + Activity = React.unstable_Activity; + Suspense = React.Suspense; + useState = React.useState; + useEffect = React.useEffect; + startTransition = React.startTransition; + use = React.use; + + const InternalTestUtils = require('internal-test-utils'); + waitFor = InternalTestUtils.waitFor; + waitForPaint = InternalTestUtils.waitForPaint; + assertLog = InternalTestUtils.assertLog; + + textCache = new Map(); + }); + + function resolveText(text) { + const record = textCache.get(text); + if (record === undefined) { + const newRecord = { + status: 'resolved', + value: text, + }; + textCache.set(text, newRecord); + } else if (record.status === 'pending') { + const resolve = record.resolve; + record.status = 'resolved'; + record.value = text; + resolve(); + } + } + + function readText(text) { + const record = textCache.get(text); + if (record !== undefined) { + switch (record.status) { + case 'pending': + Scheduler.log(`Suspend! [${text}]`); + return use(record.value); + case 'rejected': + throw record.value; + case 'resolved': + return record.value; + } + } else { + Scheduler.log(`Suspend! [${text}]`); + let resolve; + const promise = new Promise(_resolve => { + resolve = _resolve; + }); + + const newRecord = { + status: 'pending', + value: promise, + resolve, + }; + textCache.set(text, newRecord); + + return use(promise); + } + } + + function Text({text}) { + Scheduler.log(text); + return text; + } + + function AsyncText({text}) { + readText(text); + Scheduler.log(text); + return text; + } + + // @gate enableActivity + it('basic example of suspending inside hidden tree', async () => { + const root = ReactNoop.createRoot(); + + function App() { + return ( + }> + + + + + + + + + + ); + } + + // The hidden tree hasn't finished loading, but we should still be able to + // show the surrounding contents. The outer Suspense boundary + // isn't affected. + await act(() => { + root.render(); + }); + assertLog(['Visible', 'Suspend! [Hidden]']); + expect(root).toMatchRenderedOutput(Visible); + + // When the data resolves, we should be able to finish prerendering + // the hidden tree. + await act(async () => { + await resolveText('Hidden'); + }); + assertLog(['Hidden']); + expect(root).toMatchRenderedOutput( + <> + Visible + + , + ); + }); + + // @gate enableLegacyHidden + test('LegacyHidden does not handle suspense', async () => { + const root = ReactNoop.createRoot(); + + function App() { + return ( + }> + + + + + + + + + + ); + } + + // Unlike Activity, LegacyHidden never captures if something suspends + await act(() => { + root.render(); + }); + assertLog(['Visible', 'Suspend! [Hidden]', 'Loading...']); + // Nearest Suspense boundary switches to a fallback even though the + // suspended content is hidden. + expect(root).toMatchRenderedOutput( + <> + + Loading... + , + ); + }); + + // @gate enableActivity + test('Regression: Suspending on hide should not infinte loop.', async () => { + // This regression only repros in public act. + global.IS_REACT_ACT_ENVIRONMENT = true; + const root = ReactNoop.createRoot(); + + let setMode; + function Container({text}) { + const [mode, _setMode] = React.useState('visible'); + setMode = _setMode; + useEffect(() => { + return () => { + Scheduler.log(`Clear [${text}]`); + textCache.delete(text); + }; + }); + return ( + //$FlowFixMe + + + + + + ); + } + + await React.act(() => { + root.render(); + }); + assertLog([ + 'Suspend! [hello]', + ...(gate(flags => flags.enableSiblingPrerendering) + ? ['Suspend! [hello]'] + : []), + ]); + expect(root).toMatchRenderedOutput('Loading'); + + await React.act(async () => { + await resolveText('hello'); + }); + assertLog(['hello']); + expect(root).toMatchRenderedOutput('hello'); + + await React.act(() => { + setMode('hidden'); + }); + assertLog(['Clear [hello]', 'Suspend! [hello]']); + expect(root).toMatchRenderedOutput(''); + }); + + // @gate enableActivity + test("suspending inside currently hidden tree that's switching to visible", async () => { + const root = ReactNoop.createRoot(); + + function Details({open, children}) { + return ( + }> + + + + + {children} + + + ); + } + + // The hidden tree hasn't finished loading, but we should still be able to + // show the surrounding contents. It doesn't matter that there's no + // Suspense boundary because the unfinished content isn't visible. + await act(() => { + root.render( +
+ +
, + ); + }); + assertLog(['Closed', 'Suspend! [Async]']); + expect(root).toMatchRenderedOutput(Closed); + + // But when we switch the boundary from hidden to visible, it should + // now bubble to the nearest Suspense boundary. + await act(() => { + startTransition(() => { + root.render( +
+ +
, + ); + }); + }); + assertLog([ + 'Open', + 'Suspend! [Async]', + ...(gate(flags => flags.enableSiblingPrerendering) ? ['Loading...'] : []), + ]); + // It should suspend with delay to prevent the already-visible Suspense + // boundary from switching to a fallback + expect(root).toMatchRenderedOutput(Closed); + + // Resolve the data and finish rendering + await act(async () => { + await resolveText('Async'); + }); + assertLog([ + ...(gate(flags => flags.enableSiblingPrerendering) ? ['Open'] : []), + 'Async', + ]); + expect(root).toMatchRenderedOutput( + <> + Open + Async + , + ); + }); + + // @gate enableActivity + test("suspending inside currently visible tree that's switching to hidden", async () => { + const root = ReactNoop.createRoot(); + + function Details({open, children}) { + return ( + }> + + + + + {children} + + + ); + } + + // Initial mount. Nothing suspends + await act(() => { + root.render( +
+ +
, + ); + }); + assertLog(['Open', '(empty)']); + expect(root).toMatchRenderedOutput( + <> + Open + (empty) + , + ); + + // Update that suspends inside the currently visible tree + await act(() => { + startTransition(() => { + root.render( +
+ +
, + ); + }); + }); + assertLog([ + 'Open', + 'Suspend! [Async]', + ...(gate(flags => flags.enableSiblingPrerendering) ? ['Loading...'] : []), + ]); + // It should suspend with delay to prevent the already-visible Suspense + // boundary from switching to a fallback + expect(root).toMatchRenderedOutput( + <> + Open + (empty) + , + ); + + // Update that hides the suspended tree + await act(() => { + startTransition(() => { + root.render( +
+ +
, + ); + }); + }); + // Now the visible part of the tree can commit without being blocked + // by the suspended content, which is hidden. + assertLog(['Closed', 'Suspend! [Async]']); + expect(root).toMatchRenderedOutput( + <> + Closed + + , + ); + + // Resolve the data and finish rendering + await act(async () => { + await resolveText('Async'); + }); + assertLog(['Async']); + expect(root).toMatchRenderedOutput( + <> + Closed + + , + ); + }); + + // @gate enableActivity + test('update that suspends inside hidden tree', async () => { + let setText; + function Child() { + const [text, _setText] = useState('A'); + setText = _setText; + return ; + } + + function App({show}) { + return ( + + + + + + ); + } + + const root = ReactNoop.createRoot(); + resolveText('A'); + await act(() => { + root.render(); + }); + assertLog(['A']); + + await act(() => { + startTransition(() => { + setText('B'); + }); + }); + }); + + // @gate enableActivity + test('updates at multiple priorities that suspend inside hidden tree', async () => { + let setText; + let setStep; + function Child() { + const [text, _setText] = useState('A'); + setText = _setText; + + const [step, _setStep] = useState(0); + setStep = _setStep; + + return ; + } + + function App({show}) { + return ( + + + + + + ); + } + + const root = ReactNoop.createRoot(); + resolveText('A0'); + await act(() => { + root.render(); + }); + assertLog(['A0']); + expect(root).toMatchRenderedOutput(); + + await act(() => { + React.startTransition(() => { + setStep(1); + }); + ReactNoop.flushSync(() => { + setText('B'); + }); + }); + assertLog([ + // The high priority render suspends again + 'Suspend! [B0]', + // There's still pending work in another lane, so we should attempt + // that, too. + 'Suspend! [B1]', + ]); + expect(root).toMatchRenderedOutput(); + + // Resolve the data and finish rendering + await act(() => { + resolveText('B1'); + }); + assertLog(['B1']); + expect(root).toMatchRenderedOutput(); + }); + + // @gate enableActivity + test('detect updates to a hidden tree during a concurrent event', async () => { + // This is a pretty complex test case. It relates to how we detect if an + // update is made to a hidden tree: when scheduling the update, we walk up + // the fiber return path to see if any of the parents is a hidden Activity + // component. This doesn't work if there's already a render in progress, + // because the tree might be about to flip to hidden. To avoid a data race, + // queue updates atomically: wait to queue the update until after the + // current render has finished. + + let setInner; + function Child({outer}) { + const [inner, _setInner] = useState(0); + setInner = _setInner; + + useEffect(() => { + // Inner and outer values are always updated simultaneously, so they + // should always be consistent. + if (inner !== outer) { + Scheduler.log('Tearing! Inner and outer are inconsistent!'); + } else { + Scheduler.log('Inner and outer are consistent'); + } + }, [inner, outer]); + + return ; + } + + let setOuter; + function App({show}) { + const [outer, _setOuter] = useState(0); + setOuter = _setOuter; + return ( + <> + + + + + + + + + }> + + + + + + ); + } + + // Render a hidden tree + const root = ReactNoop.createRoot(); + resolveText('Async: 0'); + await act(() => { + root.render(); + }); + assertLog([ + 'Inner: 0', + 'Outer: 0', + 'Sibling: 0', + 'Inner and outer are consistent', + ]); + expect(root).toMatchRenderedOutput( + <> + Inner: 0 + Outer: 0 + Sibling: 0 + , + ); + + await act(async () => { + // Update a value both inside and outside the hidden tree. These values + // must always be consistent. + startTransition(() => { + setOuter(1); + setInner(1); + // In the same render, also hide the offscreen tree. + root.render(); + }); + + await waitFor([ + // The outer update will commit, but the inner update is deferred until + // a later render. + 'Outer: 1', + ]); + + // Assert that we haven't committed quite yet + expect(root).toMatchRenderedOutput( + <> + Inner: 0 + Outer: 0 + Sibling: 0 + , + ); + + // Before the tree commits, schedule a concurrent event. The inner update + // is to a tree that's just about to be hidden. + startTransition(() => { + setOuter(2); + setInner(2); + }); + + // Finish rendering and commit the in-progress render. + await waitForPaint(['Sibling: 1']); + expect(root).toMatchRenderedOutput( + <> + + Outer: 1 + Sibling: 1 + , + ); + + // Now reveal the hidden tree at high priority. + ReactNoop.flushSync(() => { + root.render(); + }); + assertLog([ + // There are two pending updates on Inner, but only the first one + // is processed, even though they share the same lane. If the second + // update were erroneously processed, then Inner would be inconsistent + // with Outer. + 'Inner: 1', + 'Outer: 1', + 'Sibling: 1', + 'Inner and outer are consistent', + ]); + }); + assertLog([ + 'Inner: 2', + 'Outer: 2', + 'Sibling: 2', + 'Inner and outer are consistent', + ]); + expect(root).toMatchRenderedOutput( + <> + Inner: 2 + Outer: 2 + Sibling: 2 + , + ); + }); +}); From 831ba5eddf1e546b0493c441185cc2959bc0ad37 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Mon, 11 Nov 2024 16:03:28 -0500 Subject: [PATCH 4/6] Fix lint --- .../src/__tests__/ActivityLegacySuspense-test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js b/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js index ddeda70f77fc1..9b12445226dbd 100644 --- a/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js +++ b/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js @@ -1,5 +1,3 @@ -import {enableSiblingPrerendering} from 'shared/ReactFeatureFlags'; - let React; let ReactNoop; let Scheduler; From 547279521175b9d7af74179355fad3dd08ba221e Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Mon, 11 Nov 2024 16:07:45 -0500 Subject: [PATCH 5/6] Update tests --- .../src/__tests__/ActivityLegacySuspense-test.js | 10 ++++++---- .../src/__tests__/ActivitySuspense-test.js | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js b/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js index 9b12445226dbd..85a8ca11e3e09 100644 --- a/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js +++ b/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js @@ -175,7 +175,9 @@ describe('Activity Suspense', () => { }); // @gate enableActivity - test('suspend, resolve, hide (forces refresh), suspend, reveal', async () => { + test('Regression: Suspending on hide should not infinite loop.', async () => { + // This regression only repros in public act. + global.IS_REACT_ACT_ENVIRONMENT = true; const root = ReactNoop.createRoot(); let setMode; @@ -198,7 +200,7 @@ describe('Activity Suspense', () => { ); } - await act(() => { + await React.act(() => { root.render(); }); assertLog([ @@ -209,13 +211,13 @@ describe('Activity Suspense', () => { ]); expect(root).toMatchRenderedOutput('Loading'); - await act(async () => { + await React.act(async () => { await resolveText('hello'); }); assertLog(['hello']); expect(root).toMatchRenderedOutput('hello'); - await act(async () => { + await React.act(async () => { setMode('hidden'); }); assertLog(['Clear [hello]', 'Suspend! [hello]']); diff --git a/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js b/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js index 3679ddd5e8296..9bca757b5a595 100644 --- a/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js +++ b/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js @@ -172,7 +172,7 @@ describe('Activity Suspense', () => { }); // @gate enableActivity - test('Regression: Suspending on hide should not infinte loop.', async () => { + test('Regression: Suspending on hide should not infinite loop.', async () => { // This regression only repros in public act. global.IS_REACT_ACT_ENVIRONMENT = true; const root = ReactNoop.createRoot(); From 4696b45cb662ef28f7d108ae87cbd45d263ebb95 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Mon, 11 Nov 2024 16:57:06 -0500 Subject: [PATCH 6/6] Gate public act tests to __DEV__ --- .../src/__tests__/ActivityLegacySuspense-test.js | 2 +- .../react-reconciler/src/__tests__/ActivitySuspense-test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js b/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js index 85a8ca11e3e09..424594cc35e80 100644 --- a/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js +++ b/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js @@ -174,7 +174,7 @@ describe('Activity Suspense', () => { ); }); - // @gate enableActivity + // @gate __DEV__ && enableActivity test('Regression: Suspending on hide should not infinite loop.', async () => { // This regression only repros in public act. global.IS_REACT_ACT_ENVIRONMENT = true; diff --git a/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js b/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js index 9bca757b5a595..2fb35df4574ad 100644 --- a/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js +++ b/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js @@ -171,7 +171,7 @@ describe('Activity Suspense', () => { ); }); - // @gate enableActivity + // @gate __DEV__ && enableActivity test('Regression: Suspending on hide should not infinite loop.', async () => { // This regression only repros in public act. global.IS_REACT_ACT_ENVIRONMENT = true;