From 04ee54cd128a48cb3fdac7256e1a45d6d9743d8c Mon Sep 17 00:00:00 2001 From: Ricky Date: Tue, 11 Nov 2025 12:47:56 -0500 Subject: [PATCH 1/3] [tests] add more portal activity tests (#35095) I copied some tests from [`Activity-test.js`](https://github.com/facebook/react/blob/1d68bce19c9409ed70604d1d16b70b68ce71dc4a/packages/react-reconciler/src/__tests__/Activity-test.js) and made them portal specific just to confirm my understanding of how Portals + Activity interact is correct. Seems good to include them. --- .../src/__tests__/ReactDOMActivity-test.js | 441 +++++++++++++++++- 1 file changed, 436 insertions(+), 5 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMActivity-test.js b/packages/react-dom/src/__tests__/ReactDOMActivity-test.js index e849ddc501d7e..ec2048ec434f9 100644 --- a/packages/react-dom/src/__tests__/ReactDOMActivity-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMActivity-test.js @@ -10,11 +10,17 @@ 'use strict'; let React; -let Activity; -let useState; let ReactDOM; let ReactDOMClient; +let Scheduler; let act; +let Activity; +let useState; +let useLayoutEffect; +let useEffect; +let LegacyHidden; +let assertLog; +let Suspense; describe('ReactDOMActivity', () => { let container; @@ -22,11 +28,19 @@ describe('ReactDOMActivity', () => { beforeEach(() => { jest.resetModules(); React = require('react'); + Scheduler = require('scheduler/unstable_mock'); Activity = React.Activity; useState = React.useState; + Suspense = React.Suspense; + useState = React.useState; + LegacyHidden = React.unstable_LegacyHidden; + useLayoutEffect = React.useLayoutEffect; + useEffect = React.useEffect; ReactDOM = require('react-dom'); ReactDOMClient = require('react-dom/client'); - act = require('internal-test-utils').act; + const InternalTestUtils = require('internal-test-utils'); + act = InternalTestUtils.act; + assertLog = InternalTestUtils.assertLog; container = document.createElement('div'); document.body.appendChild(container); }); @@ -35,6 +49,11 @@ describe('ReactDOMActivity', () => { document.body.removeChild(container); }); + function Text(props) { + Scheduler.log(props.text); + return {props.children}; + } + // @gate enableActivity it( 'hiding an Activity boundary also hides the direct children of any ' + @@ -53,7 +72,7 @@ describe('ReactDOMActivity', () => { ); } - function App({portalContents}) { + function App() { return (
@@ -99,7 +118,7 @@ describe('ReactDOMActivity', () => { ); } - function App({portalContents}) { + function App() { return (
@@ -131,4 +150,416 @@ describe('ReactDOMActivity', () => { ); }, ); + + // @gate enableActivity + it('hides new portals added to an already hidden tree', async () => { + function Child() { + return ; + } + + const portalContainer = document.createElement('div'); + + function Portal({children}) { + return
{ReactDOM.createPortal(children, portalContainer)}
; + } + + const root = ReactDOMClient.createRoot(container); + // Mount hidden tree. + await act(() => { + root.render( + + + , + ); + }); + assertLog(['Parent']); + expect(container.innerHTML).toBe( + '', + ); + expect(portalContainer.innerHTML).toBe(''); + + // Add a portal inside the hidden tree. + await act(() => { + root.render( + + + + + + , + ); + }); + assertLog(['Parent', 'Child']); + expect(container.innerHTML).toBe( + '
', + ); + expect(portalContainer.innerHTML).toBe( + '', + ); + + // Now reveal it. + await act(() => { + root.render( + + + + + + , + ); + }); + + assertLog(['Parent', 'Child']); + expect(container.innerHTML).toBe( + '
', + ); + expect(portalContainer.innerHTML).toBe( + '', + ); + }); + + // @gate enableActivity + it('hides new insertions inside an already hidden portal', async () => { + function Child({text}) { + useLayoutEffect(() => { + Scheduler.log(`Mount layout ${text}`); + return () => { + Scheduler.log(`Unmount layout ${text}`); + }; + }, [text]); + return ; + } + + const portalContainer = document.createElement('div'); + + function Portal({children}) { + return
{ReactDOM.createPortal(children, portalContainer)}
; + } + + const root = ReactDOMClient.createRoot(container); + // Mount hidden tree. + await act(() => { + root.render( + + + + + , + ); + }); + assertLog(['A']); + expect(container.innerHTML).toBe('
'); + expect(portalContainer.innerHTML).toBe( + '', + ); + + // Add a node inside the hidden portal. + await act(() => { + root.render( + + + + + + , + ); + }); + assertLog(['A', 'B']); + expect(container.innerHTML).toBe('
'); + expect(portalContainer.innerHTML).toBe( + '', + ); + + // Now reveal it. + await act(() => { + root.render( + + + + + + , + ); + }); + + assertLog(['A', 'B', 'Mount layout A', 'Mount layout B']); + expect(container.innerHTML).toBe('
'); + expect(portalContainer.innerHTML).toBe( + '', + ); + }); + + // @gate enableActivity + it('reveal an inner Suspense boundary without revealing an outer Activity on the same host child', async () => { + const promise = new Promise(() => {}); + + function Child({showInner}) { + useLayoutEffect(() => { + Scheduler.log('Mount layout'); + return () => { + Scheduler.log('Unmount layout'); + }; + }, []); + return ( + <> + {showInner ? null : promise} + + + ); + } + + const portalContainer = document.createElement('div'); + + function Portal({children}) { + return
{ReactDOM.createPortal(children, portalContainer)}
; + } + + const root = ReactDOMClient.createRoot(container); + + // Prerender the whole tree. + await act(() => { + root.render( + + + Loading}> + + + + , + ); + }); + + assertLog(['Child']); + expect(container.innerHTML).toBe('
'); + expect(portalContainer.innerHTML).toBe( + '', + ); + + // Re-suspend the inner. + await act(() => { + root.render( + + + Loading}> + + + + , + ); + }); + assertLog([]); + expect(container.innerHTML).toBe('
'); + expect(portalContainer.innerHTML).toBe( + 'Loading', + ); + + // Toggle to visible while suspended. + await act(() => { + root.render( + + + Loading}> + + + + , + ); + }); + assertLog([]); + expect(container.innerHTML).toBe('
'); + expect(portalContainer.innerHTML).toBe( + 'Loading', + ); + + // Now reveal. + await act(() => { + root.render( + + + Loading}> + + + + , + ); + }); + assertLog(['Child', 'Mount layout']); + expect(container.innerHTML).toBe('
'); + expect(portalContainer.innerHTML).toBe( + '', + ); + }); + + // @gate enableActivity + it('mounts/unmounts layout effects in portal when visibility changes (starting visible)', async () => { + function Child() { + useLayoutEffect(() => { + Scheduler.log('Mount layout'); + return () => { + Scheduler.log('Unmount layout'); + }; + }, []); + return ; + } + + const portalContainer = document.createElement('div'); + + function Portal({children}) { + return
{ReactDOM.createPortal(children, portalContainer)}
; + } + + const root = ReactDOMClient.createRoot(container); + // Mount visible tree. + await act(() => { + root.render( + + + + + , + ); + }); + assertLog(['Child', 'Mount layout']); + expect(container.innerHTML).toBe('
'); + expect(portalContainer.innerHTML).toBe(''); + + // Hide the tree. The layout effect is unmounted. + await act(() => { + root.render( + + + + + , + ); + }); + assertLog(['Unmount layout', 'Child']); + expect(container.innerHTML).toBe('
'); + expect(portalContainer.innerHTML).toBe( + '', + ); + }); + + // @gate enableActivity + it('mounts/unmounts layout effects in portal when visibility changes (starting hidden)', async () => { + function Child() { + useLayoutEffect(() => { + Scheduler.log('Mount layout'); + return () => { + Scheduler.log('Unmount layout'); + }; + }, []); + return ; + } + + const portalContainer = document.createElement('div'); + + function Portal({children}) { + return
{ReactDOM.createPortal(children, portalContainer)}
; + } + + const root = ReactDOMClient.createRoot(container); + // Mount hidden tree. + await act(() => { + root.render( + + + + + , + ); + }); + // No layout effect. + assertLog(['Child']); + expect(container.innerHTML).toBe('
'); + expect(portalContainer.innerHTML).toBe( + '', + ); + + // Unhide the tree. The layout effect is mounted. + await act(() => { + root.render( + + + + + , + ); + }); + assertLog(['Child', 'Mount layout']); + expect(container.innerHTML).toBe('
'); + expect(portalContainer.innerHTML).toBe( + '', + ); + }); + + // @gate enableLegacyHidden + it('does not toggle effects or hide nodes for LegacyHidden component inside portal', async () => { + function Child() { + useLayoutEffect(() => { + Scheduler.log('Mount layout'); + return () => { + Scheduler.log('Unmount layout'); + }; + }, []); + useEffect(() => { + Scheduler.log('Mount passive'); + return () => { + Scheduler.log('Unmount passive'); + }; + }, []); + return ; + } + + const portalContainer = document.createElement('div'); + + function Portal({children}) { + return
{ReactDOM.createPortal(children, portalContainer)}
; + } + + const root = ReactDOMClient.createRoot(container); + // Mount visible tree. + await act(() => { + root.render( + + + + + , + ); + }); + assertLog(['Child', 'Mount layout', 'Mount passive']); + expect(container.innerHTML).toBe('
'); + expect(portalContainer.innerHTML).toBe(''); + + // Hide the tree. + await act(() => { + root.render( + + + + + , + ); + }); + // Effects not unmounted. + assertLog(['Child']); + expect(container.innerHTML).toBe('
'); + expect(portalContainer.innerHTML).toBe(''); + + // Unhide the tree. + await act(() => { + root.render( + + + + + , + ); + }); + // Effects already mounted. + assertLog(['Child']); + expect(container.innerHTML).toBe('
'); + expect(portalContainer.innerHTML).toBe(''); + }); }); From db8273c12f363f350330c4712aeaf969a3eee820 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes <57368278+jorge-cab@users.noreply.github.com> Date: Tue, 11 Nov 2025 10:16:04 -0800 Subject: [PATCH 2/3] [compiler] Update test snap to include fixture comment (#35100) Summary: I missed this test case failing and now having @loggerTestOnly after landing some other PRs good to know they're not land blocking --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35100). * #35099 * __->__ #35100 --- ...m-prop-no-show-in-data-flow-tree.expect.md | 31 +++++++++++++------ ...ved-from-prop-no-show-in-data-flow-tree.js | 2 +- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md index 7ab14466b256b..87cf7722da353 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.expect.md @@ -2,17 +2,23 @@ ## Input ```javascript -// @validateNoDerivedComputationsInEffects_exp +// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly -function Component({ prop }) { - const [s, setS] = useState(prop) - const [second, setSecond] = useState(prop) +function Component({prop}) { + const [s, setS] = useState(); + const [second, setSecond] = useState(prop); + /* + * `second` is a source of state. It will inherit the value of `prop` in + * the first render, but after that it will no longer be updated when + * `prop` changes. So we shouldn't consider `second` as being derived from + * `prop` + */ useEffect(() => { - setS(second) - }, [second]) + setS(second); + }, [second]); - return
{s}
+ return
{s}
; } ``` @@ -20,12 +26,12 @@ function Component({ prop }) { ## Code ```javascript -import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp +import { c as _c } from "react/compiler-runtime"; // @validateNoDerivedComputationsInEffects_exp @loggerTestOnly function Component(t0) { const $ = _c(5); const { prop } = t0; - const [s, setS] = useState(prop); + const [s, setS] = useState(); const [second] = useState(prop); let t1; let t2; @@ -54,6 +60,13 @@ function Component(t0) { } ``` + +## Logs + +``` +{"kind":"CompileError","detail":{"options":{"description":"Using an effect triggers an additional render which can hurt performance and user experience, potentially briefly showing stale values to the user\n\nThis setState call is setting a derived value that depends on the following reactive sources:\n\nState: [second]\n\nData Flow Tree:\nā””ā”€ā”€ second (State)\n\nSee: https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state","category":"EffectDerivationsOfState","reason":"You might not need an effect. Derive values in render, not effects.","details":[{"kind":"error","loc":{"start":{"line":14,"column":4,"index":443},"end":{"line":14,"column":8,"index":447},"filename":"usestate-derived-from-prop-no-show-in-data-flow-tree.ts","identifierName":"setS"},"message":"This should be computed during render, not in an effect"}]}},"fnLoc":null} +{"kind":"CompileSuccess","fnLoc":{"start":{"line":3,"column":0,"index":64},"end":{"line":18,"column":1,"index":500},"filename":"usestate-derived-from-prop-no-show-in-data-flow-tree.ts"},"fnName":"Component","memoSlots":5,"memoBlocks":2,"memoValues":3,"prunedMemoBlocks":0,"prunedMemoValues":0} +``` ### Eval output (kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.js index 5c62fa2e8f1fc..5a7a693d50b01 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/effect-derived-computations/usestate-derived-from-prop-no-show-in-data-flow-tree.js @@ -1,4 +1,4 @@ -// @validateNoDerivedComputationsInEffects_exp +// @validateNoDerivedComputationsInEffects_exp @loggerTestOnly function Component({prop}) { const [s, setS] = useState(); From 5e94655cbbeae6f4b1806fdbfbb9902b136fa574 Mon Sep 17 00:00:00 2001 From: Jorge Cabiedes <57368278+jorge-cab@users.noreply.github.com> Date: Tue, 11 Nov 2025 10:16:20 -0800 Subject: [PATCH 3/3] [compiler] _exp version of ValidateNoDerivedComputationsInEffects take precedence over stable version when enabled (#35099) Summary: We should only run one version of the validation. I think it makes sense that if the exp version is enable it takes precedence over the stable one --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35099). * __->__ #35099 * #35100 --- .../babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 0c777f87707da..220c9d5c3d2b9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -272,12 +272,10 @@ function runWithEnvironment( validateNoSetStateInRender(hir).unwrap(); } - if (env.config.validateNoDerivedComputationsInEffects) { - validateNoDerivedComputationsInEffects(hir); - } - if (env.config.validateNoDerivedComputationsInEffects_exp) { env.logErrors(validateNoDerivedComputationsInEffects_exp(hir)); + } else if (env.config.validateNoDerivedComputationsInEffects) { + validateNoDerivedComputationsInEffects(hir); } if (env.config.validateNoSetStateInEffects) {