diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.expect.md new file mode 100644 index 0000000000..f79df15d52 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.expect.md @@ -0,0 +1,89 @@ + +## Input + +```javascript +import {useRef} from 'react'; +import {Stringify} from 'shared-runtime'; + +/** + * Bug: we're currently filtering out `ref.current` dependencies in + * `propagateScopeDependencies:checkValidDependency`. This is incorrect. + * Instead, we should always take a dependency on ref values (the outer box) as + * they may be reactive. Pruning should be done in + * `pruneNonReactiveDependencies` + * + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) + *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
+ *
{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}
+ * Forget: + * (kind: ok) + *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
+ *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
+ */ +function Component({cond}) { + const ref1 = useRef(1); + const ref2 = useRef(2); + const ref = cond ? ref1 : ref2; + const cb = () => ref.current; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: true}], + sequentialRenders: [{cond: true}, {cond: false}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { useRef } from "react"; +import { Stringify } from "shared-runtime"; + +/** + * Bug: we're currently filtering out `ref.current` dependencies in + * `propagateScopeDependencies:checkValidDependency`. This is incorrect. + * Instead, we should always take a dependency on ref values (the outer box) as + * they may be reactive. Pruning should be done in + * `pruneNonReactiveDependencies` + * + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) + *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
+ *
{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}
+ * Forget: + * (kind: ok) + *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
+ *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
+ */ +function Component(t0) { + const $ = _c(1); + const { cond } = t0; + const ref1 = useRef(1); + const ref2 = useRef(2); + const ref = cond ? ref1 : ref2; + let t1; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + const cb = () => ref.current; + t1 = ; + $[0] = t1; + } else { + t1 = $[0]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ cond: true }], + sequentialRenders: [{ cond: true }, { cond: false }], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.tsx new file mode 100644 index 0000000000..a0115f2df3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-nonreactive-ref.tsx @@ -0,0 +1,33 @@ +import {useRef} from 'react'; +import {Stringify} from 'shared-runtime'; + +/** + * Bug: we're currently filtering out `ref.current` dependencies in + * `propagateScopeDependencies:checkValidDependency`. This is incorrect. + * Instead, we should always take a dependency on ref values (the outer box) as + * they may be reactive. Pruning should be done in + * `pruneNonReactiveDependencies` + * + * Found differences in evaluator results + * Non-forget (expected): + * (kind: ok) + *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
+ *
{"cb":{"kind":"Function","result":2},"shouldInvokeFns":true}
+ * Forget: + * (kind: ok) + *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
+ *
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
+ */ +function Component({cond}) { + const ref1 = useRef(1); + const ref2 = useRef(2); + const ref = cond ? ref1 : ref2; + const cb = () => ref.current; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{cond: true}], + sequentialRenders: [{cond: true}, {cond: false}], +}; diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 1971fe0d33..6c136f6310 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -484,6 +484,7 @@ const skipFilter = new Set([ 'bug-aliased-capture-mutate', 'bug-functiondecl-hoisting', 'bug-try-catch-maybe-null-dependency', + 'bug-nonreactive-ref', 'reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted', 'bug-invalid-phi-as-dependency', 'reduce-reactive-deps/bug-merge-uncond-optional-chain-and-cond', diff --git a/packages/react-reconciler/src/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js index 39f6f466ed..1f529d9e4d 100644 --- a/packages/react-reconciler/src/ReactFiberRootScheduler.js +++ b/packages/react-reconciler/src/ReactFiberRootScheduler.js @@ -482,7 +482,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); diff --git a/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js b/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js new file mode 100644 index 0000000000..424594cc35 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ActivityLegacySuspense-test.js @@ -0,0 +1,604 @@ +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; + +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; + + 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 thenable = record.value; + record.status = 'resolved'; + record.value = text; + thenable.pings.forEach(t => t()); + } + } + + function readText(text) { + const record = textCache.get(text); + if (record !== undefined) { + switch (record.status) { + case 'pending': + Scheduler.log(`Suspend! [${text}]`); + throw record.value; + case 'rejected': + throw record.value; + case 'resolved': + return record.value; + } + } else { + Scheduler.log(`Suspend! [${text}]`); + const thenable = { + pings: [], + then(resolve) { + if (newRecord.status === 'pending') { + thenable.pings.push(resolve); + } else { + Promise.resolve().then(() => resolve(newRecord.value)); + } + }, + }; + + const newRecord = { + status: 'pending', + value: thenable, + }; + textCache.set(text, newRecord); + + throw thenable; + } + } + + 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 __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; + 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(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(); + + 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]', '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(['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]', '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 + , + ); + }); +}); diff --git a/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js b/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js index d02ae70e52..2fb35df457 100644 --- a/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js +++ b/packages/react-reconciler/src/__tests__/ActivitySuspense-test.js @@ -12,6 +12,7 @@ let textCache; let waitFor; let waitForPaint; let assertLog; +let use; describe('Activity Suspense', () => { beforeEach(() => { @@ -27,6 +28,7 @@ describe('Activity Suspense', () => { useState = React.useState; useEffect = React.useEffect; startTransition = React.startTransition; + use = React.use; const InternalTestUtils = require('internal-test-utils'); waitFor = InternalTestUtils.waitFor; @@ -45,10 +47,10 @@ describe('Activity Suspense', () => { }; textCache.set(text, newRecord); } else if (record.status === 'pending') { - const thenable = record.value; + const resolve = record.resolve; record.status = 'resolved'; record.value = text; - thenable.pings.forEach(t => t()); + resolve(); } } @@ -58,7 +60,7 @@ describe('Activity Suspense', () => { switch (record.status) { case 'pending': Scheduler.log(`Suspend! [${text}]`); - throw record.value; + return use(record.value); case 'rejected': throw record.value; case 'resolved': @@ -66,24 +68,19 @@ describe('Activity Suspense', () => { } } else { Scheduler.log(`Suspend! [${text}]`); - const thenable = { - pings: [], - then(resolve) { - if (newRecord.status === 'pending') { - thenable.pings.push(resolve); - } else { - Promise.resolve().then(() => resolve(newRecord.value)); - } - }, - }; + let resolve; + const promise = new Promise(_resolve => { + resolve = _resolve; + }); const newRecord = { status: 'pending', - value: thenable, + value: promise, + resolve, }; textCache.set(text, newRecord); - throw thenable; + return use(promise); } } @@ -174,6 +171,56 @@ describe('Activity Suspense', () => { ); }); + // @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; + 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(); @@ -215,7 +262,11 @@ describe('Activity Suspense', () => { ); }); }); - assertLog(['Open', 'Suspend! [Async]', 'Loading...']); + 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); @@ -224,7 +275,10 @@ describe('Activity Suspense', () => { await act(async () => { await resolveText('Async'); }); - assertLog(['Open', 'Async']); + assertLog([ + ...(gate(flags => flags.enableSiblingPrerendering) ? ['Open'] : []), + 'Async', + ]); expect(root).toMatchRenderedOutput( <> Open @@ -276,7 +330,11 @@ describe('Activity Suspense', () => { ); }); }); - assertLog(['Open', 'Suspend! [Async]', 'Loading...']); + 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(