From 53b12e46a17549ec7644e13c126440ed2f3629fd Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 1 Feb 2024 13:28:14 -0500 Subject: [PATCH 01/62] Add stable React.act export (#28160) Starting in version 19, users can import the `act` testing API from the `react` package instead of using a renderer specific API, like `react-dom/test-utils`. --- fixtures/dom/src/__tests__/nested-act-test.js | 4 ++-- .../src/__tests__/storeComponentFilters-test.js | 4 +++- packages/react-devtools-shared/src/__tests__/utils.js | 4 +++- .../react-dom/src/__tests__/ReactDOMHydrationDiff-test.js | 2 +- .../src/__tests__/ReactTestUtilsActUnmockedScheduler-test.js | 2 +- packages/react-dom/src/test-utils/ReactTestUtils.js | 4 +++- .../react-reconciler/src/__tests__/ReactActWarnings-test.js | 2 +- .../src/__tests__/ReactFiberHostContext-test.internal.js | 2 +- .../react-reconciler/src/__tests__/ReactIsomorphicAct-test.js | 2 +- .../src/__tests__/ReactSchedulerIntegration-test.js | 2 +- packages/react-refresh/src/__tests__/ReactFresh-test.js | 4 ++-- .../src/__tests__/ReactFlightDOMForm-test.js | 2 +- packages/react-test-renderer/src/ReactTestRenderer.js | 2 +- packages/react/index.classic.fb.js | 2 +- packages/react/index.experimental.js | 2 +- packages/react/index.js | 2 +- packages/react/index.modern.fb.js | 2 +- packages/react/index.stable.js | 2 +- 18 files changed, 26 insertions(+), 20 deletions(-) diff --git a/fixtures/dom/src/__tests__/nested-act-test.js b/fixtures/dom/src/__tests__/nested-act-test.js index 4a4c63eaad105..6c7f60c2e2f13 100644 --- a/fixtures/dom/src/__tests__/nested-act-test.js +++ b/fixtures/dom/src/__tests__/nested-act-test.js @@ -20,7 +20,7 @@ describe('unmocked scheduler', () => { beforeEach(() => { jest.resetModules(); React = require('react'); - DOMAct = React.unstable_act; + DOMAct = React.act; TestRenderer = require('react-test-renderer'); TestAct = TestRenderer.act; }); @@ -61,7 +61,7 @@ describe('mocked scheduler', () => { require.requireActual('scheduler/unstable_mock') ); React = require('react'); - DOMAct = React.unstable_act; + DOMAct = React.act; TestRenderer = require('react-test-renderer'); TestAct = TestRenderer.act; }); diff --git a/packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js b/packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js index 44c7e85217cfd..e2a59177b687f 100644 --- a/packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js +++ b/packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js @@ -19,7 +19,9 @@ describe('Store component filters', () => { let utils; const act = async (callback: Function) => { - if (React.unstable_act != null) { + if (React.act != null) { + await React.act(callback); + } else if (React.unstable_act != null) { await React.unstable_act(callback); } else { callback(); diff --git a/packages/react-devtools-shared/src/__tests__/utils.js b/packages/react-devtools-shared/src/__tests__/utils.js index 1a74c1fee9c61..21a6314eebb57 100644 --- a/packages/react-devtools-shared/src/__tests__/utils.js +++ b/packages/react-devtools-shared/src/__tests__/utils.js @@ -21,7 +21,9 @@ export function act( const {act: actTestRenderer} = require('react-test-renderer'); // Use `require('react-dom/test-utils').act` as a fallback for React 17, which can be used in integration tests for React DevTools. const actDOM = - require('react').unstable_act || require('react-dom/test-utils').act; + require('react').act || + require('react').unstable_act || + require('react-dom/test-utils').act; actDOM(() => { actTestRenderer(() => { diff --git a/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js b/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js index 0b6d5dabffe42..76a8229e5a89f 100644 --- a/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js @@ -25,7 +25,7 @@ describe('ReactDOMServerHydration', () => { React = require('react'); ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); - act = React.unstable_act; + act = React.act; console.error = jest.fn(); container = document.createElement('div'); diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsActUnmockedScheduler-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsActUnmockedScheduler-test.js index d7ed05673c67f..45babfd4032d3 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsActUnmockedScheduler-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsActUnmockedScheduler-test.js @@ -32,7 +32,7 @@ beforeEach(() => { yields = []; React = require('react'); ReactDOMClient = require('react-dom/client'); - act = React.unstable_act; + act = React.act; container = document.createElement('div'); document.body.appendChild(container); }); diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 568e8327198f1..daea520a61eca 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -39,7 +39,9 @@ const getFiberCurrentPropsFromNode = EventInternals[2]; const enqueueStateRestore = EventInternals[3]; const restoreStateIfNeeded = EventInternals[4]; -const act = React.unstable_act; +// TODO: Add a warning if this API is accessed with advice to switch to +// importing directly from the React package instead. +const act = React.act; function Event(suffix) {} diff --git a/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js b/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js index 4cf0fd4d3bd12..61cde5648a9e6 100644 --- a/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js +++ b/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js @@ -28,7 +28,7 @@ describe('act warnings', () => { React = require('react'); Scheduler = require('scheduler'); ReactNoop = require('react-noop-renderer'); - act = React.unstable_act; + act = React.act; useState = React.useState; Suspense = React.Suspense; startTransition = React.startTransition; diff --git a/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js index 63dc79e6f99d3..0711fb3adb3d0 100644 --- a/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js @@ -20,7 +20,7 @@ describe('ReactFiberHostContext', () => { beforeEach(() => { jest.resetModules(); React = require('react'); - act = React.unstable_act; + act = React.act; ReactFiberReconciler = require('react-reconciler'); ConcurrentRoot = require('react-reconciler/src/ReactRootTags').ConcurrentRoot; diff --git a/packages/react-reconciler/src/__tests__/ReactIsomorphicAct-test.js b/packages/react-reconciler/src/__tests__/ReactIsomorphicAct-test.js index 28b5333e4cf4c..6b1c284a7eb9d 100644 --- a/packages/react-reconciler/src/__tests__/ReactIsomorphicAct-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIsomorphicAct-test.js @@ -28,7 +28,7 @@ describe('isomorphic act()', () => { ReactNoop = require('react-noop-renderer'); DiscreteEventPriority = require('react-reconciler/constants').DiscreteEventPriority; - act = React.unstable_act; + act = React.act; use = React.use; Suspense = React.Suspense; startTransition = React.startTransition; diff --git a/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js b/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js index 17067638ac811..a29280b36c569 100644 --- a/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js @@ -370,7 +370,7 @@ describe('`act` bypasses Scheduler methods completely,', () => { } const root = ReactNoop.createRoot(); - const publicAct = React.unstable_act; + const publicAct = React.act; const prevIsReactActEnvironment = global.IS_REACT_ACT_ENVIRONMENT; try { global.IS_REACT_ACT_ENVIRONMENT = true; diff --git a/packages/react-refresh/src/__tests__/ReactFresh-test.js b/packages/react-refresh/src/__tests__/ReactFresh-test.js index 8ce009ed72043..94e170e3346e6 100644 --- a/packages/react-refresh/src/__tests__/ReactFresh-test.js +++ b/packages/react-refresh/src/__tests__/ReactFresh-test.js @@ -34,7 +34,7 @@ describe('ReactFresh', () => { ReactDOM = require('react-dom'); ReactDOMClient = require('react-dom/client'); Scheduler = require('scheduler'); - act = React.unstable_act; + act = React.act; internalAct = require('internal-test-utils').act; const InternalTestUtils = require('internal-test-utils'); @@ -3792,7 +3792,7 @@ describe('ReactFresh', () => { React = require('react'); ReactDOM = require('react-dom'); Scheduler = require('scheduler'); - act = React.unstable_act; + act = React.act; internalAct = require('internal-test-utils').act; // Important! Inject into the global hook *after* ReactDOM runs: diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMForm-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMForm-test.js index b95a6c824f092..dcab688b10c83 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMForm-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMForm-test.js @@ -54,7 +54,7 @@ describe('ReactFlightDOMForm', () => { ReactServerDOMClient = require('react-server-dom-webpack/client.edge'); ReactDOMServer = require('react-dom/server.edge'); ReactDOMClient = require('react-dom/client'); - act = React.unstable_act; + act = React.act; useFormState = require('react-dom').useFormState; container = document.createElement('div'); document.body.appendChild(container); diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index 4b19f599db5ac..c82c33811539c 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -54,7 +54,7 @@ import {getPublicInstance} from './ReactFiberConfigTestHost'; import {ConcurrentRoot, LegacyRoot} from 'react-reconciler/src/ReactRootTags'; import {allowConcurrentByDefault} from 'shared/ReactFeatureFlags'; -const act = React.unstable_act; +const act = React.act; // TODO: Remove from public bundle diff --git a/packages/react/index.classic.fb.js b/packages/react/index.classic.fb.js index a417f3e755cab..7c84775bb6cc3 100644 --- a/packages/react/index.classic.fb.js +++ b/packages/react/index.classic.fb.js @@ -9,7 +9,7 @@ export { __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED, - act as unstable_act, + act, Children, Component, Fragment, diff --git a/packages/react/index.experimental.js b/packages/react/index.experimental.js index dd86090f093e1..c35fd1bd55157 100644 --- a/packages/react/index.experimental.js +++ b/packages/react/index.experimental.js @@ -9,7 +9,7 @@ export { __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED, - act as unstable_act, + act, Children, Component, Fragment, diff --git a/packages/react/index.js b/packages/react/index.js index abce6537b5675..70f61f58e070b 100644 --- a/packages/react/index.js +++ b/packages/react/index.js @@ -30,7 +30,7 @@ export type ChildrenArray<+T> = $ReadOnlyArray> | T; // We can't use export * from in Flow for some reason. export { __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED, - act as unstable_act, + act, Children, Component, Fragment, diff --git a/packages/react/index.modern.fb.js b/packages/react/index.modern.fb.js index 10ae150f64eef..4f73dded7f7eb 100644 --- a/packages/react/index.modern.fb.js +++ b/packages/react/index.modern.fb.js @@ -9,7 +9,7 @@ export { __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED, - act as unstable_act, + act, Children, Component, Fragment, diff --git a/packages/react/index.stable.js b/packages/react/index.stable.js index 9f8e46063782a..2997a62b4a44a 100644 --- a/packages/react/index.stable.js +++ b/packages/react/index.stable.js @@ -9,7 +9,7 @@ export { __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED, - act as unstable_act, + act, Children, Component, Fragment, From a1433ca0bacff76f720ffec9a0020f56e8c9ffed Mon Sep 17 00:00:00 2001 From: StyleShit <32631382+StyleShit@users.noreply.github.com> Date: Thu, 1 Feb 2024 21:30:17 +0200 Subject: [PATCH 02/62] fix(eslint-plugin-react-hooks): accepting `as` expressions as deps array (#28189) ## Summary This PR closes #25844 The original issue talks about `as const`, but seems like it fails for any `as X` expressions since it adds another nesting level to the AST. EDIT: Also closes #20162 ## How did you test this change? Added unit tests --- .../__tests__/ESLintRuleExhaustiveDeps-test.js | 18 ++++++++++++++++++ .../src/ExhaustiveDeps.js | 13 +++++++++++-- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index eb58f8d4d1fbe..7f545540acf90 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -7747,6 +7747,24 @@ const testsTypescript = { } `, }, + { + code: normalizeIndent` + function App(props) { + React.useEffect(() => { + console.log(props.test); + }, [props.test] as const); + } + `, + }, + { + code: normalizeIndent` + function App(props) { + React.useEffect(() => { + console.log(props.test); + }, [props.test] as any); + } + `, + }, ], invalid: [ { diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 62a451377f6e6..e32a2c7a111b3 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -618,7 +618,12 @@ export default { const declaredDependencies = []; const externalDependencies = new Set(); - if (declaredDependenciesNode.type !== 'ArrayExpression') { + const isArrayExpression = + declaredDependenciesNode.type === 'ArrayExpression'; + const isTSAsArrayExpression = + declaredDependenciesNode.type === 'TSAsExpression' && + declaredDependenciesNode.expression.type === 'ArrayExpression'; + if (!isArrayExpression && !isTSAsArrayExpression) { // If the declared dependencies are not an array expression then we // can't verify that the user provided the correct dependencies. Tell // the user this in an error. @@ -631,7 +636,11 @@ export default { 'dependencies.', }); } else { - declaredDependenciesNode.elements.forEach(declaredDependencyNode => { + const arrayExpression = isTSAsArrayExpression + ? declaredDependenciesNode.expression + : declaredDependenciesNode; + + arrayExpression.elements.forEach(declaredDependencyNode => { // Skip elided elements. if (declaredDependencyNode === null) { return; From 6f8f0005926cc1cdf1d12b3e1c2cc9fce1a17baa Mon Sep 17 00:00:00 2001 From: Ricky Date: Thu, 1 Feb 2024 14:44:26 -0500 Subject: [PATCH 03/62] Update react docs link in issue template (#28195) --- .github/ISSUE_TEMPLATE/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml index e5bb31b2b3787..9f8129db9e4ee 100644 --- a/.github/ISSUE_TEMPLATE/config.yml +++ b/.github/ISSUE_TEMPLATE/config.yml @@ -1,6 +1,6 @@ contact_links: - name: 📃 Documentation Issue - url: https://github.com/reactjs/reactjs.org/issues/new + url: https://github.com/reactjs/react.dev/issues/new/choose about: This issue tracker is not for documentation issues. Please file documentation issues here. - name: 🤔 Questions and Help url: https://reactjs.org/community/support.html From 94259cd57a72123af224d6786c96f5915de06d63 Mon Sep 17 00:00:00 2001 From: Noah Lemen Date: Thu, 1 Feb 2024 14:45:06 -0500 Subject: [PATCH 04/62] convert ReactElementClone-test from renderIntoDocument (#28193) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary migrates to createRoot – renderIntoDocument uses ReactDOM.render ## How did you test this change? yarn test ReactElementClone --- .../src/__tests__/ReactElementClone-test.js | 108 ++++++++++++------ 1 file changed, 76 insertions(+), 32 deletions(-) diff --git a/packages/react/src/__tests__/ReactElementClone-test.js b/packages/react/src/__tests__/ReactElementClone-test.js index 1aecc91359c28..053341013a6ed 100644 --- a/packages/react/src/__tests__/ReactElementClone-test.js +++ b/packages/react/src/__tests__/ReactElementClone-test.js @@ -9,19 +9,20 @@ 'use strict'; +let act; let PropTypes; let React; -let ReactDOM; -let ReactTestUtils; +let ReactDOMClient; describe('ReactElementClone', () => { let ComponentClass; beforeEach(() => { + act = require('internal-test-utils').act; + PropTypes = require('prop-types'); React = require('react'); - ReactDOM = require('react-dom'); - ReactTestUtils = require('react-dom/test-utils'); + ReactDOMClient = require('react-dom/client'); // NOTE: We're explicitly not using JSX here. This is intended to test // classic JS without JSX. @@ -32,10 +33,15 @@ describe('ReactElementClone', () => { }; }); - it('should clone a DOM component with new props', () => { + it('should clone a DOM component with new props', async () => { + let div; class Grandparent extends React.Component { render() { - return } />; + return ( + (div = node)} className="child" />} + /> + ); } } class Parent extends React.Component { @@ -47,14 +53,21 @@ describe('ReactElementClone', () => { ); } } - const component = ReactTestUtils.renderIntoDocument(); - expect(ReactDOM.findDOMNode(component).childNodes[0].className).toBe('xyz'); + + const root = ReactDOMClient.createRoot(document.createElement('div')); + await act(() => { + root.render(); + }); + expect(div.className).toBe('xyz'); }); - it('should clone a composite component with new props', () => { + it('should clone a composite component with new props', async () => { + let div; class Child extends React.Component { render() { - return
; + return ( +
(div = node)} className={this.props.className} /> + ); } } class Grandparent extends React.Component { @@ -71,8 +84,11 @@ describe('ReactElementClone', () => { ); } } - const component = ReactTestUtils.renderIntoDocument(); - expect(ReactDOM.findDOMNode(component).childNodes[0].className).toBe('xyz'); + const root = ReactDOMClient.createRoot(document.createElement('div')); + await act(() => { + root.render(); + }); + expect(div.className).toBe('xyz'); }); it('does not fail if config has no prototype', () => { @@ -80,10 +96,15 @@ describe('ReactElementClone', () => { React.cloneElement(
, config); }); - it('should keep the original ref if it is not overridden', () => { + it('should keep the original ref if it is not overridden', async () => { + let component; class Grandparent extends React.Component { yoloRef = React.createRef(); + componentDidMount() { + component = this; + } + render() { return } />; } @@ -97,7 +118,11 @@ describe('ReactElementClone', () => { } } - const component = ReactTestUtils.renderIntoDocument(); + const root = ReactDOMClient.createRoot(document.createElement('div')); + await act(() => { + root.render(); + }); + expect(component.yoloRef.current.tagName).toBe('DIV'); }); @@ -111,7 +136,7 @@ describe('ReactElementClone', () => { expect(clone.key).toBe('xyz'); }); - it('should transfer children', () => { + it('should transfer children', async () => { class Component extends React.Component { render() { expect(this.props.children).toBe('xyz'); @@ -119,12 +144,13 @@ describe('ReactElementClone', () => { } } - ReactTestUtils.renderIntoDocument( - React.cloneElement(, {children: 'xyz'}), - ); + const root = ReactDOMClient.createRoot(document.createElement('div')); + await act(() => { + root.render(React.cloneElement(, {children: 'xyz'})); + }); }); - it('should shallow clone children', () => { + it('should shallow clone children', async () => { class Component extends React.Component { render() { expect(this.props.children).toBe('xyz'); @@ -132,9 +158,10 @@ describe('ReactElementClone', () => { } } - ReactTestUtils.renderIntoDocument( - React.cloneElement(xyz, {}), - ); + const root = ReactDOMClient.createRoot(document.createElement('div')); + await act(() => { + root.render(React.cloneElement(xyz, {})); + }); }); it('should accept children as rest arguments', () => { @@ -174,7 +201,8 @@ describe('ReactElementClone', () => { expect(element2.props.children).toBe(undefined); }); - it('should support keys and refs', () => { + it('should support keys and refs', async () => { + let component; class Parent extends React.Component { xyzRef = React.createRef(); @@ -192,6 +220,10 @@ describe('ReactElementClone', () => { class Grandparent extends React.Component { parentRef = React.createRef(); + componentDidMount() { + component = this; + } + render() { return ( @@ -201,11 +233,13 @@ describe('ReactElementClone', () => { } } - const component = ReactTestUtils.renderIntoDocument(); + const root = ReactDOMClient.createRoot(document.createElement('div')); + await act(() => root.render()); expect(component.parentRef.current.xyzRef.current.tagName).toBe('SPAN'); }); - it('should steal the ref if a new ref is specified', () => { + it('should steal the ref if a new ref is specified', async () => { + let component; class Parent extends React.Component { xyzRef = React.createRef(); @@ -221,6 +255,10 @@ describe('ReactElementClone', () => { parentRef = React.createRef(); childRef = React.createRef(); + componentDidMount() { + component = this; + } + render() { return ( @@ -230,12 +268,13 @@ describe('ReactElementClone', () => { } } - const component = ReactTestUtils.renderIntoDocument(); + const root = ReactDOMClient.createRoot(document.createElement('div')); + await act(() => root.render()); expect(component.childRef).toEqual({current: null}); expect(component.parentRef.current.xyzRef.current.tagName).toBe('SPAN'); }); - it('should overwrite props', () => { + it('should overwrite props', async () => { class Component extends React.Component { render() { expect(this.props.myprop).toBe('xyz'); @@ -243,8 +282,11 @@ describe('ReactElementClone', () => { } } - ReactTestUtils.renderIntoDocument( - React.cloneElement(, {myprop: 'xyz'}), + const root = ReactDOMClient.createRoot(document.createElement('div')); + await act(() => + root.render( + React.cloneElement(, {myprop: 'xyz'}), + ), ); }); @@ -287,7 +329,7 @@ describe('ReactElementClone', () => { React.cloneElement(
, null, [{}, {}]); }); - it('should check declared prop types after clone', () => { + it('should check declared prop types after clone', async () => { class Component extends React.Component { static propTypes = { color: PropTypes.string.isRequired, @@ -308,8 +350,10 @@ describe('ReactElementClone', () => { }); } } - expect(() => - ReactTestUtils.renderIntoDocument(React.createElement(GrandParent)), + const root = ReactDOMClient.createRoot(document.createElement('div')); + await expect( + async () => + await act(() => root.render(React.createElement(GrandParent))), ).toErrorDev( 'Warning: Failed prop type: ' + 'Invalid prop `color` of type `number` supplied to `Component`, ' + From 4dd475c97799f3fb83bdd2fff2d028e0e30041cf Mon Sep 17 00:00:00 2001 From: Noah Lemen Date: Thu, 1 Feb 2024 14:49:30 -0500 Subject: [PATCH 05/62] convert ReactElement-test from renderIntoDocument (#28161) ## Summary refactors ReactElement-test to use `createRoot` instead of `renderIntoDocument`, which uses `ReactDOM.render` under the hood ## How did you test this change? `yarn test ReactElement` --- .../react/src/__tests__/ReactElement-test.js | 46 ++++++++++++------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/packages/react/src/__tests__/ReactElement-test.js b/packages/react/src/__tests__/ReactElement-test.js index a7ff7d9747374..05aab3c326db7 100644 --- a/packages/react/src/__tests__/ReactElement-test.js +++ b/packages/react/src/__tests__/ReactElement-test.js @@ -13,7 +13,6 @@ let act; let React; let ReactDOMClient; -let ReactTestUtils; describe('ReactElement', () => { let ComponentClass; @@ -25,7 +24,6 @@ describe('ReactElement', () => { React = require('react'); ReactDOMClient = require('react-dom/client'); - ReactTestUtils = require('react-dom/test-utils'); // NOTE: We're explicitly not using JSX here. This is intended to test // classic JS without JSX. ComponentClass = class extends React.Component { @@ -223,19 +221,21 @@ describe('ReactElement', () => { expect(element.props).toEqual({foo: '56'}); }); - it('preserves the owner on the element', () => { + it('preserves the owner on the element', async () => { let element; + let instance; class Wrapper extends React.Component { + componentDidMount() { + instance = this; + } render() { element = React.createElement(ComponentClass); return element; } } - - const instance = ReactTestUtils.renderIntoDocument( - React.createElement(Wrapper), - ); + const root = ReactDOMClient.createRoot(document.createElement('div')); + await act(() => root.render(React.createElement(Wrapper))); expect(element._owner.stateNode).toBe(instance); }); @@ -327,23 +327,28 @@ describe('ReactElement', () => { // NOTE: We're explicitly not using JSX here. This is intended to test // classic JS without JSX. - it('should normalize props with default values', () => { + it('should normalize props with default values', async () => { + let instance; class Component extends React.Component { + componentDidMount() { + instance = this; + } render() { return React.createElement('span', null, this.props.prop); } } Component.defaultProps = {prop: 'testKey'}; - const instance = ReactTestUtils.renderIntoDocument( - React.createElement(Component), - ); + const root = ReactDOMClient.createRoot(document.createElement('div')); + await act(() => { + root.render(React.createElement(Component)); + }); expect(instance.props.prop).toBe('testKey'); - const inst2 = ReactTestUtils.renderIntoDocument( - React.createElement(Component, {prop: null}), - ); - expect(inst2.props.prop).toBe(null); + await act(() => { + root.render(React.createElement(Component, {prop: null})); + }); + expect(instance.props.prop).toBe(null); }); it('throws when changing a prop (in dev) after element creation', async () => { @@ -410,13 +415,20 @@ describe('ReactElement', () => { } }); - it('does not warn for NaN props', () => { + it('does not warn for NaN props', async () => { + let test; class Test extends React.Component { + componentDidMount() { + test = this; + } render() { return
; } } - const test = ReactTestUtils.renderIntoDocument(); + const root = ReactDOMClient.createRoot(document.createElement('div')); + await act(() => { + root.render(); + }); expect(test.props.value).toBeNaN(); }); }); From 3d1da1f9ab7d54984c096e6a04c8729f3a50fd8a Mon Sep 17 00:00:00 2001 From: Ricky Date: Thu, 1 Feb 2024 14:54:20 -0500 Subject: [PATCH 06/62] Remove createRootStrictEffectsByDefault flag (#28102) There's no need to separate strict mode from strict effects mode any more. I didn't clean up the `StrictEffectMode` fiber flag, because it's used to prevent strict effects in legacy mode. I could replace those checks with `LegacyMode` checks, but when we remove legacy mode, we can remove that flag and condense them into one StrictMode flag away. --- ...DOMServerPartialHydration-test.internal.js | 9 +- packages/react-reconciler/src/ReactFiber.js | 3 +- .../src/__tests__/StrictEffectsMode-test.js | 206 +++++++++++++----- ...StrictEffectsModeDefaults-test.internal.js | 131 ++++++++--- packages/shared/ReactFeatureFlags.js | 7 - .../forks/ReactFeatureFlags.native-fb.js | 2 - .../forks/ReactFeatureFlags.native-oss.js | 1 - .../forks/ReactFeatureFlags.test-renderer.js | 1 - .../ReactFeatureFlags.test-renderer.native.js | 1 - .../ReactFeatureFlags.test-renderer.www.js | 1 - .../shared/forks/ReactFeatureFlags.www.js | 1 - scripts/jest/TestFlags.js | 3 - 12 files changed, 248 insertions(+), 118 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index f373213b099bf..8d0023b6f2078 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -776,14 +776,7 @@ describe('ReactDOMServerPartialHydration', () => { const span2 = container.getElementsByTagName('span')[0]; // This is a new node. expect(span).not.toBe(span2); - - if (gate(flags => flags.dfsEffectsRefactor)) { - // The effects list refactor causes this to be null because the Suspense Activity's child - // is null. However, since we can't hydrate Suspense in legacy this change in behavior is ok - expect(ref.current).toBe(null); - } else { - expect(ref.current).toBe(span2); - } + expect(ref.current).toBe(null); // Resolving the promise should render the final content. suspend = false; diff --git a/packages/react-reconciler/src/ReactFiber.js b/packages/react-reconciler/src/ReactFiber.js index 53d7bc601aa05..573e497454028 100644 --- a/packages/react-reconciler/src/ReactFiber.js +++ b/packages/react-reconciler/src/ReactFiber.js @@ -28,7 +28,6 @@ import { isHostSingletonType, } from './ReactFiberConfig'; import { - createRootStrictEffectsByDefault, enableCache, enableProfilerTimer, enableScopeAPI, @@ -456,7 +455,7 @@ export function createHostRootFiber( let mode; if (tag === ConcurrentRoot) { mode = ConcurrentMode; - if (isStrictMode === true || createRootStrictEffectsByDefault) { + if (isStrictMode === true) { mode |= StrictLegacyMode | StrictEffectsMode; } if ( diff --git a/packages/react-reconciler/src/__tests__/StrictEffectsMode-test.js b/packages/react-reconciler/src/__tests__/StrictEffectsMode-test.js index af35defe2325c..62ff0e19e6230 100644 --- a/packages/react-reconciler/src/__tests__/StrictEffectsMode-test.js +++ b/packages/react-reconciler/src/__tests__/StrictEffectsMode-test.js @@ -27,15 +27,6 @@ describe('StrictEffectsMode', () => { assertLog = InternalTestUtils.assertLog; }); - function supportsDoubleInvokeEffects() { - return gate( - flags => - flags.build === 'development' && - flags.createRootStrictEffectsByDefault && - flags.dfsEffectsRefactor, - ); - } - it('should not double invoke effects in legacy mode', async () => { function App({text}) { React.useEffect(() => { @@ -52,7 +43,11 @@ describe('StrictEffectsMode', () => { } await act(() => { - ReactTestRenderer.create(); + ReactTestRenderer.create( + + + , + ); }); assertLog(['useLayoutEffect mount', 'useEffect mount']); @@ -75,12 +70,17 @@ describe('StrictEffectsMode', () => { let renderer; await act(() => { - renderer = ReactTestRenderer.create(, { - isConcurrent: true, - }); + renderer = ReactTestRenderer.create( + + + , + { + isConcurrent: true, + }, + ); }); - if (supportsDoubleInvokeEffects()) { + if (__DEV__) { assertLog([ 'useLayoutEffect mount', 'useEffect mount', @@ -94,7 +94,11 @@ describe('StrictEffectsMode', () => { } await act(() => { - renderer.update(); + renderer.update( + + + , + ); }); assertLog([ @@ -128,12 +132,17 @@ describe('StrictEffectsMode', () => { let renderer; await act(() => { - renderer = ReactTestRenderer.create(, { - isConcurrent: true, - }); + renderer = ReactTestRenderer.create( + + + , + { + isConcurrent: true, + }, + ); }); - if (supportsDoubleInvokeEffects()) { + if (__DEV__) { assertLog([ 'useEffect One mount', 'useEffect Two mount', @@ -147,7 +156,11 @@ describe('StrictEffectsMode', () => { } await act(() => { - renderer.update(); + renderer.update( + + + , + ); }); assertLog([ @@ -181,12 +194,17 @@ describe('StrictEffectsMode', () => { let renderer; await act(() => { - renderer = ReactTestRenderer.create(, { - isConcurrent: true, - }); + renderer = ReactTestRenderer.create( + + + , + { + isConcurrent: true, + }, + ); }); - if (supportsDoubleInvokeEffects()) { + if (__DEV__) { assertLog([ 'useLayoutEffect One mount', 'useLayoutEffect Two mount', @@ -200,7 +218,11 @@ describe('StrictEffectsMode', () => { } await act(() => { - renderer.update(); + renderer.update( + + + , + ); }); assertLog([ @@ -232,12 +254,17 @@ describe('StrictEffectsMode', () => { let renderer; await act(() => { - renderer = ReactTestRenderer.create(, { - isConcurrent: true, - }); + renderer = ReactTestRenderer.create( + + + , + { + isConcurrent: true, + }, + ); }); - if (supportsDoubleInvokeEffects()) { + if (__DEV__) { assertLog([ 'useLayoutEffect mount', 'useEffect mount', @@ -249,7 +276,11 @@ describe('StrictEffectsMode', () => { } await act(() => { - renderer.update(); + renderer.update( + + + , + ); }); assertLog(['useLayoutEffect mount', 'useEffect mount']); @@ -286,10 +317,15 @@ describe('StrictEffectsMode', () => { } await act(() => { - ReactTestRenderer.create(, {isConcurrent: true}); + ReactTestRenderer.create( + + + , + {isConcurrent: true}, + ); }); - if (supportsDoubleInvokeEffects()) { + if (__DEV__) { assertLog([ 'componentDidMount', 'componentWillUnmount', @@ -321,12 +357,17 @@ describe('StrictEffectsMode', () => { let renderer; await act(() => { - renderer = ReactTestRenderer.create(, { - isConcurrent: true, - }); + renderer = ReactTestRenderer.create( + + + , + { + isConcurrent: true, + }, + ); }); - if (supportsDoubleInvokeEffects()) { + if (__DEV__) { assertLog([ 'componentDidMount', 'componentWillUnmount', @@ -337,7 +378,11 @@ describe('StrictEffectsMode', () => { } await act(() => { - renderer.update(); + renderer.update( + + + , + ); }); assertLog(['componentDidUpdate']); @@ -366,19 +411,28 @@ describe('StrictEffectsMode', () => { let renderer; await act(() => { - renderer = ReactTestRenderer.create(, { - isConcurrent: true, - }); + renderer = ReactTestRenderer.create( + + + , + { + isConcurrent: true, + }, + ); }); - if (supportsDoubleInvokeEffects()) { + if (__DEV__) { assertLog(['componentWillUnmount']); } else { assertLog([]); } await act(() => { - renderer.update(); + renderer.update( + + + , + ); }); assertLog(['componentDidUpdate']); @@ -410,7 +464,11 @@ describe('StrictEffectsMode', () => { } await act(() => { - ReactTestRenderer.create(); + ReactTestRenderer.create( + + + , + ); }); assertLog(['componentDidMount']); @@ -437,12 +495,17 @@ describe('StrictEffectsMode', () => { } await act(() => { - ReactTestRenderer.create(, { - isConcurrent: true, - }); + ReactTestRenderer.create( + + + , + { + isConcurrent: true, + }, + ); }); - if (supportsDoubleInvokeEffects()) { + if (__DEV__) { assertLog([ 'mount', 'useLayoutEffect mount', @@ -502,10 +565,15 @@ describe('StrictEffectsMode', () => { } await act(() => { - ReactTestRenderer.create(, {isConcurrent: true}); + ReactTestRenderer.create( + + + , + {isConcurrent: true}, + ); }); - if (supportsDoubleInvokeEffects()) { + if (__DEV__) { assertLog([ 'App useLayoutEffect mount', 'App useEffect mount', @@ -522,7 +590,7 @@ describe('StrictEffectsMode', () => { _setShowChild(true); }); - if (supportsDoubleInvokeEffects()) { + if (__DEV__) { assertLog([ 'App useLayoutEffect unmount', 'Child useLayoutEffect mount', @@ -585,12 +653,17 @@ describe('StrictEffectsMode', () => { let renderer; await act(() => { - renderer = ReactTestRenderer.create(, { - isConcurrent: true, - }); + renderer = ReactTestRenderer.create( + + + , + { + isConcurrent: true, + }, + ); }); - if (supportsDoubleInvokeEffects()) { + if (__DEV__) { assertLog([ 'componentDidMount', 'useLayoutEffect mount', @@ -611,7 +684,11 @@ describe('StrictEffectsMode', () => { } await act(() => { - renderer.update(); + renderer.update( + + + , + ); }); assertLog([ @@ -666,12 +743,17 @@ describe('StrictEffectsMode', () => { let renderer; await act(() => { - renderer = ReactTestRenderer.create(, { - isConcurrent: true, - }); + renderer = ReactTestRenderer.create( + + + , + { + isConcurrent: true, + }, + ); }); - if (supportsDoubleInvokeEffects()) { + if (__DEV__) { assertLog([ 'useLayoutEffect mount', 'useEffect mount', @@ -686,7 +768,11 @@ describe('StrictEffectsMode', () => { } await act(() => { - renderer.update(); + renderer.update( + + + , + ); }); assertLog([ diff --git a/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js b/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js index fa72a610ab9a8..880b77a27abe7 100644 --- a/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js +++ b/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js @@ -32,9 +32,6 @@ describe('StrictEffectsMode defaults', () => { waitForAll = InternalTestUtils.waitForAll; waitForPaint = InternalTestUtils.waitForPaint; assertLog = InternalTestUtils.assertLog; - - const ReactFeatureFlags = require('shared/ReactFeatureFlags'); - ReactFeatureFlags.createRootStrictEffectsByDefault = __DEV__; }); it('should not double invoke effects in legacy mode', async () => { @@ -53,7 +50,11 @@ describe('StrictEffectsMode defaults', () => { } await act(() => { - ReactNoop.renderLegacySyncRoot(); + ReactNoop.renderLegacySyncRoot( + + + , + ); }); assertLog(['useLayoutEffect mount', 'useEffect mount']); @@ -79,7 +80,11 @@ describe('StrictEffectsMode defaults', () => { } await act(() => { - ReactNoop.renderLegacySyncRoot(); + ReactNoop.renderLegacySyncRoot( + + + , + ); }); assertLog(['componentDidMount']); @@ -98,9 +103,9 @@ describe('StrictEffectsMode defaults', () => { await act(async () => { ReactNoop.render( - <> + - , + , ); await waitForPaint([ @@ -112,10 +117,10 @@ describe('StrictEffectsMode defaults', () => { await act(async () => { ReactNoop.render( - <> + - , + , ); assertLog([]); @@ -151,9 +156,9 @@ describe('StrictEffectsMode defaults', () => { await act(async () => { ReactNoop.render( - <> + - , + , ); await waitForAll([ @@ -168,10 +173,10 @@ describe('StrictEffectsMode defaults', () => { await act(async () => { ReactNoop.render( - <> + - , + , ); await waitFor([ @@ -209,7 +214,11 @@ describe('StrictEffectsMode defaults', () => { return text; } await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); assertLog([ @@ -222,7 +231,11 @@ describe('StrictEffectsMode defaults', () => { ]); await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); assertLog([ @@ -255,7 +268,11 @@ describe('StrictEffectsMode defaults', () => { } await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); assertLog([ @@ -268,7 +285,11 @@ describe('StrictEffectsMode defaults', () => { ]); await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); assertLog([ @@ -301,7 +322,11 @@ describe('StrictEffectsMode defaults', () => { } await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); assertLog([ @@ -314,7 +339,11 @@ describe('StrictEffectsMode defaults', () => { ]); await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); assertLog([ @@ -345,7 +374,11 @@ describe('StrictEffectsMode defaults', () => { } await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); assertLog([ @@ -356,7 +389,11 @@ describe('StrictEffectsMode defaults', () => { ]); await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); assertLog(['useLayoutEffect mount', 'useEffect mount']); @@ -383,7 +420,11 @@ describe('StrictEffectsMode defaults', () => { } await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); expect(onRefMock.mock.calls.length).toBe(3); @@ -417,7 +458,11 @@ describe('StrictEffectsMode defaults', () => { } await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); assertLog([ @@ -447,7 +492,11 @@ describe('StrictEffectsMode defaults', () => { } await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); assertLog([ @@ -457,7 +506,11 @@ describe('StrictEffectsMode defaults', () => { ]); await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); assertLog(['componentDidUpdate']); @@ -490,7 +543,11 @@ describe('StrictEffectsMode defaults', () => { } await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); assertLog([ @@ -540,7 +597,11 @@ describe('StrictEffectsMode defaults', () => { } await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); assertLog([ @@ -599,15 +660,19 @@ describe('StrictEffectsMode defaults', () => { function App({text}) { return ( - <> + - + ); } await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); assertLog([ @@ -623,7 +688,11 @@ describe('StrictEffectsMode defaults', () => { ]); await act(() => { - ReactNoop.render(); + ReactNoop.render( + + + , + ); }); assertLog([ diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 2645259e21244..845ff9645a838 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -171,13 +171,6 @@ export const enableFilterEmptyStringAttributesDOM = __NEXT_MAJOR__; // when we plan to enable them. // ----------------------------------------------------------------------------- -// This flag enables Strict Effects by default. We're not turning this on until -// after 18 because it requires migration work. Recommendation is to use -// to gradually upgrade components. -// If TRUE, trees rendered with createRoot will be StrictEffectsMode. -// If FALSE, these trees will be StrictLegacyMode. -export const createRootStrictEffectsByDefault = false; - export const disableModulePatternComponents = false; export const enableUseRefAccessWarning = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index a755367778cf1..d98e4e508101f 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -72,8 +72,6 @@ export const retryLaneExpirationMs = 5000; export const syncLaneExpirationMs = 250; export const transitionLaneExpirationMs = 5000; -export const createRootStrictEffectsByDefault = false; - export const disableSchedulerTimeoutInWorkLoop = false; export const enableLazyContextPropagation = false; export const enableLegacyHidden = true; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index bbf13e845205d..a8fb8c9ad740e 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -55,7 +55,6 @@ export const retryLaneExpirationMs = 5000; export const syncLaneExpirationMs = 250; export const transitionLaneExpirationMs = 5000; -export const createRootStrictEffectsByDefault = false; export const enableUseRefAccessWarning = false; export const disableSchedulerTimeoutInWorkLoop = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 3d38a4d24795f..fa019f07600d3 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -55,7 +55,6 @@ export const retryLaneExpirationMs = 5000; export const syncLaneExpirationMs = 250; export const transitionLaneExpirationMs = 5000; -export const createRootStrictEffectsByDefault = false; export const enableUseRefAccessWarning = false; export const disableSchedulerTimeoutInWorkLoop = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index d3c65d1679607..347ff62abf9af 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -49,7 +49,6 @@ export const enableCPUSuspense = false; export const enableUseMemoCacheHook = true; export const enableUseEffectEventHook = false; export const enableClientRenderFallbackOnTextMismatch = true; -export const createRootStrictEffectsByDefault = false; export const enableUseRefAccessWarning = false; export const enableRetryLaneExpiration = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index e5c1d418c80a1..236d46b4a5756 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -55,7 +55,6 @@ export const retryLaneExpirationMs = 5000; export const syncLaneExpirationMs = 250; export const transitionLaneExpirationMs = 5000; -export const createRootStrictEffectsByDefault = false; export const enableUseRefAccessWarning = false; export const disableSchedulerTimeoutInWorkLoop = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 8d5c0ed009fbb..15f5b993c8254 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -49,7 +49,6 @@ export const enableProfilerNestedUpdateScheduledHook: boolean = __PROFILE__ && dynamicFeatureFlags.enableProfilerNestedUpdateScheduledHook; export const enableUpdaterTracking = __PROFILE__; -export const createRootStrictEffectsByDefault = false; export const enableSuspenseAvoidThisFallback = true; export const enableSuspenseAvoidThisFallbackFizz = false; diff --git a/scripts/jest/TestFlags.js b/scripts/jest/TestFlags.js index 78908d91eb169..80b9efe4faac7 100644 --- a/scripts/jest/TestFlags.js +++ b/scripts/jest/TestFlags.js @@ -50,9 +50,6 @@ const environmentFlags = { FIXME: false, TODO: false, - // Turn these flags back on (or delete) once the effect list is removed in - // favor of a depth-first traversal using `subtreeTags`. - dfsEffectsRefactor: true, enableUseJSStackToTrackPassiveDurations: false, }; From 2efa38332adc1fc1500753d79fcba41a0197a7a6 Mon Sep 17 00:00:00 2001 From: StyleShit <32631382+StyleShit@users.noreply.github.com> Date: Thu, 1 Feb 2024 22:08:21 +0200 Subject: [PATCH 07/62] fix(eslint-plugin-react-hooks): accepting `as` expression as a callback (#28202) ## Summary Closes #20750 ## How did you test this change? Added a test case --- .../__tests__/ESLintRuleExhaustiveDeps-test.js | 9 +++++++++ packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js | 9 +++++++++ 2 files changed, 18 insertions(+) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 7f545540acf90..a26a5e19b2f27 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -7765,6 +7765,15 @@ const testsTypescript = { } `, }, + { + code: normalizeIndent` + function App(props) { + React.useEffect((() => { + console.log(props.test); + }) as any, [props.test]); + } + `, + }, ], invalid: [ { diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index e32a2c7a111b3..6958466a2ff5a 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -1223,6 +1223,15 @@ export default { isEffect, ); return; // Handled + case 'TSAsExpression': + visitFunctionWithDependencies( + callback.expression, + declaredDependenciesNode, + reactiveHook, + reactiveHookName, + isEffect, + ); + return; // Handled case 'Identifier': if (!declaredDependenciesNode) { // No deps, no problems. From 6054be9c86f2e01e5e24c1fe2182479fec8667e4 Mon Sep 17 00:00:00 2001 From: Ricky Date: Thu, 1 Feb 2024 18:26:33 -0500 Subject: [PATCH 08/62] Add ReactDOMClient to ServerIntegrationTestUtils (#28130) ## Overview Adds support for `ReactDOMClient` for `ServerIntegration*` tests. Also converts tests that pass without any other changes. Will follow up with other PRs for more complex cases. --- .../ReactDOMServerIntegrationCheckbox-test.js | 6 ++--- ...MServerIntegrationClassContextType-test.js | 6 ++--- .../ReactDOMServerIntegrationFragment-test.js | 6 ++--- .../ReactDOMServerIntegrationInput-test.js | 6 ++--- .../ReactDOMServerIntegrationModes-test.js | 6 ++--- .../ReactDOMServerIntegrationRefs-test.js | 6 ++--- ...ctDOMServerIntegrationSpecialTypes-test.js | 6 ++--- .../ReactDOMServerIntegrationTextarea-test.js | 6 ++--- ...OMServerIntegrationUserInteraction-test.js | 6 ++--- .../ReactDOMserverIntegrationProgress-test.js | 6 ++--- .../ReactDOMServerIntegrationTestUtils.js | 26 ++++++++++++++++--- scripts/jest/shouldIgnoreConsoleError.js | 15 ++++++++++- 12 files changed, 66 insertions(+), 35 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationCheckbox-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationCheckbox-test.js index 606ec88b89893..178ed7982a44f 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationCheckbox-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationCheckbox-test.js @@ -15,7 +15,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegratio const {disableInputAttributeSyncing} = require('shared/ReactFeatureFlags'); let React; -let ReactDOM; +let ReactDOMClient; let ReactDOMServer; let ReactTestUtils; @@ -23,13 +23,13 @@ function initModules() { // Reset warning cache. jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); // Make them available to the helpers. return { - ReactDOM, + ReactDOMClient, ReactDOMServer, ReactTestUtils, }; diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationClassContextType-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationClassContextType-test.js index dda475b7ced3e..2df2d66b9b9fa 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationClassContextType-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationClassContextType-test.js @@ -13,7 +13,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils'); let React; -let ReactDOM; +let ReactDOMClient; let ReactDOMServer; let ReactTestUtils; @@ -21,13 +21,13 @@ function initModules() { // Reset warning cache. jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); // Make them available to the helpers. return { - ReactDOM, + ReactDOMClient, ReactDOMServer, ReactTestUtils, }; diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationFragment-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationFragment-test.js index 680f283b6dbf2..8e8fc2aa8fe27 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationFragment-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationFragment-test.js @@ -13,7 +13,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils'); let React; -let ReactDOM; +let ReactDOMClient; let ReactDOMServer; let ReactTestUtils; @@ -21,13 +21,13 @@ function initModules() { // Reset warning cache. jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); // Make them available to the helpers. return { - ReactDOM, + ReactDOMClient, ReactDOMServer, ReactTestUtils, }; diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationInput-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationInput-test.js index afbbf28a41ecb..54780dae52cdb 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationInput-test.js @@ -15,7 +15,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegratio const {disableInputAttributeSyncing} = require('shared/ReactFeatureFlags'); let React; -let ReactDOM; +let ReactDOMClient; let ReactDOMServer; let ReactTestUtils; @@ -23,13 +23,13 @@ function initModules() { // Reset warning cache. jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); // Make them available to the helpers. return { - ReactDOM, + ReactDOMClient, ReactDOMServer, ReactTestUtils, }; diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationModes-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationModes-test.js index e551a72b9ace4..99cf33b821f17 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationModes-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationModes-test.js @@ -13,7 +13,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils'); let React; -let ReactDOM; +let ReactDOMClient; let ReactDOMServer; let ReactTestUtils; @@ -21,13 +21,13 @@ function initModules() { // Reset warning cache. jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); // Make them available to the helpers. return { - ReactDOM, + ReactDOMClient, ReactDOMServer, ReactTestUtils, }; diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js index 76da3e92c82ad..e5564d3d9348c 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js @@ -12,7 +12,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils'); let React; -let ReactDOM; +let ReactDOMClient; let ReactDOMServer; let ReactTestUtils; @@ -20,13 +20,13 @@ function initModules() { // Reset warning cache. jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); // Make them available to the helpers. return { - ReactDOM, + ReactDOMClient, ReactDOMServer, ReactTestUtils, }; diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationSpecialTypes-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationSpecialTypes-test.js index f3a8b869ad818..8ea1c9d53baee 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationSpecialTypes-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationSpecialTypes-test.js @@ -13,7 +13,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils'); let React; -let ReactDOM; +let ReactDOMClient; let ReactDOMServer; let ReactTestUtils; let forwardRef; @@ -26,7 +26,7 @@ function initModules() { // Reset warning cache. jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); forwardRef = React.forwardRef; @@ -44,7 +44,7 @@ function initModules() { // Make them available to the helpers. return { - ReactDOM, + ReactDOMClient, ReactDOMServer, ReactTestUtils, }; diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationTextarea-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationTextarea-test.js index 697ec7f340d88..dd19385e62c56 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationTextarea-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationTextarea-test.js @@ -13,7 +13,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils'); let React; -let ReactDOM; +let ReactDOMClient; let ReactDOMServer; let ReactTestUtils; @@ -21,13 +21,13 @@ function initModules() { // Reset warning cache. jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); // Make them available to the helpers. return { - ReactDOM, + ReactDOMClient, ReactDOMServer, ReactTestUtils, }; diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUserInteraction-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUserInteraction-test.js index b335b03b01d38..bc5980f23dda2 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUserInteraction-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUserInteraction-test.js @@ -12,7 +12,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils'); let React; -let ReactDOM; +let ReactDOMClient; let ReactDOMServer; let ReactTestUtils; @@ -20,13 +20,13 @@ function initModules() { // Reset warning cache. jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); // Make them available to the helpers. return { - ReactDOM, + ReactDOMClient, ReactDOMServer, ReactTestUtils, }; diff --git a/packages/react-dom/src/__tests__/ReactDOMserverIntegrationProgress-test.js b/packages/react-dom/src/__tests__/ReactDOMserverIntegrationProgress-test.js index b949e0ab522f9..cf51eff4aced3 100644 --- a/packages/react-dom/src/__tests__/ReactDOMserverIntegrationProgress-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMserverIntegrationProgress-test.js @@ -13,7 +13,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils'); let React; -let ReactDOM; +let ReactDOMClient; let ReactDOMServer; let ReactTestUtils; @@ -21,13 +21,13 @@ function initModules() { // Reset warning cache. jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); // Make them available to the helpers. return { - ReactDOM, + ReactDOMClient, ReactDOMServer, ReactTestUtils, }; diff --git a/packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js b/packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js index 8392b57af03cf..e9c4479b028fb 100644 --- a/packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js +++ b/packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js @@ -14,11 +14,12 @@ const shouldIgnoreConsoleError = require('../../../../../scripts/jest/shouldIgno module.exports = function (initModules) { let ReactDOM; + let ReactDOMClient; let ReactDOMServer; let act; function resetModules() { - ({ReactDOM, ReactDOMServer} = initModules()); + ({ReactDOM, ReactDOMClient, ReactDOMServer} = initModules()); act = require('internal-test-utils').act; } @@ -51,11 +52,24 @@ module.exports = function (initModules) { async function asyncReactDOMRender(reactElement, domElement, forceHydrate) { if (forceHydrate) { await act(() => { - ReactDOM.hydrate(reactElement, domElement); + if (ReactDOMClient) { + ReactDOMClient.hydrateRoot(domElement, reactElement, { + onRecoverableError: () => { + // TODO: assert on recoverable error count. + }, + }); + } else { + ReactDOM.hydrate(reactElement, domElement); + } }); } else { await act(() => { - ReactDOM.render(reactElement, domElement); + if (ReactDOMClient) { + const root = ReactDOMClient.createRoot(domElement); + root.render(reactElement); + } else { + ReactDOM.render(reactElement, domElement); + } }); } } @@ -80,7 +94,11 @@ module.exports = function (initModules) { for (let i = 0; i < console.error.mock.calls.length; i++) { const args = console.error.mock.calls[i]; const [format, ...rest] = args; - if (!shouldIgnoreConsoleError(format, rest)) { + if ( + !shouldIgnoreConsoleError(format, rest, { + TODO_ignoreHydrationErrors: true, + }) + ) { filteredWarnings.push(args); } } diff --git a/scripts/jest/shouldIgnoreConsoleError.js b/scripts/jest/shouldIgnoreConsoleError.js index 42aae220debed..79ec7fc2ad7d6 100644 --- a/scripts/jest/shouldIgnoreConsoleError.js +++ b/scripts/jest/shouldIgnoreConsoleError.js @@ -1,6 +1,10 @@ 'use strict'; -module.exports = function shouldIgnoreConsoleError(format, args) { +module.exports = function shouldIgnoreConsoleError( + format, + args, + {TODO_ignoreHydrationErrors} = {TODO_ignoreHydrationErrors: false} +) { if (__DEV__) { if (typeof format === 'string') { if (format.indexOf('Error: Uncaught [') === 0) { @@ -23,6 +27,15 @@ module.exports = function shouldIgnoreConsoleError(format, args) { // We haven't finished migrating our tests to use createRoot. return true; } + if ( + TODO_ignoreHydrationErrors && + format.indexOf( + 'An error occurred during hydration. The server HTML was replaced with client content in' + ) !== -1 + ) { + // This also gets logged by onRecoverableError, so we can ignore it. + return true; + } } else if ( format != null && typeof format.message === 'string' && From c42e7c7adc3a036c0c176d5b3dd7cf9215862815 Mon Sep 17 00:00:00 2001 From: Ricky Date: Thu, 1 Feb 2024 18:31:52 -0500 Subject: [PATCH 09/62] Add ReactDOMClient to ServerIntegrationSelect (#28132) ## Overview Branched off https://github.com/facebook/react/pull/28130 ## React for count changing ### Before These tests are weird because on main they pass, but log to the console: ``` We expected 2 warning(s), but saw 1 warning(s). We saw these warnings: Warning: Expected server HTML to contain a matching in
. at select ``` But the test fails due to an unexpected error count. The new ignored errors are: ``` Error: Uncaught [Error: Hydration failed because the initial UI does not match what was rendered on the server.] Warning: An error occurred during hydration. The server HTML was replaced with client content in
. Error: Hydration failed because the initial UI does not match what was rendered on the server. Error: There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering. ``` These seem to be the correct warnings to fire in `createRoot`, so the fix is to update the number of warnings we expect. --- .../__tests__/ReactDOMServerIntegrationSelect-test.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationSelect-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationSelect-test.js index e567ac9eac3ad..9e503be7520b2 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationSelect-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationSelect-test.js @@ -13,7 +13,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils'); let React; -let ReactDOM; +let ReactDOMClient; let ReactDOMServer; let ReactTestUtils; @@ -21,13 +21,13 @@ function initModules() { // Reset warning cache. jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); // Make them available to the helpers. return { - ReactDOM, + ReactDOMClient, ReactDOMServer, ReactTestUtils, }; @@ -253,7 +253,7 @@ describe('ReactDOMServerIntegrationSelect', () => { , - 1, + 2, ); expect(e.firstChild.selected).toBe(false); expect(e.lastChild.selected).toBe(true); @@ -268,7 +268,7 @@ describe('ReactDOMServerIntegrationSelect', () => { , - 1, + 2, ); expect(e.firstChild.selected).toBe(true); expect(e.lastChild.selected).toBe(false); From 278199b3deeee73f969a242c777c2d38406f8bfe Mon Sep 17 00:00:00 2001 From: Ricky Date: Thu, 1 Feb 2024 18:32:04 -0500 Subject: [PATCH 10/62] Add ReactDOMClient to ServerIntegrationBasic (#28133) ## Overview Branched off https://github.com/facebook/react/pull/28130 In `hydrateRoot`, we now error if you pass `undefined`: ``` Warning: Must provide initial children as second argument to hydrateRoot. ``` So we expect 1 error for this now. --- .../__tests__/ReactDOMServerIntegrationBasic-test.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationBasic-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationBasic-test.js index a0793a6215b66..2821dd088bb48 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationBasic-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationBasic-test.js @@ -15,7 +15,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegratio const TEXT_NODE_TYPE = 3; let React; -let ReactDOM; +let ReactDOMClient; let ReactDOMServer; let ReactTestUtils; @@ -23,13 +23,13 @@ function initModules() { // Reset warning cache. jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); // Make them available to the helpers. return { - ReactDOM, + ReactDOMClient, ReactDOMServer, ReactTestUtils, }; @@ -149,8 +149,10 @@ describe('ReactDOMServerIntegration', () => { expect(await render([])).toBe(null); expect(await render(false)).toBe(null); expect(await render(true)).toBe(null); - expect(await render(undefined)).toBe(null); expect(await render([[[false]], undefined])).toBe(null); + + // hydrateRoot errors for undefined children. + expect(await render(undefined, 1)).toBe(null); }); }); }); From c0d927713002fe27c4d58717d35cd930e6814c2b Mon Sep 17 00:00:00 2001 From: Ricky Date: Thu, 1 Feb 2024 18:32:18 -0500 Subject: [PATCH 11/62] Add ReactDOMClient to ServerIntegrationElements (#28134) ## Overview Branched off https://github.com/facebook/react/pull/28130 ## ~Failing~ Fixed by @eps1lon The tests are currently failing because of two tests covering special characters. I've tried a few ways to fix, but I'm stuck and will need some help understanding why they fail and how to fix. --------- Co-authored-by: Sebastian Silbermann --- .../ReactDOMServerIntegrationElements-test.js | 38 +++++++++++++++---- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js index c482010d39647..3bbdd379813e1 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js @@ -16,19 +16,23 @@ const TEXT_NODE_TYPE = 3; let React; let ReactDOM; +let ReactDOMClient; let ReactDOMServer; +let ReactFeatureFlags; let ReactTestUtils; function initModules() { jest.resetModules(); React = require('react'); ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactTestUtils = require('react-dom/test-utils'); // Make them available to the helpers. return { - ReactDOM, + ReactDOMClient, ReactDOMServer, ReactTestUtils, }; @@ -136,7 +140,13 @@ describe('ReactDOMServerIntegration', () => { // DOM nodes on the client side. We force it to fire early // so that it gets deduplicated later, and doesn't fail the test. expect(() => { - ReactDOM.render(, document.createElement('div')); + ReactDOM.flushSync(() => { + const root = ReactDOMClient.createRoot( + document.createElement('div'), + ); + + root.render(); + }); }).toErrorDev('The tag is unrecognized in this browser.'); const e = await render(Text); @@ -833,15 +843,21 @@ describe('ReactDOMServerIntegration', () => { 'an element with one text child with special characters', async render => { const e = await render(
{'foo\rbar\r\nbaz\nqux\u0000'}
); - if (render === serverRender || render === streamRender) { + if ( + render === serverRender || + render === streamRender || + (render === clientRenderOnServerString && + ReactFeatureFlags.enableClientRenderFallbackOnTextMismatch) + ) { expect(e.childNodes.length).toBe(1); - // Everything becomes LF when parsed from server HTML. + // Everything becomes LF when parsed from server HTML or hydrated if enableClientRenderFallbackOnTextMismatch is on. // Null character is ignored. expectNode(e.childNodes[0], TEXT_NODE_TYPE, 'foo\nbar\nbaz\nqux'); } else { expect(e.childNodes.length).toBe(1); - // Client rendering (or hydration) uses JS value with CR. + // Client rendering (or hydration without enableClientRenderFallbackOnTextMismatch) uses JS value with CR. // Null character stays. + expectNode( e.childNodes[0], TEXT_NODE_TYPE, @@ -860,17 +876,23 @@ describe('ReactDOMServerIntegration', () => { {'\r\nbaz\nqux\u0000'}
, ); - if (render === serverRender || render === streamRender) { + if ( + render === serverRender || + render === streamRender || + (render === clientRenderOnServerString && + ReactFeatureFlags.enableClientRenderFallbackOnTextMismatch) + ) { // We have three nodes because there is a comment between them. expect(e.childNodes.length).toBe(3); - // Everything becomes LF when parsed from server HTML. + // Everything becomes LF when parsed from server HTML or hydrated if enableClientRenderFallbackOnTextMismatch is on. // Null character is ignored. expectNode(e.childNodes[0], TEXT_NODE_TYPE, 'foo\nbar'); expectNode(e.childNodes[2], TEXT_NODE_TYPE, '\nbaz\nqux'); } else if (render === clientRenderOnServerString) { // We have three nodes because there is a comment between them. expect(e.childNodes.length).toBe(3); - // Hydration uses JS value with CR and null character. + // Hydration without enableClientRenderFallbackOnTextMismatch uses JS value with CR and null character. + expectNode(e.childNodes[0], TEXT_NODE_TYPE, 'foo\rbar'); expectNode(e.childNodes[2], TEXT_NODE_TYPE, '\r\nbaz\nqux\u0000'); } else { From fa6674b5bcf52610e92d19a5105308e56091c386 Mon Sep 17 00:00:00 2001 From: Ricky Date: Thu, 1 Feb 2024 18:32:27 -0500 Subject: [PATCH 12/62] Add ReactDOMClient to ServerIntegration(Hooks|NewContext) (#28135) ## Overview Branched off https://github.com/facebook/react/pull/28130 ### ~Failing~ Fixed by @eps1lon Most of the tests pass, but there are 3 tests that have additional warnings due to client render error retries. For example, before we would log: ``` Warning: Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks. Warning: Expected server HTML to contain a matching text node for "0" in
. ``` And now we log ``` Warning: Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks. Warning: Expected server HTML to contain a matching text node for "0" in
. Warning: Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks. ``` We can't just update the expected error count for these tests, because the additional error only happens on the client. So I need some guidance on how to fix these. --------- Co-authored-by: Sebastian Silbermann --- .../ReactDOMServerIntegrationHooks-test.js | 40 ++++++++++++++----- ...eactDOMServerIntegrationNewContext-test.js | 28 +++++++++---- 2 files changed, 51 insertions(+), 17 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js index 08f0b1a8a7de8..7e46bea5f93d9 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js @@ -15,7 +15,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils'); let React; -let ReactDOM; +let ReactDOMClient; let ReactDOMServer; let ReactTestUtils; let useState; @@ -39,7 +39,7 @@ function initModules() { jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); useState = React.useState; @@ -67,14 +67,19 @@ function initModules() { // Make them available to the helpers. return { - ReactDOM, + ReactDOMClient, ReactDOMServer, ReactTestUtils, }; } -const {resetModules, itRenders, itThrowsWhenRendering, serverRender} = - ReactDOMServerIntegrationUtils(initModules); +const { + resetModules, + itRenders, + itThrowsWhenRendering, + clientRenderOnBadMarkup, + serverRender, +} = ReactDOMServerIntegrationUtils(initModules); describe('ReactDOMServerHooks', () => { beforeEach(() => { @@ -422,8 +427,13 @@ describe('ReactDOMServerHooks', () => { }); return 'hi'; } - - const domNode = await render(, 1); + const domNode = await render( + , + render === clientRenderOnBadMarkup + ? // On hydration mismatch we retry and therefore log the warning again. + 2 + : 1, + ); expect(domNode.textContent).toEqual('hi'); }); @@ -436,7 +446,13 @@ describe('ReactDOMServerHooks', () => { return value; } - const domNode = await render(, 1); + const domNode = await render( + , + render === clientRenderOnBadMarkup + ? // On hydration mismatch we retry and therefore log the warning again. + 2 + : 1, + ); expect(domNode.textContent).toEqual('0'); }); }); @@ -859,7 +875,13 @@ describe('ReactDOMServerHooks', () => { return ; } - const domNode1 = await render(, 1); + const domNode1 = await render( + , + render === clientRenderOnBadMarkup + ? // On hydration mismatch we retry and therefore log the warning again. + 2 + : 1, + ); expect(domNode1.textContent).toEqual('42'); const domNode2 = await render(, 1); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js index cfcfc323e1d3d..cf0167eef1fd2 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js @@ -13,7 +13,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegrationTestUtils'); let React; -let ReactDOM; +let ReactDOMClient; let ReactDOMServer; let ReactTestUtils; @@ -21,19 +21,20 @@ function initModules() { // Reset warning cache. jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); // Make them available to the helpers. return { - ReactDOM, + ReactDOMClient, ReactDOMServer, ReactTestUtils, }; } -const {resetModules, itRenders} = ReactDOMServerIntegrationUtils(initModules); +const {resetModules, itRenders, clientRenderOnBadMarkup} = + ReactDOMServerIntegrationUtils(initModules); describe('ReactDOMServerIntegration', () => { beforeEach(() => { @@ -365,8 +366,13 @@ describe('ReactDOMServerIntegration', () => {
); }; - // We expect 1 error. - await render(, 1); + await render( + , + render === clientRenderOnBadMarkup + ? // On hydration mismatch we retry and therefore log the warning again. + 2 + : 1, + ); }, ); @@ -391,8 +397,14 @@ describe('ReactDOMServerIntegration', () => {
); }; - // We expect 1 error. - await render(, 1); + + await render( + , + render === clientRenderOnBadMarkup + ? // On hydration mismatch we retry and therefore log the warning again. + 2 + : 1, + ); }, ); From 4bd5e3ea5c449fdd2089f6bdfb07fb1120b0eec7 Mon Sep 17 00:00:00 2001 From: Ricky Date: Thu, 1 Feb 2024 18:32:38 -0500 Subject: [PATCH 13/62] Add ReactDOMClient to ServerIntegrationReconnecting (#28136) ## Overview Branched off https://github.com/facebook/react/pull/28130 ## Why In https://github.com/facebook/react/pull/24276 we changed the new root behavior to error when a client-render is forced for certain cases, so these now expect a mismatch even though they're using `suppressHydrationWarning`. --- ...ctDOMServerIntegrationReconnecting-test.js | 126 ++++++++++++++---- 1 file changed, 98 insertions(+), 28 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationReconnecting-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationReconnecting-test.js index eb201c17b484d..76612f510d7d2 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationReconnecting-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationReconnecting-test.js @@ -13,30 +13,30 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegratio let React; let ReactDOM; +let ReactDOMClient; let ReactDOMServer; let ReactTestUtils; -function initModules() { - // Reset warning cache. - jest.resetModules(); - - React = require('react'); - ReactDOM = require('react-dom'); - ReactDOMServer = require('react-dom/server'); - ReactTestUtils = require('react-dom/test-utils'); - - // Make them available to the helpers. - return { - ReactDOM, - ReactDOMServer, - ReactTestUtils, - }; -} - -const {resetModules, expectMarkupMismatch, expectMarkupMatch} = - ReactDOMServerIntegrationUtils(initModules); - describe('ReactDOMServerIntegration', () => { + function initModules() { + // Reset warning cache. + jest.resetModules(); + + React = require('react'); + ReactDOMClient = require('react-dom/client'); + ReactDOMServer = require('react-dom/server'); + ReactTestUtils = require('react-dom/test-utils'); + + // Make them available to the helpers. + return { + ReactDOMClient, + ReactDOMServer, + ReactTestUtils, + }; + } + + const {resetModules, expectMarkupMismatch, expectMarkupMatch} = + ReactDOMServerIntegrationUtils(initModules); beforeEach(() => { resetModules(); }); @@ -123,8 +123,8 @@ describe('ReactDOMServerIntegration', () => { it('should error reconnecting different attribute values', () => expectMarkupMismatch(
,
)); - it('can explicitly ignore errors reconnecting different element types of children', () => - expectMarkupMatch( + it('should error reconnecting different element types of children', () => + expectMarkupMismatch(
, @@ -354,8 +354,8 @@ describe('ReactDOMServerIntegration', () => {
{''}
, )); - it('can explicitly ignore reconnecting more children', () => - expectMarkupMatch( + it('can not ignore reconnecting more children', () => + expectMarkupMismatch(
, @@ -365,8 +365,8 @@ describe('ReactDOMServerIntegration', () => {
, )); - it('can explicitly ignore reconnecting fewer children', () => - expectMarkupMatch( + it('can not ignore reconnecting fewer children', () => + expectMarkupMismatch(
@@ -376,8 +376,8 @@ describe('ReactDOMServerIntegration', () => {
, )); - it('can explicitly ignore reconnecting reordered children', () => - expectMarkupMatch( + it('can not ignore reconnecting reordered children', () => + expectMarkupMismatch(
@@ -456,3 +456,73 @@ describe('ReactDOMServerIntegration', () => { )); }); }); + +describe('ReactDOMServerIntegration (legacy)', () => { + function initModules() { + // Reset warning cache. + jest.resetModules(); + + React = require('react'); + ReactDOM = require('react-dom'); + ReactDOMServer = require('react-dom/server'); + ReactTestUtils = require('react-dom/test-utils'); + + // Make them available to the helpers. + return { + ReactDOM, + ReactDOMServer, + ReactTestUtils, + }; + } + + const {resetModules, expectMarkupMatch} = + ReactDOMServerIntegrationUtils(initModules); + + beforeEach(() => { + resetModules(); + }); + + it('legacy mode can explicitly ignore errors reconnecting different element types of children', () => + expectMarkupMatch( +
+
+
, +
+ +
, + )); + + it('legacy mode can explicitly ignore reconnecting more children', () => + expectMarkupMatch( +
+
+
, +
+
+
+
, + )); + + it('legacy mode can explicitly ignore reconnecting fewer children', () => + expectMarkupMatch( +
+
+
+
, +
+
+
, + )); + + it('legacy mode can explicitly ignore reconnecting reordered children', () => + expectMarkupMatch( +
+
+ +
, +
+ +
+
, + )); +}); From c39ec17e66552d8ea8d6d3e11c1d7f85a9f7247f Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Fri, 2 Feb 2024 00:48:58 +0100 Subject: [PATCH 14/62] Convert ReactErrorBoundariesHooks to createRoot (#28175) --- ...ReactErrorBoundariesHooks-test.internal.js | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundariesHooks-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundariesHooks-test.internal.js index 63558ea98acd2..4a5679e5fcff2 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundariesHooks-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundariesHooks-test.internal.js @@ -10,16 +10,18 @@ 'use strict'; let React; -let ReactDOM; +let ReactDOMClient; +let act; describe('ReactErrorBoundariesHooks', () => { beforeEach(() => { jest.resetModules(); - ReactDOM = require('react-dom'); React = require('react'); + ReactDOMClient = require('react-dom/client'); + act = require('internal-test-utils').act; }); - it('should preserve hook order if errors are caught', () => { + it('should preserve hook order if errors are caught', async () => { function ErrorThrower() { React.useMemo(() => undefined, []); throw new Error('expected'); @@ -57,10 +59,15 @@ describe('ReactErrorBoundariesHooks', () => { } const container = document.createElement('div'); - ReactDOM.render(, container); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(); + }); - expect(() => { - ReactDOM.render(, container); - }).not.toThrow(); + await expect( + act(() => { + root.render(); + }), + ).resolves.not.toThrow(); }); }); From bc219090e3c4e54cc504215db13b08c6e8f1bddb Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Fri, 2 Feb 2024 00:49:18 +0100 Subject: [PATCH 15/62] Convert SyntheticClipboardEvent to createRoot (#28174) --- .../__tests__/SyntheticClipboardEvent-test.js | 64 ++++++++++++------- 1 file changed, 40 insertions(+), 24 deletions(-) diff --git a/packages/react-dom/src/events/__tests__/SyntheticClipboardEvent-test.js b/packages/react-dom/src/events/__tests__/SyntheticClipboardEvent-test.js index 30064ecad99d8..aae6b02239eba 100644 --- a/packages/react-dom/src/events/__tests__/SyntheticClipboardEvent-test.js +++ b/packages/react-dom/src/events/__tests__/SyntheticClipboardEvent-test.js @@ -10,14 +10,16 @@ 'use strict'; let React; -let ReactDOM; +let ReactDOMClient; +let act; describe('SyntheticClipboardEvent', () => { let container; beforeEach(() => { React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); + act = require('internal-test-utils').act; // The container has to be attached for events to fire. container = document.createElement('div'); @@ -32,7 +34,7 @@ describe('SyntheticClipboardEvent', () => { describe('ClipboardEvent interface', () => { describe('clipboardData', () => { describe('when event has clipboardData', () => { - it("returns event's clipboardData", () => { + it("returns event's clipboardData", async () => { let expectedCount = 0; // Mock clipboardData since jsdom implementation doesn't have a constructor @@ -47,30 +49,39 @@ describe('SyntheticClipboardEvent', () => { expect(event.clipboardData).toBe(clipboardData); expectedCount++; }; - const div = ReactDOM.render( -
, - container, - ); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( +
, + ); + }); + + const div = container.firstChild; let event; event = document.createEvent('Event'); event.initEvent('copy', true, true); event.clipboardData = clipboardData; - div.dispatchEvent(event); - + await act(() => { + div.dispatchEvent(event); + }); event = document.createEvent('Event'); event.initEvent('cut', true, true); event.clipboardData = clipboardData; - div.dispatchEvent(event); + await act(() => { + div.dispatchEvent(event); + }); event = document.createEvent('Event'); event.initEvent('paste', true, true); event.clipboardData = clipboardData; - div.dispatchEvent(event); + await act(() => { + div.dispatchEvent(event); + }); expect(expectedCount).toBe(3); }); @@ -79,7 +90,7 @@ describe('SyntheticClipboardEvent', () => { }); describe('EventInterface', () => { - it('is able to `preventDefault` and `stopPropagation`', () => { + it('is able to `preventDefault` and `stopPropagation`', async () => { let expectedCount = 0; const eventHandler = event => { @@ -92,14 +103,19 @@ describe('SyntheticClipboardEvent', () => { expectedCount++; }; - const div = ReactDOM.render( -
, - container, - ); + const root = ReactDOMClient.createRoot(container); + + await act(() => { + root.render( +
, + ); + }); + + const div = container.firstChild; let event; event = document.createEvent('Event'); From a7f1622117d7bbd9b2f681e962dcfba4aed62456 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Fri, 2 Feb 2024 00:49:40 +0100 Subject: [PATCH 16/62] Convert SyntheticFocusEvent to createRoot (#28173) --- .../__tests__/SyntheticFocusEvent-test.js | 72 +++++++++++-------- 1 file changed, 42 insertions(+), 30 deletions(-) diff --git a/packages/react-dom/src/events/__tests__/SyntheticFocusEvent-test.js b/packages/react-dom/src/events/__tests__/SyntheticFocusEvent-test.js index 9e70ca1486439..ef889c4b60d10 100644 --- a/packages/react-dom/src/events/__tests__/SyntheticFocusEvent-test.js +++ b/packages/react-dom/src/events/__tests__/SyntheticFocusEvent-test.js @@ -9,13 +9,15 @@ describe('SyntheticFocusEvent', () => { let React; - let ReactDOM; + let ReactDOMClient; + let act; let container; beforeEach(() => { jest.resetModules(); React = require('react'); - ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); + act = require('internal-test-utils').act; container = document.createElement('div'); document.body.appendChild(container); @@ -26,44 +28,54 @@ describe('SyntheticFocusEvent', () => { container = null; }); - test('onFocus events have the focus type', () => { + test('onFocus events have the focus type', async () => { const log = []; - ReactDOM.render( - + + ); +} + export default function CustomHooks(): React.Node { return ( @@ -124,6 +137,7 @@ export default function CustomHooks(): React.Node { + ); } From 9a1db2d21fac88a42094fa3fc266d1692551a5bd Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 5 Feb 2024 15:38:48 +0000 Subject: [PATCH 47/62] chore: add single versioned implementation of act for DevTools tests (#28186) - Moving `act` implementation to a single getter-function, which is based on React version we are testing RDT against. - Removing unused mocks for `act`, which were designed for legacy versions of React, validated with running tests against React 16 build. --- .../src/__tests__/utils.js | 40 +++++++++++++++---- .../config.build-devtools-regression.js | 2 - .../setupTests.build-devtools-regression.js | 30 -------------- 3 files changed, 32 insertions(+), 40 deletions(-) delete mode 100644 scripts/jest/devtools/setupTests.build-devtools-regression.js diff --git a/packages/react-devtools-shared/src/__tests__/utils.js b/packages/react-devtools-shared/src/__tests__/utils.js index 21a6314eebb57..50798c3ba70b2 100644 --- a/packages/react-devtools-shared/src/__tests__/utils.js +++ b/packages/react-devtools-shared/src/__tests__/utils.js @@ -7,6 +7,8 @@ * @flow */ +import semver from 'semver'; + import typeof ReactTestRenderer from 'react-test-renderer'; import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; @@ -14,16 +16,38 @@ import type Store from 'react-devtools-shared/src/devtools/store'; import type {ProfilingDataFrontend} from 'react-devtools-shared/src/devtools/views/Profiler/types'; import type {ElementType} from 'react-devtools-shared/src/frontend/types'; +import {ReactVersion} from '../../../../ReactVersions'; + +const requestedReactVersion = process.env.REACT_VERSION || ReactVersion; +export function getActDOMImplementation(): () => void | Promise { + // This is for React < 18, where act was distributed in react-dom/test-utils. + if (semver.lt(requestedReactVersion, '18.0.0')) { + const ReactDOMTestUtils = require('react-dom/test-utils'); + return ReactDOMTestUtils.act; + } + + const React = require('react'); + // This is for React 18, where act was distributed in react as unstable. + if (React.unstable_act) { + return React.unstable_act; + } + + // This is for React > 18, where act is marked as stable. + if (React.act) { + return React.act; + } + + throw new Error("Couldn't find any available act implementation"); +} + export function act( callback: Function, recursivelyFlush: boolean = true, ): void { + // act from react-test-renderer has some side effects on React DevTools + // it injects the renderer for DevTools, see ReactTestRenderer.js const {act: actTestRenderer} = require('react-test-renderer'); - // Use `require('react-dom/test-utils').act` as a fallback for React 17, which can be used in integration tests for React DevTools. - const actDOM = - require('react').act || - require('react').unstable_act || - require('react-dom/test-utils').act; + const actDOM = getActDOMImplementation(); actDOM(() => { actTestRenderer(() => { @@ -47,10 +71,10 @@ export async function actAsync( cb: () => *, recursivelyFlush: boolean = true, ): Promise { + // act from react-test-renderer has some side effects on React DevTools + // it injects the renderer for DevTools, see ReactTestRenderer.js const {act: actTestRenderer} = require('react-test-renderer'); - // Use `require('react-dom/test-utils').act` as a fallback for React 17, which can be used in integration tests for React DevTools. - const actDOM = - require('react').unstable_act || require('react-dom/test-utils').act; + const actDOM = getActDOMImplementation(); await actDOM(async () => { await actTestRenderer(async () => { diff --git a/scripts/jest/devtools/config.build-devtools-regression.js b/scripts/jest/devtools/config.build-devtools-regression.js index 2b8b650173c1e..672cb98258fde 100644 --- a/scripts/jest/devtools/config.build-devtools-regression.js +++ b/scripts/jest/devtools/config.build-devtools-regression.js @@ -31,8 +31,6 @@ if (REACT_VERSION) { '^react-dom/client$' ] = `/build/${NODE_MODULES_DIR}/react-dom`; } - - setupFiles.push(require.resolve('./setupTests.build-devtools-regression')); } module.exports = { diff --git a/scripts/jest/devtools/setupTests.build-devtools-regression.js b/scripts/jest/devtools/setupTests.build-devtools-regression.js deleted file mode 100644 index d0126731512bd..0000000000000 --- a/scripts/jest/devtools/setupTests.build-devtools-regression.js +++ /dev/null @@ -1,30 +0,0 @@ -'use strict'; - -// Regression tests use a React DOM profiling, so we need -// to replace these tests with scheduler/tracing-profiling -jest.mock('scheduler/tracing', () => { - return jest.requireActual('scheduler/tracing-profiling'); -}); - -// act doesn't exist in older versions of React, but -// DevTools tests sometimes import and depend on act to run. -// If act doesn't exist for a particular version of React, we will -// mock it with a function. This should work in most tests -// that we want to call with older versions of React. -// TODO (luna) Refactor act in DevTools test utils to not depend -// on act in react-dom or react-test-renderer so we don't need to do this -jest.mock('react-test-renderer', () => { - const reactTestRenderer = jest.requireActual('react-test-renderer'); - if (!reactTestRenderer.act) { - reactTestRenderer.act = fn => fn(); - } - return reactTestRenderer; -}); - -jest.mock('react-dom/test-utils', () => { - const testUtils = jest.requireActual('react-dom/test-utils'); - if (!testUtils.act) { - testUtils.act = fn => fn(); - } - return testUtils; -}); From 596827f6a72f6c4df0181e941c524fcbd0048914 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 5 Feb 2024 15:53:23 +0000 Subject: [PATCH 48/62] chore: add versioned render implementation for DevTools tests (#28210) Adding getter-functions for renderer implementations, which can be used for jest tests. If we are testing against React with version < 18, we are going to use legacy rendering, otherwise the concurrent one. --- .../src/__tests__/utils.js | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/packages/react-devtools-shared/src/__tests__/utils.js b/packages/react-devtools-shared/src/__tests__/utils.js index 50798c3ba70b2..8555302507ff5 100644 --- a/packages/react-devtools-shared/src/__tests__/utils.js +++ b/packages/react-devtools-shared/src/__tests__/utils.js @@ -15,6 +15,7 @@ import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; import type Store from 'react-devtools-shared/src/devtools/store'; import type {ProfilingDataFrontend} from 'react-devtools-shared/src/devtools/views/Profiler/types'; import type {ElementType} from 'react-devtools-shared/src/frontend/types'; +import type {Node as ReactNode} from 'react'; import {ReactVersion} from '../../../../ReactVersions'; @@ -99,6 +100,123 @@ export async function actAsync( } } +type RenderImplementation = { + render: (elements: ?ReactNode) => () => void, + unmount: () => void, + createContainer: () => void, + getContainer: () => ?HTMLElement, +}; + +export function getLegacyRenderImplementation(): RenderImplementation { + let ReactDOM; + let container; + const containersToRemove = []; + + beforeEach(() => { + ReactDOM = require('react-dom'); + + createContainer(); + }); + + afterEach(() => { + containersToRemove.forEach(c => document.body.removeChild(c)); + containersToRemove.splice(0, containersToRemove.length); + + ReactDOM = null; + container = null; + }); + + function render(elements) { + withErrorsOrWarningsIgnored( + ['ReactDOM.render is no longer supported in React 18'], + () => { + ReactDOM.render(elements, container); + }, + ); + + return unmount; + } + + function unmount() { + ReactDOM.unmountComponentAtNode(container); + } + + function createContainer() { + container = document.createElement('div'); + document.body.appendChild(container); + + containersToRemove.push(container); + } + + function getContainer() { + return container; + } + + return { + render, + unmount, + createContainer, + getContainer, + }; +} + +export function getModernRenderImplementation(): RenderImplementation { + let ReactDOMClient; + let container; + let root; + const containersToRemove = []; + + beforeEach(() => { + ReactDOMClient = require('react-dom/client'); + + createContainer(); + }); + + afterEach(() => { + containersToRemove.forEach(c => document.body.removeChild(c)); + containersToRemove.splice(0, containersToRemove.length); + + ReactDOMClient = null; + container = null; + root = null; + }); + + function render(elements) { + root.render(elements); + + return unmount; + } + + function unmount() { + root.unmount(); + } + + function createContainer() { + container = document.createElement('div'); + document.body.appendChild(container); + + root = ReactDOMClient.createRoot(container); + + containersToRemove.push(container); + } + + function getContainer() { + return container; + } + + return { + render, + unmount, + createContainer, + getContainer, + }; +} + +export const getVersionedRenderImplementation: () => RenderImplementation = + semver.lt(requestedReactVersion, '18.0.0') + ? getLegacyRenderImplementation + : getModernRenderImplementation; + export function beforeEachProfiling(): void { // Mock React's timing information so that test runs are predictable. jest.mock('scheduler', () => jest.requireActual('scheduler/unstable_mock')); From 88b0809447d3c20af8773a3c060ee5ace0d695f8 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 5 Feb 2024 17:07:41 +0000 Subject: [PATCH 49/62] fix: partially revert jest setup config removal to fix regression tests (#28247) Partially reverting what has been removed in https://github.com/facebook/react/pull/28186. We need `'scheduler/tracing'` mock for React >= 16.8. The error: ``` Invariant Violation: It is not supported to run the profiling version of a renderer (for example, `react-dom/profiling`) without also replacing the `scheduler/tracing` module with `scheduler/tracing-profiling`. Your bundler might have a setting for aliasing both modules. Learn more at http://fb.me/react-profiling ``` Validated by running regression tests for the whole version matrix: ``` ./scripts/circleci/download_devtools_regression_build.js 16.0 --replaceBuild && node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 16.0 --ci && ./scripts/circleci/download_devtools_regression_build.js 16.5 --replaceBuild && node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 16.5 --ci && ./scripts/circleci/download_devtools_regression_build.js 16.8 --replaceBuild && node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 16.8 --ci && ./scripts/circleci/download_devtools_regression_build.js 17.0 --replaceBuild && node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 17.0 --ci && ./scripts/circleci/download_devtools_regression_build.js 18.0 --replaceBuild && node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 18.0 --ci ``` --- scripts/jest/devtools/config.build-devtools-regression.js | 2 ++ .../jest/devtools/setupTests.build-devtools-regression.js | 7 +++++++ 2 files changed, 9 insertions(+) create mode 100644 scripts/jest/devtools/setupTests.build-devtools-regression.js diff --git a/scripts/jest/devtools/config.build-devtools-regression.js b/scripts/jest/devtools/config.build-devtools-regression.js index 672cb98258fde..2b8b650173c1e 100644 --- a/scripts/jest/devtools/config.build-devtools-regression.js +++ b/scripts/jest/devtools/config.build-devtools-regression.js @@ -31,6 +31,8 @@ if (REACT_VERSION) { '^react-dom/client$' ] = `/build/${NODE_MODULES_DIR}/react-dom`; } + + setupFiles.push(require.resolve('./setupTests.build-devtools-regression')); } module.exports = { diff --git a/scripts/jest/devtools/setupTests.build-devtools-regression.js b/scripts/jest/devtools/setupTests.build-devtools-regression.js new file mode 100644 index 0000000000000..a1221d53d2fa6 --- /dev/null +++ b/scripts/jest/devtools/setupTests.build-devtools-regression.js @@ -0,0 +1,7 @@ +'use strict'; + +// Regression tests use a React DOM profiling, so we need +// to replace these tests with scheduler/tracing-profiling +jest.mock('scheduler/tracing', () => { + return jest.requireActual('scheduler/tracing-profiling'); +}); From f3a70990a5a03bb26e6a71e43c204849392d2b07 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 5 Feb 2024 17:09:26 +0000 Subject: [PATCH 50/62] chore: use versioned render in FastRefreshDevToolsIntegration test (#28211) --- .../FastRefreshDevToolsIntegration-test.js | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/FastRefreshDevToolsIntegration-test.js b/packages/react-devtools-shared/src/__tests__/FastRefreshDevToolsIntegration-test.js index f95d1a2d5fda7..2ef5ae8d02393 100644 --- a/packages/react-devtools-shared/src/__tests__/FastRefreshDevToolsIntegration-test.js +++ b/packages/react-devtools-shared/src/__tests__/FastRefreshDevToolsIntegration-test.js @@ -7,25 +7,20 @@ * @flow */ +import {getVersionedRenderImplementation} from './utils'; + describe('Fast Refresh', () => { let React; let ReactFreshRuntime; let act; let babel; - let container; let exportsObj; let freshPlugin; - let legacyRender; let store; let withErrorsOrWarningsIgnored; - afterEach(() => { - jest.resetModules(); - }); - beforeEach(() => { exportsObj = undefined; - container = document.createElement('div'); babel = require('@babel/core'); freshPlugin = require('react-refresh/babel'); @@ -39,10 +34,12 @@ describe('Fast Refresh', () => { const utils = require('./utils'); act = utils.act; - legacyRender = utils.legacyRender; withErrorsOrWarningsIgnored = utils.withErrorsOrWarningsIgnored; }); + const {render: renderImplementation, getContainer} = + getVersionedRenderImplementation(); + function execute(source) { const compiled = babel.transform(source, { babelrc: false, @@ -73,7 +70,7 @@ describe('Fast Refresh', () => { function render(source) { const Component = execute(source); act(() => { - legacyRender(, container); + renderImplementation(); }); // Module initialization shouldn't be counted as a hot update. expect(ReactFreshRuntime.performReactRefresh()).toBe(null); @@ -98,7 +95,7 @@ describe('Fast Refresh', () => { // Here, we'll just force a re-render using the newer type to emulate this. const NextComponent = nextExports.default; act(() => { - legacyRender(, container); + renderImplementation(); }); } act(() => { @@ -142,8 +139,8 @@ describe('Fast Refresh', () => { `); - let element = container.firstChild; - expect(container.firstChild).not.toBe(null); + let element = getContainer().firstChild; + expect(getContainer().firstChild).not.toBe(null); patch(` function Parent() { @@ -163,8 +160,8 @@ describe('Fast Refresh', () => { `); // State is preserved; this verifies that Fast Refresh is wired up. - expect(container.firstChild).toBe(element); - element = container.firstChild; + expect(getContainer().firstChild).toBe(element); + element = getContainer().firstChild; patch(` function Parent() { @@ -184,7 +181,7 @@ describe('Fast Refresh', () => { `); // State is reset because hooks changed. - expect(container.firstChild).not.toBe(element); + expect(getContainer().firstChild).not.toBe(element); }); // @reactVersion >= 16.9 From f244fd3884adbe5717b7e46da18911e9899cade9 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 5 Feb 2024 17:11:22 +0000 Subject: [PATCH 51/62] chore: use versioned render in useEditableValue test (#28212) --- .../src/__tests__/useEditableValue-test.js | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/useEditableValue-test.js b/packages/react-devtools-shared/src/__tests__/useEditableValue-test.js index d3ae11df7999f..61b538e2d2e6f 100644 --- a/packages/react-devtools-shared/src/__tests__/useEditableValue-test.js +++ b/packages/react-devtools-shared/src/__tests__/useEditableValue-test.js @@ -7,22 +7,24 @@ * @flow */ +import {getVersionedRenderImplementation} from './utils'; + describe('useEditableValue', () => { let act; let React; - let legacyRender; let useEditableValue; beforeEach(() => { const utils = require('./utils'); act = utils.act; - legacyRender = utils.legacyRender; React = require('react'); useEditableValue = require('../devtools/views/hooks').useEditableValue; }); + const {render} = getVersionedRenderImplementation(); + it('should not cause a loop with values like NaN', () => { let state; @@ -32,8 +34,8 @@ describe('useEditableValue', () => { return null; } - const container = document.createElement('div'); - legacyRender(, container); + act(() => render()); + expect(state.editableValue).toEqual('NaN'); expect(state.externalValue).toEqual(NaN); expect(state.parsedValue).toEqual(NaN); @@ -50,8 +52,8 @@ describe('useEditableValue', () => { return null; } - const container = document.createElement('div'); - legacyRender(, container); + act(() => render()); + expect(state.editableValue).toEqual('1'); expect(state.externalValue).toEqual(1); expect(state.parsedValue).toEqual(1); @@ -60,7 +62,8 @@ describe('useEditableValue', () => { // If there are NO pending changes, // an update to the external prop value should override the local/pending value. - legacyRender(, container); + act(() => render()); + expect(state.editableValue).toEqual('2'); expect(state.externalValue).toEqual(2); expect(state.parsedValue).toEqual(2); @@ -78,8 +81,8 @@ describe('useEditableValue', () => { return null; } - const container = document.createElement('div'); - legacyRender(, container); + act(() => render()); + expect(state.editableValue).toEqual('1'); expect(state.externalValue).toEqual(1); expect(state.parsedValue).toEqual(1); @@ -102,7 +105,8 @@ describe('useEditableValue', () => { // If there ARE pending changes, // an update to the external prop value should NOT override the local/pending value. - legacyRender(, container); + act(() => render()); + expect(state.editableValue).toEqual('2'); expect(state.externalValue).toEqual(3); expect(state.parsedValue).toEqual(2); @@ -120,8 +124,8 @@ describe('useEditableValue', () => { return null; } - const container = document.createElement('div'); - legacyRender(, container); + act(() => render()); + expect(state.editableValue).toEqual('1'); expect(state.externalValue).toEqual(1); expect(state.parsedValue).toEqual(1); @@ -153,8 +157,8 @@ describe('useEditableValue', () => { return null; } - const container = document.createElement('div'); - legacyRender(, container); + act(() => render()); + expect(state.editableValue).toEqual('1'); expect(state.externalValue).toEqual(1); expect(state.parsedValue).toEqual(1); From 09c0769448655deaf69e2bf65eecc74268fda02a Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 5 Feb 2024 17:12:40 +0000 Subject: [PATCH 52/62] chore: use versioned render in console test (#28213) --- .../src/__tests__/console-test.js | 28 +++++++++---------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/console-test.js b/packages/react-devtools-shared/src/__tests__/console-test.js index e70c8e3a0afa8..e2674b10f3526 100644 --- a/packages/react-devtools-shared/src/__tests__/console-test.js +++ b/packages/react-devtools-shared/src/__tests__/console-test.js @@ -7,13 +7,12 @@ * @flow */ -import {normalizeCodeLocInfo} from './utils'; +import {getVersionedRenderImplementation, normalizeCodeLocInfo} from './utils'; let React; let ReactDOMClient; let act; let fakeConsole; -let legacyRender; let mockError; let mockInfo; let mockGroup; @@ -67,9 +66,10 @@ describe('console', () => { const utils = require('./utils'); act = utils.act; - legacyRender = utils.legacyRender; }); + const {render} = getVersionedRenderImplementation(); + // @reactVersion >=18.0 it('should not patch console methods that are not explicitly overridden', () => { expect(fakeConsole.error).not.toBe(mockError); @@ -185,7 +185,7 @@ describe('console', () => { return null; }; - act(() => legacyRender(, document.createElement('div'))); + act(() => render()); expect(mockWarn).toHaveBeenCalledTimes(1); expect(mockWarn.mock.calls[0]).toHaveLength(1); @@ -215,7 +215,7 @@ describe('console', () => { return null; }; - act(() => legacyRender(, document.createElement('div'))); + act(() => render()); expect(mockLog).toHaveBeenCalledTimes(1); expect(mockLog.mock.calls[0]).toHaveLength(1); @@ -256,7 +256,7 @@ describe('console', () => { return null; }; - act(() => legacyRender(, document.createElement('div'))); + act(() => render()); expect(mockLog).toHaveBeenCalledTimes(2); expect(mockLog.mock.calls[0]).toHaveLength(1); @@ -313,9 +313,8 @@ describe('console', () => { } } - const container = document.createElement('div'); - act(() => legacyRender(, container)); - act(() => legacyRender(, container)); + act(() => render()); + act(() => render()); expect(mockLog).toHaveBeenCalledTimes(2); expect(mockLog.mock.calls[0]).toHaveLength(1); @@ -367,7 +366,7 @@ describe('console', () => { } } - act(() => legacyRender(, document.createElement('div'))); + act(() => render()); expect(mockLog).toHaveBeenCalledTimes(1); expect(mockLog.mock.calls[0]).toHaveLength(1); @@ -396,7 +395,7 @@ describe('console', () => { return null; }; - act(() => legacyRender(, document.createElement('div'))); + act(() => render()); expect(mockWarn).toHaveBeenCalledTimes(1); expect(mockWarn.mock.calls[0]).toHaveLength(1); @@ -410,7 +409,7 @@ describe('console', () => { breakOnConsoleErrors: false, showInlineWarningsAndErrors: false, }); - act(() => legacyRender(, document.createElement('div'))); + act(() => render()); expect(mockWarn).toHaveBeenCalledTimes(2); expect(mockWarn.mock.calls[1]).toHaveLength(2); @@ -457,7 +456,7 @@ describe('console', () => { return null; }; - act(() => legacyRender(, document.createElement('div'))); + act(() => render()); expect(mockLog).toHaveBeenCalledTimes(1); expect(mockLog.mock.calls[0]).toHaveLength(1); @@ -483,7 +482,7 @@ describe('console', () => { return null; }; - act(() => legacyRender(, document.createElement('div'))); + act(() => render()); expect(mockWarn).toHaveBeenCalledTimes(1); expect(mockWarn.mock.calls[0][0]).toBe('Symbol:'); @@ -824,7 +823,6 @@ describe('console error', () => { const utils = require('./utils'); act = utils.act; - legacyRender = utils.legacyRender; }); // @reactVersion >=18.0 From 7e77e29ca93857b68bfc31bcd5cd566b33b7cd15 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 5 Feb 2024 17:14:00 +0000 Subject: [PATCH 53/62] chore: use versioned render in componentStacks test (#28214) --- .../src/__tests__/componentStacks-test.js | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/componentStacks-test.js b/packages/react-devtools-shared/src/__tests__/componentStacks-test.js index 401af1d9c5546..3f3ce3774332e 100644 --- a/packages/react-devtools-shared/src/__tests__/componentStacks-test.js +++ b/packages/react-devtools-shared/src/__tests__/componentStacks-test.js @@ -7,12 +7,11 @@ * @flow */ -import {normalizeCodeLocInfo} from './utils'; +import {getVersionedRenderImplementation, normalizeCodeLocInfo} from './utils'; describe('component stack', () => { let React; let act; - let legacyRender; let mockError; let mockWarn; @@ -30,11 +29,12 @@ describe('component stack', () => { const utils = require('./utils'); act = utils.act; - legacyRender = utils.legacyRender; React = require('react'); }); + const {render} = getVersionedRenderImplementation(); + // @reactVersion >=16.9 it('should log the current component stack along with an error or warning', () => { const Grandparent = () => ; @@ -45,9 +45,7 @@ describe('component stack', () => { return null; }; - const container = document.createElement('div'); - - act(() => legacyRender(, container)); + act(() => render()); expect(mockError).toHaveBeenCalledWith( 'Test error.', @@ -79,8 +77,7 @@ describe('component stack', () => { return null; }; - const container = document.createElement('div'); - act(() => legacyRender(, container)); + act(() => render()); expect(useEffectCount).toBe(1); From 4f82bf4f59a91e81191f39fa328a3d26aba6371a Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 5 Feb 2024 17:15:16 +0000 Subject: [PATCH 54/62] chore: use versioned render in storeOwners test (#28215) --- .../src/__tests__/storeOwners-test.js | 44 +++++-------------- 1 file changed, 11 insertions(+), 33 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/storeOwners-test.js b/packages/react-devtools-shared/src/__tests__/storeOwners-test.js index df388f69a6c06..f4fd6ba2ba826 100644 --- a/packages/react-devtools-shared/src/__tests__/storeOwners-test.js +++ b/packages/react-devtools-shared/src/__tests__/storeOwners-test.js @@ -8,11 +8,11 @@ */ const {printOwnersList} = require('../devtools/utils'); +const {getVersionedRenderImplementation} = require('./utils'); describe('Store owners list', () => { let React; let act; - let legacyRender; let store; beforeEach(() => { @@ -23,9 +23,10 @@ describe('Store owners list', () => { const utils = require('./utils'); act = utils.act; - legacyRender = utils.legacyRender; }); + const {render} = getVersionedRenderImplementation(); + function getFormattedOwnersList(elementID) { const ownersList = store.getOwnersListForElement(elementID); return printOwnersList(ownersList); @@ -43,7 +44,7 @@ describe('Store owners list', () => { const Leaf = () =>
Leaf
; const Intermediate = ({children}) => {children}; - act(() => legacyRender(, document.createElement('div'))); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¾ @@ -80,7 +81,7 @@ describe('Store owners list', () => { {children}, ]; - act(() => legacyRender(, document.createElement('div'))); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¾ @@ -122,14 +123,7 @@ describe('Store owners list', () => { const Leaf = () =>
Leaf
; const Intermediate = ({children}) => {children}; - const container = document.createElement('div'); - - act(() => - legacyRender( - , - container, - ), - ); + act(() => render()); const rootID = store.getElementIDAtIndex(0); expect(store).toMatchInlineSnapshot(` @@ -145,12 +139,7 @@ describe('Store owners list', () => { " `); - act(() => - legacyRender( - , - container, - ), - ); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¾ @@ -166,12 +155,7 @@ describe('Store owners list', () => { " `); - act(() => - legacyRender( - , - container, - ), - ); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¾ @@ -182,12 +166,7 @@ describe('Store owners list', () => { " `); - act(() => - legacyRender( - , - container, - ), - ); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] @@ -204,8 +183,7 @@ describe('Store owners list', () => { : [, , ]; const Leaf = () =>
Leaf
; - const container = document.createElement('div'); - act(() => legacyRender(, container)); + act(() => render()); const rootID = store.getElementIDAtIndex(0); expect(store).toMatchInlineSnapshot(` @@ -222,7 +200,7 @@ describe('Store owners list', () => { " `); - act(() => legacyRender(, container)); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¾ From 761d5750ce51cec527a311a36190426fbc3470db Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 5 Feb 2024 17:17:07 +0000 Subject: [PATCH 55/62] chore: use versioned render in profilerChangeDescriptions test (#28221) --- .../src/__tests__/profilerChangeDescriptions-test.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/profilerChangeDescriptions-test.js b/packages/react-devtools-shared/src/__tests__/profilerChangeDescriptions-test.js index 3536034d9f748..cf5304664b815 100644 --- a/packages/react-devtools-shared/src/__tests__/profilerChangeDescriptions-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilerChangeDescriptions-test.js @@ -7,9 +7,10 @@ * @flow */ +import {getVersionedRenderImplementation} from './utils'; + describe('Profiler change descriptions', () => { let React; - let legacyRender; let store; let utils; @@ -17,8 +18,6 @@ describe('Profiler change descriptions', () => { utils = require('./utils'); utils.beforeEachProfiling(); - legacyRender = utils.legacyRender; - store = global.store; store.collapseNodesByDefault = false; store.recordChangeDescriptions = true; @@ -26,6 +25,8 @@ describe('Profiler change descriptions', () => { React = require('react'); }); + const {render} = getVersionedRenderImplementation(); + // @reactVersion >=18.0 it('should identify useContext as the cause for a re-render', () => { const Context = React.createContext(0); @@ -62,10 +63,8 @@ describe('Profiler change descriptions', () => { ); }; - const container = document.createElement('div'); - utils.act(() => store.profilerStore.startProfiling()); - utils.act(() => legacyRender(, container)); + utils.act(() => render()); utils.act(() => forceUpdate()); utils.act(() => store.profilerStore.stopProfiling()); From e6979aa14299f8119bbe858d767e71b88c4c8f64 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 5 Feb 2024 17:18:32 +0000 Subject: [PATCH 56/62] chore: use versioned render in profilingCharts test (#28235) --- .../src/__tests__/profilingCharts-test.js | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/profilingCharts-test.js b/packages/react-devtools-shared/src/__tests__/profilingCharts-test.js index d59d616d67e61..41d9093feeb54 100644 --- a/packages/react-devtools-shared/src/__tests__/profilingCharts-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilingCharts-test.js @@ -9,10 +9,11 @@ import type Store from 'react-devtools-shared/src/devtools/store'; +import {getVersionedRenderImplementation} from './utils'; + describe('profiling charts', () => { let React; let Scheduler; - let legacyRender; let store: Store; let utils; @@ -20,8 +21,6 @@ describe('profiling charts', () => { utils = require('./utils'); utils.beforeEachProfiling(); - legacyRender = utils.legacyRender; - store = global.store; store.collapseNodesByDefault = false; store.recordChangeDescriptions = true; @@ -30,6 +29,8 @@ describe('profiling charts', () => { Scheduler = require('scheduler'); }); + const {render} = getVersionedRenderImplementation(); + function getFlamegraphChartData(rootID, commitIndex) { const commitTree = store.profilerStore.profilingCache.getCommitTree({ commitIndex, @@ -78,11 +79,9 @@ describe('profiling charts', () => { return null; }); - const container = document.createElement('div'); - utils.act(() => store.profilerStore.startProfiling()); - utils.act(() => legacyRender(, container)); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¾ @@ -91,7 +90,7 @@ describe('profiling charts', () => { [Memo] `); - utils.act(() => legacyRender(, container)); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¾ @@ -228,11 +227,9 @@ describe('profiling charts', () => { return null; }); - const container = document.createElement('div'); - utils.act(() => store.profilerStore.startProfiling()); - utils.act(() => legacyRender(, container)); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¾ @@ -241,7 +238,7 @@ describe('profiling charts', () => { [Memo] `); - utils.act(() => legacyRender(, container)); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¾ From 49d89b149746094c18da7f24c42b906aceb4e040 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 5 Feb 2024 17:20:05 +0000 Subject: [PATCH 57/62] chore: use versioned render in profilerStore test (#28238) --- .../src/__tests__/profilerStore-test.js | 61 ++++++------------- 1 file changed, 20 insertions(+), 41 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/profilerStore-test.js b/packages/react-devtools-shared/src/__tests__/profilerStore-test.js index e9339649359de..3b375c5d9832f 100644 --- a/packages/react-devtools-shared/src/__tests__/profilerStore-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilerStore-test.js @@ -9,10 +9,10 @@ import type Store from 'react-devtools-shared/src/devtools/store'; +import {getVersionedRenderImplementation} from './utils'; + describe('ProfilerStore', () => { let React; - let ReactDOM; - let legacyRender; let store: Store; let utils; @@ -20,16 +20,17 @@ describe('ProfilerStore', () => { utils = require('./utils'); utils.beforeEachProfiling(); - legacyRender = utils.legacyRender; - store = global.store; store.collapseNodesByDefault = false; store.recordChangeDescriptions = true; React = require('react'); - ReactDOM = require('react-dom'); }); + const {render, unmount} = getVersionedRenderImplementation(); + const {render: renderOther, unmount: unmountOther} = + getVersionedRenderImplementation(); + // @reactVersion >= 16.9 it('should not remove profiling data when roots are unmounted', async () => { const Parent = ({count}) => @@ -38,19 +39,16 @@ describe('ProfilerStore', () => { .map((_, index) => ); const Child = () =>
Hi!
; - const containerA = document.createElement('div'); - const containerB = document.createElement('div'); - utils.act(() => { - legacyRender(, containerA); - legacyRender(, containerB); + render(); + renderOther(); }); utils.act(() => store.profilerStore.startProfiling()); utils.act(() => { - legacyRender(, containerA); - legacyRender(, containerB); + render(); + renderOther(); }); utils.act(() => store.profilerStore.stopProfiling()); @@ -58,12 +56,10 @@ describe('ProfilerStore', () => { const rootA = store.roots[0]; const rootB = store.roots[1]; - utils.act(() => ReactDOM.unmountComponentAtNode(containerB)); - + utils.act(() => unmountOther()); expect(store.profilerStore.getDataForRoot(rootA)).not.toBeNull(); - utils.act(() => ReactDOM.unmountComponentAtNode(containerA)); - + utils.act(() => unmount()); expect(store.profilerStore.getDataForRoot(rootB)).not.toBeNull(); }); @@ -95,14 +91,9 @@ describe('ProfilerStore', () => { return ; }; - const container = document.createElement('div'); - - // This element has to be in the for the event system to work. - document.body.appendChild(container); - // It's important that this test uses legacy sync mode. // The root API does not trigger this particular failing case. - legacyRender(, container); + utils.act(() => render()); utils.act(() => store.profilerStore.startProfiling()); @@ -148,14 +139,9 @@ describe('ProfilerStore', () => { return ; }; - const container = document.createElement('div'); - - // This element has to be in the for the event system to work. - document.body.appendChild(container); - // It's important that this test uses legacy sync mode. // The root API does not trigger this particular failing case. - legacyRender(, container); + utils.act(() => render()); expect(commitCount).toBe(1); commitCount = 0; @@ -164,10 +150,10 @@ describe('ProfilerStore', () => { // Focus and blur. const target = inputRef.current; - target.focus(); - target.blur(); - target.focus(); - target.blur(); + utils.act(() => target.focus()); + utils.act(() => target.blur()); + utils.act(() => target.focus()); + utils.act(() => target.blur()); expect(commitCount).toBe(1); utils.act(() => store.profilerStore.stopProfiling()); @@ -204,14 +190,9 @@ describe('ProfilerStore', () => { return state.hasOwnProperty; }; - const container = document.createElement('div'); - - // This element has to be in the for the event system to work. - document.body.appendChild(container); - // It's important that this test uses legacy sync mode. // The root API does not trigger this particular failing case. - legacyRender(, container); + utils.act(() => render()); utils.act(() => store.profilerStore.startProfiling()); utils.act(() => @@ -243,9 +224,7 @@ describe('ProfilerStore', () => { ); }; - const container = document.createElement('div'); - - utils.act(() => legacyRender(, container)); + utils.act(() => render()); utils.act(() => store.profilerStore.startProfiling()); }); }); From 23f318f23fdfa387aae673e158c618995513aa14 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 5 Feb 2024 17:24:22 +0000 Subject: [PATCH 58/62] chore: use versioned render in store test (#28244) --- .../src/__tests__/store-test.js | 341 +++++++++++------- 1 file changed, 207 insertions(+), 134 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index c277f6f167d06..dec3c41c8f1b6 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -7,6 +7,8 @@ * @flow */ +import {getVersionedRenderImplementation} from './utils'; + describe('Store', () => { let React; let ReactDOM; @@ -37,13 +39,13 @@ describe('Store', () => { withErrorsOrWarningsIgnored = utils.withErrorsOrWarningsIgnored; }); + const {render, unmount, createContainer} = getVersionedRenderImplementation(); + // @reactVersion >= 18.0 it('should not allow a root node to be collapsed', () => { const Component = () =>
Hi
; - act(() => - legacyRender(, document.createElement('div')), - ); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] @@ -62,15 +64,13 @@ describe('Store', () => { it('should properly handle a root with no visible nodes', () => { const Root = ({children}) => children; - const container = document.createElement('div'); - - act(() => legacyRender({null}, container)); + act(() => render({null})); expect(store).toMatchInlineSnapshot(` [root] `); - act(() => legacyRender(
, container)); + act(() => render(
)); expect(store).toMatchInlineSnapshot(`[root]`); }); @@ -93,17 +93,14 @@ describe('Store', () => { }; const Child = () => null; - const container = document.createElement('div'); - act(() => - legacyRender( + render( <> , - container, ), ); expect(store).toMatchInlineSnapshot(` @@ -119,9 +116,7 @@ describe('Store', () => { const Component = () => null; Component.displayName = '🟩💜🔵'; - const container = document.createElement('div'); - - act(() => legacyRender(, container)); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] <🟩💜🔵> @@ -196,9 +191,7 @@ describe('Store', () => { new Array(count).fill(true).map((_, index) => ); const Child = () =>
Hi!
; - const container = document.createElement('div'); - - act(() => legacyRender(, container)); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¾ @@ -214,7 +207,7 @@ describe('Store', () => { `); - act(() => legacyRender(, container)); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¾ @@ -226,12 +219,13 @@ describe('Store', () => { `); - act(() => ReactDOM.unmountComponentAtNode(container)); + act(() => unmount()); expect(store).toMatchInlineSnapshot(``); }); // @reactVersion >= 18.0 - it('should support mount and update operations for multiple roots', () => { + // @reactVersion < 19 + it('should support mount and update operations for multiple roots (legacy render)', () => { const Parent = ({count}) => new Array(count).fill(true).map((_, index) => ); const Child = () =>
Hi!
; @@ -285,6 +279,64 @@ describe('Store', () => { expect(store).toMatchInlineSnapshot(``); }); + // @reactVersion >= 18.0 + it('should support mount and update operations for multiple roots (createRoot)', () => { + const Parent = ({count}) => + new Array(count).fill(true).map((_, index) => ); + const Child = () =>
Hi!
; + + const containerA = document.createElement('div'); + const containerB = document.createElement('div'); + + const rootA = ReactDOMClient.createRoot(containerA); + const rootB = ReactDOMClient.createRoot(containerB); + + act(() => { + rootA.render(); + rootB.render(); + }); + expect(store).toMatchInlineSnapshot(` + [root] + â–¾ + + + + [root] + â–¾ + + + `); + + act(() => { + rootA.render(); + rootB.render(); + }); + expect(store).toMatchInlineSnapshot(` + [root] + â–¾ + + + + + [root] + â–¾ + + `); + + act(() => rootB.unmount()); + expect(store).toMatchInlineSnapshot(` + [root] + â–¾ + + + + + `); + + act(() => rootA.unmount()); + expect(store).toMatchInlineSnapshot(``); + }); + // @reactVersion >= 18.0 it('should filter DOM nodes from the store tree', () => { const Grandparent = () => ( @@ -302,9 +354,7 @@ describe('Store', () => { ); const Child = () =>
Hi!
; - act(() => - legacyRender(, document.createElement('div')), - ); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¾ @@ -337,8 +387,7 @@ describe('Store', () => { ); - const container = document.createElement('div'); - act(() => legacyRender(, container)); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¾ @@ -348,7 +397,7 @@ describe('Store', () => { `); act(() => { - legacyRender(, container); + render(); }); expect(store).toMatchInlineSnapshot(` [root] @@ -399,15 +448,13 @@ describe('Store', () => { ); - const container = document.createElement('div'); act(() => - legacyRender( + render( , - container, ), ); expect(store).toMatchInlineSnapshot(` @@ -425,13 +472,12 @@ describe('Store', () => { `); act(() => - legacyRender( + render( , - container, ), ); expect(store).toMatchInlineSnapshot(` @@ -449,13 +495,12 @@ describe('Store', () => { `); act(() => - legacyRender( + render( , - container, ), ); expect(store).toMatchInlineSnapshot(` @@ -473,13 +518,12 @@ describe('Store', () => { `); act(() => - legacyRender( + render( , - container, ), ); expect(store).toMatchInlineSnapshot(` @@ -497,13 +541,12 @@ describe('Store', () => { `); act(() => - legacyRender( + render( , - container, ), ); expect(store).toMatchInlineSnapshot(` @@ -514,13 +557,12 @@ describe('Store', () => { `); act(() => - legacyRender( + render( , - container, ), ); expect(store).toMatchInlineSnapshot(` @@ -538,13 +580,12 @@ describe('Store', () => { `); act(() => - legacyRender( + render( , - container, ), ); expect(store).toMatchInlineSnapshot(` @@ -599,13 +640,12 @@ describe('Store', () => { `); act(() => - legacyRender( + render( , - container, ), ); expect(store).toMatchInlineSnapshot(` @@ -658,13 +698,12 @@ describe('Store', () => { `); act(() => - legacyRender( + render( , - container, ), ); expect(store).toMatchInlineSnapshot(` @@ -743,9 +782,7 @@ describe('Store', () => { new Array(count).fill(true).map((_, index) => ); const Child = () =>
Hi!
; - act(() => - legacyRender(, document.createElement('div')), - ); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¾ @@ -816,9 +853,7 @@ describe('Store', () => { const foo = ; const bar = ; - const container = document.createElement('div'); - - act(() => legacyRender({[foo, bar]}, container)); + act(() => render({[foo, bar]})); expect(store).toMatchInlineSnapshot(` [root] â–¾ @@ -829,7 +864,7 @@ describe('Store', () => { `); - act(() => legacyRender({[bar, foo]}, container)); + act(() => render({[bar, foo]})); expect(store).toMatchInlineSnapshot(` [root] â–¾ @@ -870,15 +905,12 @@ describe('Store', () => { new Array(count).fill(true).map((_, index) => ); const Child = () =>
Hi!
; - const container = document.createElement('div'); - act(() => - legacyRender( + render( , - container, ), ); expect(store).toMatchInlineSnapshot(` @@ -888,12 +920,11 @@ describe('Store', () => { `); act(() => - legacyRender( + render( , - container, ), ); expect(store).toMatchInlineSnapshot(` @@ -902,12 +933,13 @@ describe('Store', () => { â–¸ `); - act(() => ReactDOM.unmountComponentAtNode(container)); + act(() => unmount()); expect(store).toMatchInlineSnapshot(``); }); // @reactVersion >= 18.0 - it('should support mount and update operations for multiple roots', () => { + // @reactVersion < 19 + it('should support mount and update operations for multiple roots (legacy render)', () => { const Parent = ({count}) => new Array(count).fill(true).map((_, index) => ); const Child = () =>
Hi!
; @@ -947,6 +979,50 @@ describe('Store', () => { expect(store).toMatchInlineSnapshot(``); }); + // @reactVersion >= 18.0 + it('should support mount and update operations for multiple roots (createRoot)', () => { + const Parent = ({count}) => + new Array(count).fill(true).map((_, index) => ); + const Child = () =>
Hi!
; + + const containerA = document.createElement('div'); + const containerB = document.createElement('div'); + + const rootA = ReactDOMClient.createRoot(containerA); + const rootB = ReactDOMClient.createRoot(containerB); + + act(() => { + rootA.render(); + rootB.render(); + }); + expect(store).toMatchInlineSnapshot(` + [root] + â–¸ + [root] + â–¸ + `); + + act(() => { + rootA.render(); + rootB.render(); + }); + expect(store).toMatchInlineSnapshot(` + [root] + â–¸ + [root] + â–¸ + `); + + act(() => rootB.unmount()); + expect(store).toMatchInlineSnapshot(` + [root] + â–¸ + `); + + act(() => rootA.unmount()); + expect(store).toMatchInlineSnapshot(``); + }); + // @reactVersion >= 18.0 it('should filter DOM nodes from the store tree', () => { const Grandparent = () => ( @@ -964,9 +1040,7 @@ describe('Store', () => { ); const Child = () =>
Hi!
; - act(() => - legacyRender(, document.createElement('div')), - ); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¸ @@ -1012,8 +1086,7 @@ describe('Store', () => { ); - const container = document.createElement('div'); - act(() => legacyRender(, container)); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] â–¸ @@ -1031,7 +1104,7 @@ describe('Store', () => { `); act(() => { - legacyRender(, container); + render(); }); expect(store).toMatchInlineSnapshot(` [root] @@ -1054,9 +1127,7 @@ describe('Store', () => { new Array(count).fill(true).map((_, index) => ); const Child = () =>
Hi!
; - act(() => - legacyRender(, document.createElement('div')), - ); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] ▸ @@ -1136,12 +1207,7 @@ describe('Store', () => { const ref = React.createRef(); - act(() => - legacyRender( - , - document.createElement('div'), - ), - ); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] ▸ @@ -1207,15 +1273,13 @@ describe('Store', () => { const foo = ; const bar = ; - const container = document.createElement('div'); - - act(() => legacyRender({[foo, bar]}, container)); + act(() => render({[foo, bar]})); expect(store).toMatchInlineSnapshot(` [root] ▸ `); - act(() => legacyRender({[bar, foo]}, container)); + act(() => render({[bar, foo]})); expect(store).toMatchInlineSnapshot(` [root] ▸ @@ -1264,7 +1328,7 @@ describe('Store', () => { const Parent = () => ; const Child = () => null; - act(() => legacyRender(, document.createElement('div'))); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] ▸ @@ -1328,7 +1392,7 @@ describe('Store', () => { const Parent = () => ; const Child = () => null; - act(() => legacyRender(, document.createElement('div'))); + act(() => render()); for (let i = 0; i < store.numElements; i++) { expect(store.getIndexOfElementID(store.getElementIDAtIndex(i))).toBe(i); @@ -1342,8 +1406,8 @@ describe('Store', () => { const Child = () => null; act(() => { - legacyRender(, document.createElement('div')); - legacyRender(, document.createElement('div')); + render(); + render(); }); for (let i = 0; i < store.numElements; i++) { @@ -1358,12 +1422,11 @@ describe('Store', () => { const Child = () => null; act(() => - legacyRender( + render( , - document.createElement('div'), ), ); @@ -1379,19 +1442,20 @@ describe('Store', () => { const Child = () => null; act(() => { - legacyRender( + render( , - document.createElement('div'), ); - legacyRender( + + createContainer(); + + render( , - document.createElement('div'), ); }); @@ -1402,7 +1466,8 @@ describe('Store', () => { }); // @reactVersion >= 18.0 - it('detects and updates profiling support based on the attached roots', () => { + // @reactVersion < 19 + it('detects and updates profiling support based on the attached roots (legacy render)', () => { const Component = () => null; const containerA = document.createElement('div'); @@ -1421,6 +1486,29 @@ describe('Store', () => { expect(store.rootSupportsBasicProfiling).toBe(false); }); + // @reactVersion >= 18 + it('detects and updates profiling support based on the attached roots (createRoot)', () => { + const Component = () => null; + + const containerA = document.createElement('div'); + const containerB = document.createElement('div'); + + const rootA = ReactDOMClient.createRoot(containerA); + const rootB = ReactDOMClient.createRoot(containerB); + + expect(store.rootSupportsBasicProfiling).toBe(false); + + act(() => rootA.render()); + expect(store.rootSupportsBasicProfiling).toBe(true); + + act(() => rootB.render()); + act(() => rootA.unmount()); + expect(store.rootSupportsBasicProfiling).toBe(true); + + act(() => rootB.unmount()); + expect(store.rootSupportsBasicProfiling).toBe(false); + }); + // @reactVersion >= 18.0 it('should properly serialize non-string key values', () => { const Child = () => null; @@ -1429,7 +1517,7 @@ describe('Store', () => { // This is pretty hacky. const fauxElement = Object.assign({}, , {key: 123}); - act(() => legacyRender([fauxElement], document.createElement('div'))); + act(() => render([fauxElement])); expect(store).toMatchInlineSnapshot(` [root] @@ -1488,15 +1576,13 @@ describe('Store', () => { ); - const container = document.createElement('div'); - // Render once to start fetching the lazy component - act(() => legacyRender(, container)); + act(() => render()); await Promise.resolve(); // Render again after it resolves - act(() => legacyRender(, container)); + act(() => render()); expect(store).toMatchInlineSnapshot(` [root] @@ -1543,6 +1629,7 @@ describe('Store', () => { }); // @reactVersion >= 18.0 + // @reactVersion < 19 it('should support Lazy components (legacy render)', async () => { const container = document.createElement('div'); @@ -1612,6 +1699,7 @@ describe('Store', () => { }); // @reactVersion >= 18.0 + // @reactVersion < 19 it('should support Lazy components that are unmounted before they finish loading (legacy render)', async () => { const container = document.createElement('div'); @@ -1634,6 +1722,7 @@ describe('Store', () => { }); // @reactVersion >= 18.0 + // @reactVersion < 19 it('should support Lazy components that are unmounted before they finish loading in (createRoot)', async () => { const container = document.createElement('div'); const root = ReactDOMClient.createRoot(container); @@ -1665,10 +1754,9 @@ describe('Store', () => { console.warn('test-only: render warning'); return null; } - const container = document.createElement('div'); withErrorsOrWarningsIgnored(['test-only:'], () => { - act(() => legacyRender(, container)); + act(() => render()); }); expect(store).toMatchInlineSnapshot(` @@ -1678,7 +1766,7 @@ describe('Store', () => { `); withErrorsOrWarningsIgnored(['test-only:'], () => { - act(() => legacyRender(, container)); + act(() => render()); }); expect(store).toMatchInlineSnapshot(` @@ -1697,10 +1785,9 @@ describe('Store', () => { }); return null; } - const container = document.createElement('div'); withErrorsOrWarningsIgnored(['test-only:'], () => { - act(() => legacyRender(, container)); + act(() => render()); }); expect(store).toMatchInlineSnapshot(` @@ -1710,7 +1797,7 @@ describe('Store', () => { `); withErrorsOrWarningsIgnored(['test-only:'], () => { - act(() => legacyRender(, container)); + act(() => render()); }); expect(store).toMatchInlineSnapshot(` @@ -1739,11 +1826,10 @@ describe('Store', () => { }); return null; } - const container = document.createElement('div'); withErrorsOrWarningsIgnored(['test-only:'], () => { act(() => { - legacyRender(, container); + render(); }, false); }); flushPendingBridgeOperations(); @@ -1760,7 +1846,7 @@ describe('Store', () => { ✕⚠ `); - act(() => ReactDOM.unmountComponentAtNode(container)); + act(() => unmount()); expect(store).toMatchInlineSnapshot(``); }); @@ -1778,15 +1864,12 @@ describe('Store', () => { return null; } - const container = document.createElement('div'); - withErrorsOrWarningsIgnored(['test-only:'], () => { act(() => { - legacyRender( + render( <> , - container, ); }, false); flushPendingBridgeOperations(); @@ -1797,12 +1880,11 @@ describe('Store', () => { // Before warnings and errors have flushed, flush another commit. act(() => { - legacyRender( + render( <> , - container, ); }, false); flushPendingBridgeOperations(); @@ -1823,14 +1905,13 @@ describe('Store', () => { `); - act(() => ReactDOM.unmountComponentAtNode(container)); + act(() => unmount()); expect(store).toMatchInlineSnapshot(``); }); }); // @reactVersion >= 18.0 it('from react get counted', () => { - const container = document.createElement('div'); function Example() { return []; } @@ -1841,7 +1922,7 @@ describe('Store', () => { withErrorsOrWarningsIgnored( ['Warning: Each child in a list should have a unique "key" prop'], () => { - act(() => legacyRender(, container)); + act(() => render()); }, ); @@ -1860,15 +1941,14 @@ describe('Store', () => { console.warn('test-only: render warning'); return null; } - const container = document.createElement('div'); + withErrorsOrWarningsIgnored(['test-only:'], () => { act(() => - legacyRender( + render( , - container, ), ); }); @@ -1902,15 +1982,14 @@ describe('Store', () => { console.warn('test-only: render warning'); return null; } - const container = document.createElement('div'); + withErrorsOrWarningsIgnored(['test-only:'], () => { act(() => - legacyRender( + render( , - container, ), ); }); @@ -1948,15 +2027,14 @@ describe('Store', () => { console.warn('test-only: render warning'); return null; } - const container = document.createElement('div'); + withErrorsOrWarningsIgnored(['test-only:'], () => { act(() => - legacyRender( + render( , - container, ), ); }); @@ -2002,16 +2080,15 @@ describe('Store', () => { console.warn('test-only: render warning'); return null; } - const container = document.createElement('div'); + withErrorsOrWarningsIgnored(['test-only:'], () => { act(() => - legacyRender( + render( , - container, ), ); }); @@ -2025,12 +2102,11 @@ describe('Store', () => { withErrorsOrWarningsIgnored(['test-only:'], () => { act(() => - legacyRender( + render( , - container, ), ); }); @@ -2043,11 +2119,10 @@ describe('Store', () => { withErrorsOrWarningsIgnored(['test-only:'], () => { act(() => - legacyRender( + render( , - container, ), ); }); @@ -2058,7 +2133,7 @@ describe('Store', () => { `); withErrorsOrWarningsIgnored(['test-only:'], () => { - act(() => legacyRender(, container)); + act(() => render()); }); expect(store).toMatchInlineSnapshot(`[root]`); expect(store.errorCount).toBe(0); @@ -2086,9 +2161,7 @@ describe('Store', () => { ); } - const container = document.createElement('div'); - const root = ReactDOMClient.createRoot(container); - await actAsync(() => root.render()); + await actAsync(() => render()); expect(store).toMatchInlineSnapshot(` [root] @@ -2097,7 +2170,7 @@ describe('Store', () => { `); - await actAsync(() => root.render()); + await actAsync(() => render()); expect(store).toMatchInlineSnapshot(` [root] From 09de4b2cfe9d3aa4f90b694c243f34bc9bb6b300 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 5 Feb 2024 17:29:10 +0000 Subject: [PATCH 59/62] chore: use versioned render in treeContext test (#28245) --- .../src/__tests__/treeContext-test.js | 193 ++++++------------ 1 file changed, 63 insertions(+), 130 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/treeContext-test.js b/packages/react-devtools-shared/src/__tests__/treeContext-test.js index 38b38a47e4df3..894524762ac88 100644 --- a/packages/react-devtools-shared/src/__tests__/treeContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/treeContext-test.js @@ -15,12 +15,12 @@ import type { StateContext, } from 'react-devtools-shared/src/devtools/views/Components/TreeContext'; +import {getVersionedRenderImplementation} from './utils'; + describe('TreeListContext', () => { let React; - let ReactDOM; let TestRenderer: ReactTestRenderer; let bridge: FrontendBridge; - let legacyRender; let store: Store; let utils; let withErrorsOrWarningsIgnored; @@ -36,7 +36,6 @@ describe('TreeListContext', () => { utils = require('./utils'); utils.beforeEachProfiling(); - legacyRender = utils.legacyRender; withErrorsOrWarningsIgnored = utils.withErrorsOrWarningsIgnored; bridge = global.bridge; @@ -44,7 +43,6 @@ describe('TreeListContext', () => { store.collapseNodesByDefault = false; React = require('react'); - ReactDOM = require('react-dom'); TestRenderer = utils.requireTestRenderer(); BridgeContext = @@ -54,6 +52,8 @@ describe('TreeListContext', () => { TreeContext = require('react-devtools-shared/src/devtools/views/Components/TreeContext'); }); + const {render, unmount, createContainer} = getVersionedRenderImplementation(); + afterEach(() => { // Reset between tests dispatch = ((null: any): DispatcherContext); @@ -89,9 +89,7 @@ describe('TreeListContext', () => { ); const Child = () => null; - utils.act(() => - legacyRender(, document.createElement('div')), - ); + utils.act(() => render()); let renderer; utils.act(() => (renderer = TestRenderer.create())); @@ -215,9 +213,7 @@ describe('TreeListContext', () => { ); const Child = () => null; - utils.act(() => - legacyRender(, document.createElement('div')), - ); + utils.act(() => render()); let renderer; utils.act(() => (renderer = TestRenderer.create())); @@ -301,9 +297,7 @@ describe('TreeListContext', () => { ); const Child = () => null; - utils.act(() => - legacyRender(, document.createElement('div')), - ); + utils.act(() => render()); let renderer; utils.act(() => (renderer = TestRenderer.create())); @@ -391,16 +385,14 @@ describe('TreeListContext', () => { const Parent = props => props.children || null; const Child = () => null; - const container = document.createElement('div'); utils.act(() => - legacyRender( + render( , - container, ), ); @@ -427,11 +419,10 @@ describe('TreeListContext', () => { // Remove the child (which should auto-select the parent) await utils.actAsync(() => - legacyRender( + render( , - container, ), ); expect(state).toMatchInlineSnapshot(` @@ -441,7 +432,7 @@ describe('TreeListContext', () => { `); // Unmount the root (so that nothing is selected) - await utils.actAsync(() => ReactDOM.unmountComponentAtNode(container)); + await utils.actAsync(() => unmount()); expect(state).toMatchInlineSnapshot(``); }); @@ -459,9 +450,7 @@ describe('TreeListContext', () => { .map((_, index) => ); const Child = () => null; - utils.act(() => - legacyRender(, document.createElement('div')), - ); + utils.act(() => render()); let renderer; utils.act(() => (renderer = TestRenderer.create())); @@ -620,9 +609,7 @@ describe('TreeListContext', () => { .map((_, index) => ); const Child = () => null; - utils.act(() => - legacyRender(, document.createElement('div')), - ); + utils.act(() => render()); let renderer; utils.act(() => (renderer = TestRenderer.create())); @@ -920,14 +907,13 @@ describe('TreeListContext', () => { Qux.displayName = `withHOC(${Qux.name})`; utils.act(() => - legacyRender( + render( , - document.createElement('div'), ), ); @@ -992,14 +978,13 @@ describe('TreeListContext', () => { const Baz = () => null; utils.act(() => - legacyRender( + render( , - document.createElement('div'), ), ); @@ -1096,15 +1081,12 @@ describe('TreeListContext', () => { const Bar = () => null; const Baz = () => null; - const container = document.createElement('div'); - utils.act(() => - legacyRender( + render( , - container, ), ); @@ -1125,13 +1107,12 @@ describe('TreeListContext', () => { `); await utils.actAsync(() => - legacyRender( + render( , - container, ), ); utils.act(() => renderer.update()); @@ -1157,16 +1138,13 @@ describe('TreeListContext', () => { const Bar = () => null; const Baz = () => null; - const container = document.createElement('div'); - utils.act(() => - legacyRender( + render( , - container, ), ); @@ -1198,12 +1176,11 @@ describe('TreeListContext', () => { `); await utils.actAsync(() => - legacyRender( + render( , - container, ), ); utils.act(() => renderer.update()); @@ -1243,9 +1220,7 @@ describe('TreeListContext', () => { ); const Child = () => null; - utils.act(() => - legacyRender(, document.createElement('div')), - ); + utils.act(() => render()); let renderer; utils.act(() => (renderer = TestRenderer.create())); @@ -1284,8 +1259,7 @@ describe('TreeListContext', () => { new Array(count).fill(true).map((_, index) => ); const Child = () => null; - const container = document.createElement('div'); - utils.act(() => legacyRender(, container)); + utils.act(() => render()); let renderer; utils.act(() => (renderer = TestRenderer.create())); @@ -1307,18 +1281,14 @@ describe('TreeListContext', () => { `); - await utils.actAsync(() => - legacyRender(, container), - ); + await utils.actAsync(() => render()); expect(state).toMatchInlineSnapshot(` [owners] → ▾ `); - await utils.actAsync(() => - legacyRender(, container), - ); + await utils.actAsync(() => render()); expect(state).toMatchInlineSnapshot(` [owners] → @@ -1329,13 +1299,11 @@ describe('TreeListContext', () => { const Parent = props => props.children || null; const Child = () => null; - const container = document.createElement('div'); utils.act(() => - legacyRender( + render( , - container, ), ); @@ -1355,7 +1323,7 @@ describe('TreeListContext', () => { → `); - await utils.actAsync(() => legacyRender(, container)); + await utils.actAsync(() => render()); expect(state).toMatchInlineSnapshot(` [root] → @@ -1369,7 +1337,7 @@ describe('TreeListContext', () => { → `); - await utils.actAsync(() => ReactDOM.unmountComponentAtNode(container)); + await utils.actAsync(() => unmount()); expect(state).toMatchInlineSnapshot(``); }); @@ -1387,8 +1355,7 @@ describe('TreeListContext', () => { ); - const container = document.createElement('div'); - utils.act(() => legacyRender(, container)); + utils.act(() => render()); let renderer; utils.act(() => (renderer = TestRenderer.create())); @@ -1495,13 +1462,12 @@ describe('TreeListContext', () => { it('should handle when there are no errors/warnings', () => { utils.act(() => - legacyRender( + render( , - document.createElement('div'), ), ); @@ -1558,7 +1524,7 @@ describe('TreeListContext', () => { it('should cycle through the next errors/warnings and wrap around', () => { withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( @@ -1566,7 +1532,6 @@ describe('TreeListContext', () => { , - document.createElement('div'), ), ), ); @@ -1619,7 +1584,7 @@ describe('TreeListContext', () => { it('should cycle through the previous errors/warnings and wrap around', () => { withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( @@ -1627,7 +1592,6 @@ describe('TreeListContext', () => { , - document.createElement('div'), ), ), ); @@ -1680,20 +1644,21 @@ describe('TreeListContext', () => { it('should cycle through the next errors/warnings and wrap around with multiple roots', () => { withErrorsOrWarningsIgnored(['test-only:'], () => { utils.act(() => { - legacyRender( + render( , , - document.createElement('div'), ); - legacyRender( + + createContainer(); + + render( , - document.createElement('div'), ); }); }); @@ -1750,20 +1715,21 @@ describe('TreeListContext', () => { it('should cycle through the previous errors/warnings and wrap around with multiple roots', () => { withErrorsOrWarningsIgnored(['test-only:'], () => { utils.act(() => { - legacyRender( + render( , , - document.createElement('div'), ); - legacyRender( + + createContainer(); + + render( , - document.createElement('div'), ); }); }); @@ -1820,7 +1786,7 @@ describe('TreeListContext', () => { it('should select the next or previous element relative to the current selection', () => { withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( @@ -1828,7 +1794,6 @@ describe('TreeListContext', () => { , - document.createElement('div'), ), ), ); @@ -1882,14 +1847,13 @@ describe('TreeListContext', () => { it('should update correctly when errors/warnings are cleared for a fiber in the list', () => { withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( , - document.createElement('div'), ), ), ); @@ -1955,12 +1919,11 @@ describe('TreeListContext', () => { it('should update correctly when errors/warnings are cleared for the currently selected fiber', () => { withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( , - document.createElement('div'), ), ), ); @@ -1992,18 +1955,15 @@ describe('TreeListContext', () => { }); it('should update correctly when new errors/warnings are added', () => { - const container = document.createElement('div'); - withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( , - container, ), ), ); @@ -2030,14 +1990,13 @@ describe('TreeListContext', () => { withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( , - container, ), ), ); @@ -2076,12 +2035,11 @@ describe('TreeListContext', () => { it('should update correctly when all errors/warnings are cleared', () => { withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( , - document.createElement('div'), ), ), ); @@ -2120,7 +2078,6 @@ describe('TreeListContext', () => { }); it('should update correctly when elements are added/removed', () => { - const container = document.createElement('div'); let errored = false; function ErrorOnce() { if (!errored) { @@ -2131,11 +2088,10 @@ describe('TreeListContext', () => { } withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( , - container, ), ), ); @@ -2150,12 +2106,11 @@ describe('TreeListContext', () => { withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( , - container, ), ), ); @@ -2178,7 +2133,6 @@ describe('TreeListContext', () => { }); it('should update correctly when elements are re-ordered', () => { - const container = document.createElement('div'); function ErrorOnce() { const didErrorRef = React.useRef(false); if (!didErrorRef.current) { @@ -2189,14 +2143,13 @@ describe('TreeListContext', () => { } withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( , - container, ), ), ); @@ -2227,14 +2180,13 @@ describe('TreeListContext', () => { // Re-order the tree and ensure indices are updated. withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( , - container, ), ), ); @@ -2261,14 +2213,13 @@ describe('TreeListContext', () => { // Re-order the tree and ensure indices are updated. withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( , - container, ), ), ); @@ -2289,7 +2240,7 @@ describe('TreeListContext', () => { withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( @@ -2300,7 +2251,6 @@ describe('TreeListContext', () => { , - document.createElement('div'), ), ), ); @@ -2339,7 +2289,7 @@ describe('TreeListContext', () => { withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( @@ -2350,7 +2300,6 @@ describe('TreeListContext', () => { , - document.createElement('div'), ), ), ); @@ -2425,7 +2374,7 @@ describe('TreeListContext', () => { withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( @@ -2436,7 +2385,6 @@ describe('TreeListContext', () => { , - document.createElement('div'), ), ), ); @@ -2483,12 +2431,11 @@ describe('TreeListContext', () => { withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( , - document.createElement('div'), ), ), ); @@ -2515,16 +2462,13 @@ describe('TreeListContext', () => { } const LazyComponent = React.lazy(() => fakeImport(Child)); - const container = document.createElement('div'); - withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( , - container, ), ), ); @@ -2538,12 +2482,11 @@ describe('TreeListContext', () => { await Promise.resolve(); withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( , - container, ), ), ); @@ -2565,15 +2508,12 @@ describe('TreeListContext', () => { const Fallback = () => ; - const container = document.createElement('div'); - withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( }> , - container, ), ), ); @@ -2590,11 +2530,10 @@ describe('TreeListContext', () => { await Promise.resolve(); withErrorsOrWarningsIgnored(['test-only:'], () => utils.act(() => - legacyRender( + render( }> , - container, ), ), ); @@ -2629,16 +2568,14 @@ describe('TreeListContext', () => { } } - const container = document.createElement('div'); withErrorsOrWarningsIgnored( ['test-only:', 'React will try to recreate this component tree'], () => { utils.act(() => - legacyRender( + render( , - container, ), ); }, @@ -2659,7 +2596,7 @@ describe('TreeListContext', () => { → ✕ `); - utils.act(() => ReactDOM.unmountComponentAtNode(container)); + utils.act(() => unmount()); expect(state).toMatchInlineSnapshot(``); // Should be a noop @@ -2693,16 +2630,14 @@ describe('TreeListContext', () => { } } - const container = document.createElement('div'); withErrorsOrWarningsIgnored( ['test-only:', 'React will try to recreate this component tree'], () => { utils.act(() => - legacyRender( + render( , - container, ), ); }, @@ -2723,7 +2658,7 @@ describe('TreeListContext', () => { → ✕ `); - utils.act(() => ReactDOM.unmountComponentAtNode(container)); + utils.act(() => unmount()); expect(state).toMatchInlineSnapshot(``); // Should be a noop @@ -2752,16 +2687,14 @@ describe('TreeListContext', () => { } } - const container = document.createElement('div'); withErrorsOrWarningsIgnored( ['test-only:', 'React will try to recreate this component tree'], () => { utils.act(() => - legacyRender( + render( , - container, ), ); }, From 6a21244671eda81cd93f729946f9e0ac810bb23e Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 5 Feb 2024 17:30:46 +0000 Subject: [PATCH 60/62] chore: use versioned render in editing test (#28239) --- .../src/__tests__/editing-test.js | 24 +++++++------------ 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/editing-test.js b/packages/react-devtools-shared/src/__tests__/editing-test.js index 281a2eb1836ed..cece83ba708aa 100644 --- a/packages/react-devtools-shared/src/__tests__/editing-test.js +++ b/packages/react-devtools-shared/src/__tests__/editing-test.js @@ -10,11 +10,12 @@ import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; import type Store from 'react-devtools-shared/src/devtools/store'; +import {getVersionedRenderImplementation} from './utils'; + describe('editing interface', () => { let PropTypes; let React; let bridge: FrontendBridge; - let legacyRender; let store: Store; let utils; @@ -25,8 +26,6 @@ describe('editing interface', () => { beforeEach(() => { utils = require('./utils'); - legacyRender = utils.legacyRender; - bridge = global.bridge; store = global.store; store.collapseNodesByDefault = false; @@ -36,6 +35,8 @@ describe('editing interface', () => { React = require('react'); }); + const {render} = getVersionedRenderImplementation(); + describe('props', () => { let committedClassProps; let committedFunctionProps; @@ -66,9 +67,8 @@ describe('editing interface', () => { inputRef = React.createRef(null); - const container = document.createElement('div'); await utils.actAsync(() => - legacyRender( + render( <> { , , - container, ), ); @@ -440,11 +439,9 @@ describe('editing interface', () => { } } - const container = document.createElement('div'); await utils.actAsync(() => - legacyRender( + render( , - container, ), ); @@ -662,10 +659,7 @@ describe('editing interface', () => { return null; } - const container = document.createElement('div'); - await utils.actAsync(() => - legacyRender(, container), - ); + await utils.actAsync(() => render()); hookID = 0; // index id = ((store.getElementIDAtIndex(0): any): number); @@ -917,13 +911,11 @@ describe('editing interface', () => { } } - const container = document.createElement('div'); await utils.actAsync(() => - legacyRender( + render( , - container, ), ); From 86b95edb0ae68a68f7611aca56d3139e7934fb75 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Mon, 5 Feb 2024 17:32:31 +0000 Subject: [PATCH 61/62] chore: use versioned render in ownersListContext test (#28240) --- .../src/__tests__/ownersListContext-test.js | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/ownersListContext-test.js b/packages/react-devtools-shared/src/__tests__/ownersListContext-test.js index c02a895feeaec..599492bed54a1 100644 --- a/packages/react-devtools-shared/src/__tests__/ownersListContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/ownersListContext-test.js @@ -12,11 +12,12 @@ import type {Element} from 'react-devtools-shared/src/frontend/types'; import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; import type Store from 'react-devtools-shared/src/devtools/store'; +import {getVersionedRenderImplementation} from './utils'; + describe('OwnersListContext', () => { let React; let TestRenderer: ReactTestRenderer; let bridge: FrontendBridge; - let legacyRender; let store: Store; let utils; @@ -30,8 +31,6 @@ describe('OwnersListContext', () => { utils = require('./utils'); utils.beforeEachProfiling(); - legacyRender = utils.legacyRender; - bridge = global.bridge; store = global.store; store.collapseNodesByDefault = false; @@ -51,6 +50,8 @@ describe('OwnersListContext', () => { require('react-devtools-shared/src/devtools/views/Components/TreeContext').TreeContextController; }); + const {render} = getVersionedRenderImplementation(); + const Contexts = ({children, defaultOwnerID = null}) => ( @@ -98,9 +99,7 @@ describe('OwnersListContext', () => { }; const Child = () => null; - utils.act(() => - legacyRender(, document.createElement('div')), - ); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] @@ -143,9 +142,7 @@ describe('OwnersListContext', () => { }; const Child = () => null; - utils.act(() => - legacyRender(, document.createElement('div')), - ); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] @@ -171,9 +168,7 @@ describe('OwnersListContext', () => { const Grandparent = () => ; const Parent = () => null; - utils.act(() => - legacyRender(, document.createElement('div')), - ); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] @@ -198,9 +193,7 @@ describe('OwnersListContext', () => { return ; }; - utils.act(() => - legacyRender(, document.createElement('div')), - ); + utils.act(() => render()); expect(store).toMatchInlineSnapshot(` [root] From 95ec128399a8b34884cc6bd90a041e03ce5c1844 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 5 Feb 2024 09:33:35 -0800 Subject: [PATCH 62/62] [Flight] Support Keyed Server Components (#28123) Conceptually a Server Component in the tree is the same as a Client Component. When we render a Server Component with a key, that key should be used as part of the reconciliation process to ensure the children's state are preserved when they move in a set. The key of a child should also be used to clear the state of the children when that key changes. Conversely, if a Server Component doesn't have a key it should get an implicit key based on the slot number. It should not inherit the key of its children since the children don't know if that would collide with other keys in the set the Server Component is rendered in. A Client Component also has an identity based on the function's implementation type. That mainly has to do with the state (or future state after a refactor) that Component might contain. To transfer state between two implementations it needs to be of the same state type. This is not a concern for a Server Components since they never have state so identity doesn't matter. A Component returns a set of children. If it returns a single child, that's the same as returning a fragment of one child. So if you conditionally return a single child or a fragment, they should technically reconcile against each other. The simple way to do this is to simply emit a Fragment for every Server Component. That would be correct in all cases. Unfortunately that is also unfortunate since it bloats the payload in the common cases. It also means that Fiber creates an extra indirection in the runtime. Ideally we want to fold Server Component aways into zero cost on the client. At least where possible. The common cases are that you don't specify a key on a single return child, and that you do specify a key on a Server Component in a dynamic set. The approach in this PR treats a Server Component that returns other Server Components or Lazy Nodes as a sequence that can be folded away. I.e. the parts that don't generate any output in the RSC payload. Instead, it keeps track of their keys on an internal "context". Which gets reset after each new reified JSON node gets rendered. Then we transfer the accumulated keys from any parent Server Components onto the child element. In the simple case, the child just inherits the key of the parent. If the Server Component itself is keyless but a child isn't, we have to add a wrapper fragment to ensure that this fragment gets the implicit key but we can still use the key to reset state. This is unusual though because typically if you keyed something it's because it was already in a fragment. In the case a Server Component is keyed but forks its children using a fragment, we need to key that fragment so that the whole set can move around as one. In theory this could be flattened into a parent array but that gets tricky if something suspends, because then we can't send the siblings early. The main downside of this approach is that switching between single child and fragment in a Server Component isn't always going to reconcile against each other. That's because if we saw a single child first, we'd have to add the fragment preemptively in case it forks later. This semantic of React isn't very well known anyway and it might be ok to break it here for pragmatic reasons. The tests document this discrepancy. Another compromise of this approach is that when combining keys we don't escape them fully. We instead just use a simple `,` separated concat. This is probably good enough in practice. Additionally, since we don't encode the implicit 0 index slot key, you can move things around between parents which shouldn't really reconcile but does. This keeps the keys shorter and more human readable. --- .../src/__tests__/ReactFlight-test.js | 425 ++++++++++++++++++ .../src/__tests__/ReactFlightDOMEdge-test.js | 51 ++- .../react-server/src/ReactFlightServer.js | 302 +++++++++++-- packages/shared/ReactFeatureFlags.js | 2 + .../forks/ReactFeatureFlags.native-fb.js | 2 + .../forks/ReactFeatureFlags.native-oss.js | 2 + .../forks/ReactFeatureFlags.test-renderer.js | 2 + .../ReactFeatureFlags.test-renderer.native.js | 2 + .../ReactFeatureFlags.test-renderer.www.js | 2 + .../shared/forks/ReactFeatureFlags.www.js | 2 + 10 files changed, 736 insertions(+), 56 deletions(-) diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index fc351132a3135..8e038bfc0f805 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -1700,4 +1700,429 @@ describe('ReactFlight', () => { expect(errors).toEqual([]); }); + + // @gate enableServerComponentKeys + it('preserves state when keying a server component', async () => { + function StatefulClient({name}) { + const [state] = React.useState(name.toLowerCase()); + return state; + } + const Stateful = clientReference(StatefulClient); + + function Item({item}) { + return ( +
+ {item} + +
+ ); + } + + function Items({items}) { + return items.map(item => { + return ; + }); + } + + const transport = ReactNoopFlightServer.render( + , + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport)); + }); + + expect(ReactNoop).toMatchRenderedOutput( + <> +
Aa
+
Bb
+
Cc
+ , + ); + + const transport2 = ReactNoopFlightServer.render( + , + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport2)); + }); + + expect(ReactNoop).toMatchRenderedOutput( + <> +
Bb
+
Aa
+
Dd
+
Cc
+ , + ); + }); + + // @gate enableServerComponentKeys + it('does not inherit keys of children inside a server component', async () => { + function StatefulClient({name, initial}) { + const [state] = React.useState(initial); + return state; + } + const Stateful = clientReference(StatefulClient); + + function Item({item, initial}) { + // This key is the key of the single item of this component. + // It's NOT part of the key of the list the parent component is + // in. + return ( +
+ {item} + +
+ ); + } + + function IndirectItem({item, initial}) { + // Even though we render two items with the same child key this key + // should not conflict, because the key belongs to the parent slot. + return ; + } + + // These items don't have their own keys because they're in a fixed set + const transport = ReactNoopFlightServer.render( + <> + + + + + , + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport)); + }); + + expect(ReactNoop).toMatchRenderedOutput( + <> +
A1
+
B2
+
C5
+
C6
+ , + ); + + // This means that they shouldn't swap state when the properties update + const transport2 = ReactNoopFlightServer.render( + <> + + + + + , + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport2)); + }); + + expect(ReactNoop).toMatchRenderedOutput( + <> +
B3
+
A4
+
C5
+
C6
+ , + ); + }); + + // @gate enableServerComponentKeys + it('shares state between single return and array return in a parent', async () => { + function StatefulClient({name, initial}) { + const [state] = React.useState(initial); + return state; + } + const Stateful = clientReference(StatefulClient); + + function Item({item, initial}) { + // This key is the key of the single item of this component. + // It's NOT part of the key of the list the parent component is + // in. + return ( + + {item} + + + ); + } + + function Condition({condition}) { + if (condition) { + return ; + } + // The first item in the fragment is the same as the single item. + return ( + <> + + + + ); + } + + function ConditionPlain({condition}) { + if (condition) { + return ( + + C + + + ); + } + // The first item in the fragment is the same as the single item. + return ( + <> + + C + + + + D + + + + ); + } + + const transport = ReactNoopFlightServer.render( + // This two item wrapper ensures we're already one step inside an array. + // A single item is not the same as a set when it's nested one level. + <> +
+ +
+
+ +
+
+ +
+ , + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport)); + }); + + expect(ReactNoop).toMatchRenderedOutput( + <> +
+ A1 +
+
+ C1 +
+
+ C1 +
+ , + ); + + const transport2 = ReactNoopFlightServer.render( + <> +
+ +
+
+ +
+ {null} +
+ +
+ , + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport2)); + }); + + // We're intentionally breaking from the semantics here for efficiency of the protocol. + // In the case a Server Component inside a fragment is itself implicitly keyed but its + // return value has a key, then we need a wrapper fragment. This means they can't + // reconcile. To solve this we would need to add a wrapper fragment to every Server + // Component just in case it returns a fragment later which is a lot. + expect(ReactNoop).toMatchRenderedOutput( + <> +
+ A2{/* This should be A1 ideally */} + B3 +
+
+ C1 + D3 +
+
+ C1 + D3 +
+ , + ); + }); + + it('shares state between single return and array return in a set', async () => { + function StatefulClient({name, initial}) { + const [state] = React.useState(initial); + return state; + } + const Stateful = clientReference(StatefulClient); + + function Item({item, initial}) { + // This key is the key of the single item of this component. + // It's NOT part of the key of the list the parent component is + // in. + return ( + + {item} + + + ); + } + + function Condition({condition}) { + if (condition) { + return ; + } + // The first item in the fragment is the same as the single item. + return ( + <> + + + + ); + } + + function ConditionPlain({condition}) { + if (condition) { + return ( + + C + + + ); + } + // The first item in the fragment is the same as the single item. + return ( + <> + + C + + + + D + + + + ); + } + + const transport = ReactNoopFlightServer.render( + // This two item wrapper ensures we're already one step inside an array. + // A single item is not the same as a set when it's nested one level. +
+ + + +
, + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport)); + }); + + expect(ReactNoop).toMatchRenderedOutput( +
+ A1 + C1 + C1 +
, + ); + + const transport2 = ReactNoopFlightServer.render( +
+ + + {null} + +
, + ); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport2)); + }); + + // We're intentionally breaking from the semantics here for efficiency of the protocol. + // The issue with this test scenario is that when the Server Component is in a set, + // the next slot can't be conditionally a fragment or single. That would require wrapping + // in an additional fragment for every single child just in case it every expands to a + // fragment. + expect(ReactNoop).toMatchRenderedOutput( +
+ A2{/* Should be A1 */} + B3 + C2{/* Should be C1 */} + D3 + C2{/* Should be C1 */} + D3 +
, + ); + }); + + // @gate enableServerComponentKeys + it('preserves state with keys split across async work', async () => { + let resolve; + const promise = new Promise(r => (resolve = r)); + + function StatefulClient({name}) { + const [state] = React.useState(name.toLowerCase()); + return state; + } + const Stateful = clientReference(StatefulClient); + + function Item({name}) { + if (name === 'A') { + return promise.then(() => ( +
+ {name} + +
+ )); + } + return ( +
+ {name} + +
+ ); + } + + const transport = ReactNoopFlightServer.render([ + , + null, + ]); + + // Create a gap in the stream + await resolve(); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport)); + }); + + expect(ReactNoop).toMatchRenderedOutput(
Aa
); + + const transport2 = ReactNoopFlightServer.render([ + null, + , + ]); + + await act(async () => { + ReactNoop.render(await ReactNoopFlightClient.read(transport2)); + }); + + expect(ReactNoop).toMatchRenderedOutput(
Ba
); + }); }); diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js index 5d966a16ded59..7c9271fcdcc19 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js @@ -226,20 +226,55 @@ describe('ReactFlightDOMEdge', () => { const [stream1, stream2] = passThrough(stream).tee(); const serializedContent = await readResult(stream1); + expect(serializedContent.length).toBeLessThan(400); expect(timesRendered).toBeLessThan(5); - const result = await ReactServerDOMClient.createFromReadableStream( - stream2, - { - ssrManifest: { - moduleMap: null, - moduleLoading: null, - }, + const model = await ReactServerDOMClient.createFromReadableStream(stream2, { + ssrManifest: { + moduleMap: null, + moduleLoading: null, }, + }); + + // Use the SSR render to resolve any lazy elements + const ssrStream = await ReactDOMServer.renderToReadableStream(model); + // Should still match the result when parsed + const result = await readResult(ssrStream); + expect(result).toEqual(resolvedChildren.join('')); + }); + + it('should execute repeated host components only once', async () => { + const div =
this is a long return value
; + let timesRendered = 0; + function ServerComponent() { + timesRendered++; + return div; + } + const element = ; + const children = new Array(30).fill(element); + const resolvedChildren = new Array(30).fill( + '
this is a long return value
', ); + const stream = ReactServerDOMServer.renderToReadableStream(children); + const [stream1, stream2] = passThrough(stream).tee(); + + const serializedContent = await readResult(stream1); + expect(serializedContent.length).toBeLessThan(400); + expect(timesRendered).toBeLessThan(5); + + const model = await ReactServerDOMClient.createFromReadableStream(stream2, { + ssrManifest: { + moduleMap: null, + moduleLoading: null, + }, + }); + + // Use the SSR render to resolve any lazy elements + const ssrStream = await ReactDOMServer.renderToReadableStream(model); // Should still match the result when parsed - expect(result).toEqual(resolvedChildren); + const result = await readResult(ssrStream); + expect(result).toEqual(resolvedChildren.join('')); }); it('should execute repeated server components in a compact form', async () => { diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 683cbb9feb6f7..f04642737e167 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -16,6 +16,7 @@ import { enablePostpone, enableTaint, enableServerContext, + enableServerComponentKeys, } from 'shared/ReactFeatureFlags'; import { @@ -181,6 +182,8 @@ type Task = { model: ReactClientValue, ping: () => void, toJSON: (key: string, value: ReactClientValue) => ReactJSONValue, + keyPath: null | string, // parent server component keys + implicitSlot: boolean, // true if the root server component of this sequence had a null key context: ContextSnapshot, thenableState: ThenableState | null, }; @@ -314,7 +317,14 @@ export function createRequest( }; request.pendingChunks++; const rootContext = createRootContext(context); - const rootTask = createTask(request, model, rootContext, abortSet); + const rootTask = createTask( + request, + model, + null, + false, + rootContext, + abortSet, + ); pingedTasks.push(rootTask); return request; } @@ -338,12 +348,18 @@ function createRootContext( const POP = {}; -function serializeThenable(request: Request, thenable: Thenable): number { +function serializeThenable( + request: Request, + task: Task, + thenable: Thenable, +): number { request.pendingChunks++; const newTask = createTask( request, null, - getActiveContext(), + task.keyPath, // the server component sequence continues through Promise-as-a-child. + task.implicitSlot, + task.context, request.abortableTasks, ); @@ -500,11 +516,86 @@ function createLazyWrapperAroundWakeable(wakeable: Wakeable) { return lazyType; } +function renderFragment( + request: Request, + task: Task, + children: $ReadOnlyArray, +): ReactJSONValue { + if (!enableServerComponentKeys) { + return children; + } + if (task.keyPath !== null) { + // We have a Server Component that specifies a key but we're now splitting + // the tree using a fragment. + const fragment = [ + REACT_ELEMENT_TYPE, + REACT_FRAGMENT_TYPE, + task.keyPath, + {children}, + ]; + if (!task.implicitSlot) { + // If this was keyed inside a set. I.e. the outer Server Component was keyed + // then we need to handle reorders of the whole set. To do this we need to wrap + // this array in a keyed Fragment. + return fragment; + } + // If the outer Server Component was implicit but then an inner one had a key + // we don't actually need to be able to move the whole set around. It'll always be + // in an implicit slot. The key only exists to be able to reset the state of the + // children. We could achieve the same effect by passing on the keyPath to the next + // set of components inside the fragment. This would also allow a keyless fragment + // reconcile against a single child. + // Unfortunately because of JSON.stringify, we can't call the recursive loop for + // each child within this context because we can't return a set with already resolved + // values. E.g. a string would get double encoded. Returning would pop the context. + // So instead, we wrap it with an unkeyed fragment and inner keyed fragment. + return [fragment]; + } + // Since we're yielding here, that implicitly resets the keyPath context on the + // way up. Which is what we want since we've consumed it. If this changes to + // be recursive serialization, we need to reset the keyPath and implicitSlot, + // before recursing here. + return children; +} + +function renderClientElement( + task: Task, + type: any, + key: null | string, + props: any, +): ReactJSONValue { + if (!enableServerComponentKeys) { + return [REACT_ELEMENT_TYPE, type, key, props]; + } + // We prepend the terminal client element that actually gets serialized with + // the keys of any Server Components which are not serialized. + const keyPath = task.keyPath; + if (key === null) { + key = keyPath; + } else if (keyPath !== null) { + key = keyPath + ',' + key; + } + const element = [REACT_ELEMENT_TYPE, type, key, props]; + if (task.implicitSlot && key !== null) { + // The root Server Component had no key so it was in an implicit slot. + // If we had a key lower, it would end up in that slot with an explicit key. + // We wrap the element in a fragment to give it an implicit key slot with + // an inner explicit key. + return [element]; + } + // Since we're yielding here, that implicitly resets the keyPath context on the + // way up. Which is what we want since we've consumed it. If this changes to + // be recursive serialization, we need to reset the keyPath and implicitSlot, + // before recursing here. We also need to reset it once we render into an array + // or anything else too which we also get implicitly. + return element; +} + function renderElement( request: Request, task: Task, type: any, - key: null | React$Key, + key: null | string, ref: mixed, props: any, ): ReactJSONValue { @@ -525,7 +616,7 @@ function renderElement( if (typeof type === 'function') { if (isClientReference(type)) { // This is a reference to a Client Component. - return [REACT_ELEMENT_TYPE, type, key, props]; + return renderClientElement(task, type, key, props); } // This is a server-side component. @@ -552,31 +643,52 @@ function renderElement( // the thenable here. result = createLazyWrapperAroundWakeable(result); } - return renderModelDestructive(request, task, emptyRoot, '', result); + // Track this element's key on the Server Component on the keyPath context.. + const prevKeyPath = task.keyPath; + const prevImplicitSlot = task.implicitSlot; + if (key !== null) { + // Append the key to the path. Technically a null key should really add the child + // index. We don't do that to hold the payload small and implementation simple. + task.keyPath = prevKeyPath === null ? key : prevKeyPath + ',' + key; + } else if (prevKeyPath === null) { + // This sequence of Server Components has no keys. This means that it was rendered + // in a slot that needs to assign an implicit key. Even if children below have + // explicit keys, they should not be used for the outer most key since it might + // collide with other slots in that set. + task.implicitSlot = true; + } + const json = renderModelDestructive(request, task, emptyRoot, '', result); + task.keyPath = prevKeyPath; + task.implicitSlot = prevImplicitSlot; + return json; } else if (typeof type === 'string') { // This is a host element. E.g. HTML. - return [REACT_ELEMENT_TYPE, type, key, props]; + return renderClientElement(task, type, key, props); } else if (typeof type === 'symbol') { - if (type === REACT_FRAGMENT_TYPE) { + if (type === REACT_FRAGMENT_TYPE && key === null) { // For key-less fragments, we add a small optimization to avoid serializing // it as a wrapper. - // TODO: If a key is specified, we should propagate its key to any children. - // Same as if a Server Component has a key. - return renderModelDestructive( + const prevImplicitSlot = task.implicitSlot; + if (task.keyPath === null) { + task.implicitSlot = true; + } + const json = renderModelDestructive( request, task, emptyRoot, '', props.children, ); + task.implicitSlot = prevImplicitSlot; + return json; } // This might be a built-in React component. We'll let the client decide. // Any built-in works as long as its props are serializable. - return [REACT_ELEMENT_TYPE, type, key, props]; + return renderClientElement(task, type, key, props); } else if (type != null && typeof type === 'object') { if (isClientReference(type)) { // This is a reference to a Client Component. - return [REACT_ELEMENT_TYPE, type, key, props]; + return renderClientElement(task, type, key, props); } switch (type.$$typeof) { case REACT_LAZY_TYPE: { @@ -596,7 +708,29 @@ function renderElement( prepareToUseHooksForComponent(prevThenableState); const result = render(props, undefined); - return renderModelDestructive(request, task, emptyRoot, '', result); + const prevKeyPath = task.keyPath; + const prevImplicitSlot = task.implicitSlot; + if (key !== null) { + // Append the key to the path. Technically a null key should really add the child + // index. We don't do that to hold the payload small and implementation simple. + task.keyPath = prevKeyPath === null ? key : prevKeyPath + ',' + key; + } else if (prevKeyPath === null) { + // This sequence of Server Components has no keys. This means that it was rendered + // in a slot that needs to assign an implicit key. Even if children below have + // explicit keys, they should not be used for the outer most key since it might + // collide with other slots in that set. + task.implicitSlot = true; + } + const json = renderModelDestructive( + request, + task, + emptyRoot, + '', + result, + ); + task.keyPath = prevKeyPath; + task.implicitSlot = prevImplicitSlot; + return json; } case REACT_MEMO_TYPE: { return renderElement(request, task, type.type, key, ref, props); @@ -618,13 +752,13 @@ function renderElement( ); } } - return [ - REACT_ELEMENT_TYPE, + return renderClientElement( + task, type, key, // Rely on __popProvider being serialized last to pop the provider. {value: props.value, children: props.children, __pop: POP}, - ]; + ); } // Fallthrough } @@ -647,18 +781,31 @@ function pingTask(request: Request, task: Task): void { function createTask( request: Request, model: ReactClientValue, + keyPath: null | string, + implicitSlot: boolean, context: ContextSnapshot, abortSet: Set, ): Task { const id = request.nextChunkId++; if (typeof model === 'object' && model !== null) { - // Register this model as having the ID we're about to write. - request.writtenObjects.set(model, id); + // If we're about to write this into a new task we can assign it an ID early so that + // any other references can refer to the value we're about to write. + if ( + enableServerComponentKeys && + (keyPath !== null || implicitSlot || context !== rootContextSnapshot) + ) { + // If we're in some kind of context we can't necessarily reuse this object depending + // what parent components are used. + } else { + request.writtenObjects.set(model, id); + } } const task: Task = { id, status: PENDING, model, + keyPath, + implicitSlot, context, ping: () => pingTask(request, task), toJSON: function ( @@ -855,7 +1002,9 @@ function outlineModel(request: Request, value: ReactClientValue): number { const newTask = createTask( request, value, - getActiveContext(), + null, // The way we use outlining is for reusing an object. + false, // It makes no sense for that use case to be contextual. + rootContextSnapshot, // Therefore we don't pass any contextual information along. request.abortableTasks, ); retryTask(request, newTask); @@ -988,6 +1137,8 @@ function renderModel( key: string, value: ReactClientValue, ): ReactJSONValue { + const prevKeyPath = task.keyPath; + const prevImplicitSlot = task.implicitSlot; try { return renderModelDestructive(request, task, parent, key, value); } catch (thrownValue) { @@ -1016,12 +1167,20 @@ function renderModel( const newTask = createTask( request, task.model, - getActiveContext(), + task.keyPath, + task.implicitSlot, + task.context, request.abortableTasks, ); const ping = newTask.ping; (x: any).then(ping, ping); newTask.thenableState = getThenableStateAfterSuspending(); + + // Restore the context. We assume that this will be restored by the inner + // functions in case nothing throws so we don't use "finally" here. + task.keyPath = prevKeyPath; + task.implicitSlot = prevImplicitSlot; + if (wasReactNode) { return serializeLazyID(newTask.id); } @@ -1034,12 +1193,24 @@ function renderModel( const postponeId = request.nextChunkId++; logPostpone(request, postponeInstance.message); emitPostponeChunk(request, postponeId, postponeInstance); + + // Restore the context. We assume that this will be restored by the inner + // functions in case nothing throws so we don't use "finally" here. + task.keyPath = prevKeyPath; + task.implicitSlot = prevImplicitSlot; + if (wasReactNode) { return serializeLazyID(postponeId); } return serializeByValueID(postponeId); } } + + // Restore the context. We assume that this will be restored by the inner + // functions in case nothing throws so we don't use "finally" here. + task.keyPath = prevKeyPath; + task.implicitSlot = prevImplicitSlot; + if (wasReactNode) { // Something errored. We'll still send everything we have up until this point. // We'll replace this element with a lazy reference that throws on the client @@ -1089,18 +1260,31 @@ function renderModelDestructive( const writtenObjects = request.writtenObjects; const existingId = writtenObjects.get(value); if (existingId !== undefined) { - if (existingId === -1) { - // Seen but not yet outlined. - const newId = outlineModel(request, value); - return serializeByValueID(newId); + if ( + enableServerComponentKeys && + (task.keyPath !== null || + task.implicitSlot || + task.context !== rootContextSnapshot) + ) { + // If we're in some kind of context we can't reuse the result of this render or + // previous renders of this element. We only reuse elements if they're not wrapped + // by another Server Component. } else if (modelRoot === value) { // This is the ID we're currently emitting so we need to write it // once but if we discover it again, we refer to it by id. modelRoot = null; + } else if (existingId === -1) { + // Seen but not yet outlined. + // TODO: If we throw here we can treat this as suspending which causes an outline + // but that is able to reuse the same task if we're already in one but then that + // will be a lazy future value rather than guaranteed to exist but maybe that's good. + const newId = outlineModel(request, (value: any)); + return serializeLazyID(newId); } else { - // We've already emitted this as an outlined object, so we can - // just refer to that by its existing ID. - return serializeByValueID(existingId); + // We've already emitted this as an outlined object, so we can refer to that by its + // existing ID. We use a lazy reference since, unlike plain objects, elements might + // suspend so it might not have emitted yet even if we have the ID for it. + return serializeLazyID(existingId); } } else { // This is the first time we've seen this object. We may never see it again @@ -1108,13 +1292,13 @@ function renderModelDestructive( writtenObjects.set(value, -1); } - // TODO: Concatenate keys of parents onto children. const element: React$Element = (value: any); // Attempt to render the Server Component. return renderElement( request, task, element.type, + // $FlowFixMe[incompatible-call] the key of an element is null | string element.key, element.ref, element.props, @@ -1155,7 +1339,18 @@ function renderModelDestructive( // $FlowFixMe[method-unbinding] if (typeof value.then === 'function') { if (existingId !== undefined) { - if (modelRoot === value) { + if ( + enableServerComponentKeys && + (task.keyPath !== null || + task.implicitSlot || + task.context !== rootContextSnapshot) + ) { + // If we're in some kind of context we can't reuse the result of this render or + // previous renders of this element. We only reuse Promises if they're not wrapped + // by another Server Component. + const promiseId = serializeThenable(request, task, (value: any)); + return serializePromiseID(promiseId); + } else if (modelRoot === value) { // This is the ID we're currently emitting so we need to write it // once but if we discover it again, we refer to it by id. modelRoot = null; @@ -1166,7 +1361,7 @@ function renderModelDestructive( } // We assume that any object with a .then property is a "Thenable" type, // or a Promise type. Either of which can be represented by a Promise. - const promiseId = serializeThenable(request, (value: any)); + const promiseId = serializeThenable(request, task, (value: any)); writtenObjects.set(value, promiseId); return serializePromiseID(promiseId); } @@ -1195,14 +1390,14 @@ function renderModelDestructive( } if (existingId !== undefined) { - if (existingId === -1) { - // Seen but not yet outlined. - const newId = outlineModel(request, value); - return serializeByValueID(newId); - } else if (modelRoot === value) { + if (modelRoot === value) { // This is the ID we're currently emitting so we need to write it // once but if we discover it again, we refer to it by id. modelRoot = null; + } else if (existingId === -1) { + // Seen but not yet outlined. + const newId = outlineModel(request, (value: any)); + return serializeByValueID(newId); } else { // We've already emitted this as an outlined object, so we can // just refer to that by its existing ID. @@ -1215,8 +1410,7 @@ function renderModelDestructive( } if (isArray(value)) { - // $FlowFixMe[incompatible-return] - return value; + return renderFragment(request, task, value); } if (value instanceof Map) { @@ -1282,7 +1476,7 @@ function renderModelDestructive( const iteratorFn = getIteratorFn(value); if (iteratorFn) { - return Array.from((value: any)); + return renderFragment(request, task, Array.from((value: any))); } // Verify that this is a simple plain object. @@ -1582,6 +1776,7 @@ function retryTask(request: Request, task: Task): void { return; } + const prevContext = getActiveContext(); switchContext(task.context); try { // Track the root so we know that we have to emit this object even though it @@ -1602,15 +1797,22 @@ function retryTask(request: Request, task: Task): void { // Track the root again for the resolved object. modelRoot = resolvedModel; - // If the value is a string, it means it's a terminal value adn we already escaped it - // We don't need to escape it again so it's not passed the toJSON replacer. - // Object might contain unresolved values like additional elements. - // This is simulating what the JSON loop would do if this was part of it. - // $FlowFixMe[incompatible-type] stringify can return null - const json: string = - typeof resolvedModel === 'string' - ? stringify(resolvedModel) - : stringify(resolvedModel, task.toJSON); + // The keyPath resets at any terminal child node. + task.keyPath = null; + task.implicitSlot = false; + + let json: string; + if (typeof resolvedModel === 'object' && resolvedModel !== null) { + // Object might contain unresolved values like additional elements. + // This is simulating what the JSON loop would do if this was part of it. + // $FlowFixMe[incompatible-type] stringify can return null for undefined but we never do + json = stringify(resolvedModel, task.toJSON); + } else { + // If the value is a string, it means it's a terminal value and we already escaped it + // We don't need to escape it again so it's not passed the toJSON replacer. + // $FlowFixMe[incompatible-type] stringify can return null for undefined but we never do + json = stringify(resolvedModel); + } emitModelChunk(request, task.id, json); request.abortableTasks.delete(task); @@ -1646,6 +1848,10 @@ function retryTask(request: Request, task: Task): void { task.status = ERRORED; const digest = logRecoverableError(request, x); emitErrorChunk(request, task.id, digest, x); + } finally { + if (enableServerContext) { + switchContext(prevContext); + } } } diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 845ff9645a838..332e7f7bdff27 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -15,6 +15,8 @@ export const enableComponentStackLocations = true; +export const enableServerComponentKeys = __EXPERIMENTAL__; + // ----------------------------------------------------------------------------- // Killswitch // diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index d98e4e508101f..2b314b6ff2a07 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -93,5 +93,7 @@ export const enableFizzExternalRuntime = false; export const enableAsyncActions = false; export const enableUseDeferredValueInitialArg = true; +export const enableServerComponentKeys = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index a8fb8c9ad740e..9bf741d74a278 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -85,5 +85,7 @@ export const useMicrotasksForSchedulingInFabric = false; export const passChildrenWhenCloningPersistedNodes = false; export const enableUseDeferredValueInitialArg = __EXPERIMENTAL__; +export const enableServerComponentKeys = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index fa019f07600d3..3dcf66f535a9c 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -85,5 +85,7 @@ export const useMicrotasksForSchedulingInFabric = false; export const passChildrenWhenCloningPersistedNodes = false; export const enableUseDeferredValueInitialArg = __EXPERIMENTAL__; +export const enableServerComponentKeys = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index 347ff62abf9af..a0eaf1f80966d 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -82,5 +82,7 @@ export const useMicrotasksForSchedulingInFabric = false; export const passChildrenWhenCloningPersistedNodes = false; export const enableUseDeferredValueInitialArg = __EXPERIMENTAL__; +export const enableServerComponentKeys = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 236d46b4a5756..e97d010255afa 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -85,5 +85,7 @@ export const useMicrotasksForSchedulingInFabric = false; export const passChildrenWhenCloningPersistedNodes = false; export const enableUseDeferredValueInitialArg = true; +export const enableServerComponentKeys = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 15f5b993c8254..f573a75b5c588 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -112,5 +112,7 @@ export const passChildrenWhenCloningPersistedNodes = false; export const enableAsyncDebugInfo = false; +export const enableServerComponentKeys = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType);