From c46fc90b670fb93e9a5558a36d0367cc735f812e Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 15 Feb 2024 13:14:57 -0500 Subject: [PATCH] Pass `ref` as normal prop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Changes the behavior of the JSX runtime to pass through `ref` as a normal prop, rather than plucking it from the props object and storing on the element. This is a breaking change since it changes the type receiving component. However, most code is unaffected since it's unlikely that a component would have attempted to access a `ref` prop, since it was not possible to get a reference to one. `forwardRef` _will_ still pluck `ref` from the props object, though, since it's extremely common for users to spread the props object onto the inner component and pass `ref` as a differently named prop. This is for maximum compatibility with existing code — the real impact of this change is that `forwardRef` is no longer required. Currently, refs are resolved during child reconciliation and stored on the fiber. As a result of this change, we can move ref resolution to happen only much later, and only for components that actually use them. Then we can remove the `ref` field from the Fiber type. I have not yet done that in this step, though. --- packages/jest-react/src/JestReact.js | 56 ++++--- .../react-client/src/ReactFlightClient.js | 44 ++++-- .../__tests__/legacy/storeLegacy-v15-test.js | 79 +++++----- .../src/client/ReactDOMComponent.js | 9 +- .../src/server/ReactFizzConfigDOM.js | 4 + .../src/shared/ReactDOMUnknownPropertyHook.js | 3 +- .../__tests__/ReactCompositeComponent-test.js | 38 ++++- .../ReactDeprecationWarnings-test.js | 2 + .../__tests__/ReactFunctionComponent-test.js | 6 +- packages/react-dom/src/__tests__/refs-test.js | 25 +--- .../src/createReactNoop.js | 54 +++---- .../react-reconciler/src/ReactChildFiber.js | 15 +- .../src/ReactFiberBeginWork.js | 25 +++- .../src/__tests__/ReactFiberRefs-test.js | 6 +- .../ReactIncrementalSideEffects-test.js | 26 ++-- .../src/__tests__/ReactLazy-test.internal.js | 1 + .../src/__tests__/ReactMemo-test.js | 2 + .../src/__tests__/ReactFresh-test.js | 9 +- packages/react-server/src/ReactFizzServer.js | 37 ++++- .../react-server/src/ReactFlightServer.js | 19 ++- .../ReactTestRenderer-test.internal.js | 14 +- .../src/__tests__/ReactCreateElement-test.js | 66 +++++++-- .../src/__tests__/ReactElementClone-test.js | 32 +++- .../ReactJSXElementValidator-test.js | 12 +- .../src/__tests__/ReactJSXRuntime-test.js | 1 + .../ReactJSXTransformIntegration-test.js | 47 +++++- packages/react/src/jsx/ReactJSXElement.js | 137 ++++++++++-------- scripts/jest/TestFlags.js | 13 +- 28 files changed, 534 insertions(+), 248 deletions(-) diff --git a/packages/jest-react/src/JestReact.js b/packages/jest-react/src/JestReact.js index b660ad49f94b1..47858bf7ef071 100644 --- a/packages/jest-react/src/JestReact.js +++ b/packages/jest-react/src/JestReact.js @@ -6,6 +6,7 @@ */ import {REACT_ELEMENT_TYPE, REACT_FRAGMENT_TYPE} from 'shared/ReactSymbols'; +import {enableRefAsProp} from 'shared/ReactFeatureFlags'; import isArray from 'shared/isArray'; @@ -38,6 +39,29 @@ function assertYieldsWereCleared(root) { } } +function createJSXElementForTestComparison(type, props) { + if (enableRefAsProp) { + return { + $$typeof: REACT_ELEMENT_TYPE, + type: type, + key: null, + props: props, + _owner: null, + _store: __DEV__ ? {} : undefined, + }; + } else { + return { + $$typeof: REACT_ELEMENT_TYPE, + type: type, + key: null, + ref: null, + props: props, + _owner: null, + _store: __DEV__ ? {} : undefined, + }; + } +} + export function unstable_toMatchRenderedOutput(root, expectedJSX) { assertYieldsWereCleared(root); const actualJSON = root.toJSON(); @@ -55,17 +79,9 @@ export function unstable_toMatchRenderedOutput(root, expectedJSX) { if (actualJSXChildren === null || typeof actualJSXChildren === 'string') { actualJSX = actualJSXChildren; } else { - actualJSX = { - $$typeof: REACT_ELEMENT_TYPE, - type: REACT_FRAGMENT_TYPE, - key: null, - ref: null, - props: { - children: actualJSXChildren, - }, - _owner: null, - _store: __DEV__ ? {} : undefined, - }; + actualJSX = createJSXElementForTestComparison(REACT_FRAGMENT_TYPE, { + children: actualJSXChildren, + }); } } } else { @@ -82,18 +98,12 @@ function jsonChildToJSXChild(jsonChild) { return jsonChild; } else { const jsxChildren = jsonChildrenToJSXChildren(jsonChild.children); - return { - $$typeof: REACT_ELEMENT_TYPE, - type: jsonChild.type, - key: null, - ref: null, - props: - jsxChildren === null - ? jsonChild.props - : {...jsonChild.props, children: jsxChildren}, - _owner: null, - _store: __DEV__ ? {} : undefined, - }; + return createJSXElementForTestComparison( + jsonChild.type, + jsxChildren === null + ? jsonChild.props + : {...jsonChild.props, children: jsxChildren}, + ); } } diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 8043e76414f8c..054de12e6fd19 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -35,7 +35,11 @@ import type { import type {Postpone} from 'react/src/ReactPostpone'; -import {enableBinaryFlight, enablePostpone} from 'shared/ReactFeatureFlags'; +import { + enableBinaryFlight, + enablePostpone, + enableRefAsProp, +} from 'shared/ReactFeatureFlags'; import { resolveClientReference, @@ -468,19 +472,31 @@ function createElement( key: mixed, props: mixed, ): React$Element { - const element: any = { - // This tag allows us to uniquely identify this as a React Element - $$typeof: REACT_ELEMENT_TYPE, - - // Built-in properties that belong on the element - type: type, - key: key, - ref: null, - props: props, - - // Record the component responsible for creating this element. - _owner: null, - }; + const element: any = enableRefAsProp + ? { + // This tag allows us to uniquely identify this as a React Element + $$typeof: REACT_ELEMENT_TYPE, + + // Built-in properties that belong on the element + type, + key, + props, + + // Record the component responsible for creating this element. + _owner: null, + } + : { + // Old behavior. When enableRefAsProp is off, `ref` is an extra field. + ref: null, + + // Everything else is the same. + $$typeof: REACT_ELEMENT_TYPE, + type, + key, + props, + _owner: null, + }; + if (__DEV__) { // We don't really need to add any of these but keeping them for good measure. // Unfortunately, _store is enumerable in jest matchers so for equality to diff --git a/packages/react-devtools-shared/src/__tests__/legacy/storeLegacy-v15-test.js b/packages/react-devtools-shared/src/__tests__/legacy/storeLegacy-v15-test.js index 899678ccdc0a4..3c24be0093d63 100644 --- a/packages/react-devtools-shared/src/__tests__/legacy/storeLegacy-v15-test.js +++ b/packages/react-devtools-shared/src/__tests__/legacy/storeLegacy-v15-test.js @@ -753,37 +753,43 @@ describe('Store (legacy)', () => { `); }); - it('should support expanding deep parts of the tree', () => { - const Wrapper = ({forwardedRef}) => ( - - ); - const Nested = ({depth, forwardedRef}) => - depth > 0 ? ( - - ) : ( -
+ // TODO: These tests don't work when enableRefAsProp is on because the + // JSX runtime that's injected into the test environment by the compiler + // is not compatible with older versions of React. Need to configure the + // the test environment in such a way that certain test modules like this + // one can use an older transform. + if (!require('shared/ReactFeatureFlags').enableRefAsProp) { + it('should support expanding deep parts of the tree', () => { + const Wrapper = ({forwardedRef}) => ( + ); - - let ref = null; - const refSetter = value => { - ref = value; - }; - - act(() => - ReactDOM.render( - , - document.createElement('div'), - ), - ); - expect(store).toMatchInlineSnapshot(` + const Nested = ({depth, forwardedRef}) => + depth > 0 ? ( + + ) : ( +
+ ); + + let ref = null; + const refSetter = value => { + ref = value; + }; + + act(() => + ReactDOM.render( + , + document.createElement('div'), + ), + ); + expect(store).toMatchInlineSnapshot(` [root] ▸ `); - const deepestedNodeID = global.agent.getIDForNode(ref); + const deepestedNodeID = global.agent.getIDForNode(ref); - act(() => store.toggleIsCollapsed(deepestedNodeID, false)); - expect(store).toMatchInlineSnapshot(` + act(() => store.toggleIsCollapsed(deepestedNodeID, false)); + expect(store).toMatchInlineSnapshot(` [root] ▾ ▾ @@ -793,16 +799,16 @@ describe('Store (legacy)', () => {
`); - const rootID = store.getElementIDAtIndex(0); + const rootID = store.getElementIDAtIndex(0); - act(() => store.toggleIsCollapsed(rootID, true)); - expect(store).toMatchInlineSnapshot(` + act(() => store.toggleIsCollapsed(rootID, true)); + expect(store).toMatchInlineSnapshot(` [root] ▸ `); - act(() => store.toggleIsCollapsed(rootID, false)); - expect(store).toMatchInlineSnapshot(` + act(() => store.toggleIsCollapsed(rootID, false)); + expect(store).toMatchInlineSnapshot(` [root] ▾ ▾ @@ -812,17 +818,17 @@ describe('Store (legacy)', () => {
`); - const id = store.getElementIDAtIndex(1); + const id = store.getElementIDAtIndex(1); - act(() => store.toggleIsCollapsed(id, true)); - expect(store).toMatchInlineSnapshot(` + act(() => store.toggleIsCollapsed(id, true)); + expect(store).toMatchInlineSnapshot(` [root] ▾ ▸ `); - act(() => store.toggleIsCollapsed(id, false)); - expect(store).toMatchInlineSnapshot(` + act(() => store.toggleIsCollapsed(id, false)); + expect(store).toMatchInlineSnapshot(` [root] ▾ ▾ @@ -831,7 +837,8 @@ describe('Store (legacy)', () => { ▾
`); - }); + }); + } it('should support reordering of children', () => { const Root = ({children}) =>
{children}
; diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js index 7dcf9e52be472..46bdd05f627d6 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js @@ -640,7 +640,9 @@ function setProp( case 'suppressHydrationWarning': case 'defaultValue': // Reserved case 'defaultChecked': - case 'innerHTML': { + case 'innerHTML': + case 'ref': { + // TODO: `ref` is pretty common, should we move it up? // Noop break; } @@ -988,7 +990,8 @@ function setPropOnCustomElement( } case 'suppressContentEditableWarning': case 'suppressHydrationWarning': - case 'innerHTML': { + case 'innerHTML': + case 'ref': { // Noop break; } @@ -2194,6 +2197,7 @@ function diffHydratedCustomComponent( case 'defaultValue': case 'defaultChecked': case 'innerHTML': + case 'ref': // Noop continue; case 'dangerouslySetInnerHTML': @@ -2307,6 +2311,7 @@ function diffHydratedGenericElement( case 'defaultValue': case 'defaultChecked': case 'innerHTML': + case 'ref': // Noop continue; case 'dangerouslySetInnerHTML': diff --git a/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js b/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js index b6fe8d9109a6f..388491a79fd38 100644 --- a/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js +++ b/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js @@ -1226,6 +1226,7 @@ function pushAttribute( case 'innerHTML': // Must use dangerouslySetInnerHTML instead. case 'suppressContentEditableWarning': case 'suppressHydrationWarning': + case 'ref': // Ignored. These are built-in to React on the client. return; case 'autoFocus': @@ -3391,6 +3392,7 @@ function pushStartCustomElement( break; case 'suppressContentEditableWarning': case 'suppressHydrationWarning': + case 'ref': // Ignored. These are built-in to React on the client. break; case 'className': @@ -4964,6 +4966,7 @@ function writeStyleResourceAttributeInJS( case 'suppressContentEditableWarning': case 'suppressHydrationWarning': case 'style': + case 'ref': // Ignored return; @@ -5157,6 +5160,7 @@ function writeStyleResourceAttributeInAttr( case 'suppressContentEditableWarning': case 'suppressHydrationWarning': case 'style': + case 'ref': // Ignored return; diff --git a/packages/react-dom-bindings/src/shared/ReactDOMUnknownPropertyHook.js b/packages/react-dom-bindings/src/shared/ReactDOMUnknownPropertyHook.js index 1785e985bfe37..3dbb1d8e2c315 100644 --- a/packages/react-dom-bindings/src/shared/ReactDOMUnknownPropertyHook.js +++ b/packages/react-dom-bindings/src/shared/ReactDOMUnknownPropertyHook.js @@ -186,7 +186,8 @@ function validateProperty(tagName, name, value, eventRegistry) { case 'suppressHydrationWarning': case 'defaultValue': // Reserved case 'defaultChecked': - case 'innerHTML': { + case 'innerHTML': + case 'ref': { return true; } case 'innerText': // Properties diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js index c58e825203f8e..b3ec748d92ffb 100644 --- a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js @@ -278,26 +278,48 @@ describe('ReactCompositeComponent', () => { } } + function refFn1(ref) { + instance1 = ref; + } + + function refFn2(ref) { + instance2 = ref; + } + + function refFn3(ref) { + instance3 = ref; + } + let instance1; let instance2; let instance3; const root = ReactDOMClient.createRoot(document.createElement('div')); await act(() => { - root.render( (instance1 = ref)} />); + root.render(); }); - expect(instance1.props).toEqual({prop: 'testKey'}); + if (gate(flags => flags.enableRefAsProp)) { + expect(instance1.props).toEqual({prop: 'testKey', ref: refFn1}); + } else { + expect(instance1.props).toEqual({prop: 'testKey'}); + } await act(() => { - root.render( - (instance2 = ref)} prop={undefined} />, - ); + root.render(); }); - expect(instance2.props).toEqual({prop: 'testKey'}); + if (gate(flags => flags.enableRefAsProp)) { + expect(instance2.props).toEqual({prop: 'testKey', ref: refFn2}); + } else { + expect(instance2.props).toEqual({prop: 'testKey'}); + } await act(() => { - root.render( (instance3 = ref)} prop={null} />); + root.render(); }); - expect(instance3.props).toEqual({prop: null}); + if (gate(flags => flags.enableRefAsProp)) { + expect(instance3.props).toEqual({prop: null, ref: refFn3}); + } else { + expect(instance3.props).toEqual({prop: null}); + } }); it('should not mutate passed-in props object', async () => { diff --git a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js index 4c6a0bed84a6c..63d42ce609d09 100644 --- a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js +++ b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js @@ -109,6 +109,7 @@ describe('ReactDeprecationWarnings', () => { await waitForAll([]); }); + // @gate !enableRefAsProp || !__DEV__ it('should warn when owner and self are different for string refs', async () => { class RefComponent extends React.Component { render() { @@ -140,6 +141,7 @@ describe('ReactDeprecationWarnings', () => { }); if (__DEV__) { + // @gate !enableRefAsProp it('should warn when owner and self are different for string refs', async () => { class RefComponent extends React.Component { render() { diff --git a/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js b/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js index 08406bdd72d04..6bc620443b4d6 100644 --- a/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js @@ -199,6 +199,7 @@ describe('ReactFunctionComponent', () => { ); }); + // @gate !enableRefAsProp || !__DEV__ it('should warn when given a string ref', async () => { function Indirection(props) { return
{props.children}
; @@ -240,7 +241,8 @@ describe('ReactFunctionComponent', () => { }); }); - it('should warn when given a function ref and ignore them', async () => { + // @gate !enableRefAsProp || !__DEV__ + it('should warn when given a function ref', async () => { function Indirection(props) { return
{props.children}
; } @@ -283,6 +285,7 @@ describe('ReactFunctionComponent', () => { }); }); + // @gate !enableRefAsProp || !__DEV__ it('deduplicates ref warnings based on element or owner', async () => { // When owner uses JSX, we can use exact line location to dedupe warnings class AnonymousParentUsingJSX extends React.Component { @@ -373,6 +376,7 @@ describe('ReactFunctionComponent', () => { // This guards against a regression caused by clearing the current debug fiber. // https://github.com/facebook/react/issues/10831 // @gate !disableLegacyContext || !__DEV__ + // @gate !enableRefAsProp || !__DEV__ it('should warn when giving a function ref with context', async () => { function Child() { return null; diff --git a/packages/react-dom/src/__tests__/refs-test.js b/packages/react-dom/src/__tests__/refs-test.js index 2018134c32511..fbc2133237e43 100644 --- a/packages/react-dom/src/__tests__/refs-test.js +++ b/packages/react-dom/src/__tests__/refs-test.js @@ -414,30 +414,21 @@ describe('ref swapping', () => { }).rejects.toThrow( 'Expected ref to be a function, a string, an object returned by React.createRef(), or null.', ); + }); - await act(() => { - root.render(
); - }); - - await act(() => { - root.render({ - $$typeof: Symbol.for('react.element'), - type: 'div', - props: {}, - key: null, - ref: null, - }); - }); - - // But this doesn't + // @gate !enableRefAsProp + it('undefined ref on manually inlined React element triggers error', async () => { + const container = document.createElement('div'); + const root = ReactDOMClient.createRoot(container); await expect(async () => { await act(() => { root.render({ $$typeof: Symbol.for('react.element'), type: 'div', - props: {}, + props: { + ref: undefined, + }, key: null, - ref: undefined, }); }); }).rejects.toThrow( diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 6fc58593b7076..8c4f45c426336 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -32,6 +32,7 @@ import { ConcurrentRoot, LegacyRoot, } from 'react-reconciler/constants'; +import {enableRefAsProp} from 'shared/ReactFeatureFlags'; type Container = { rootID: string, @@ -781,6 +782,29 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { let currentEventPriority = DefaultEventPriority; + function createJSXElementForTestComparison(type, props) { + if (enableRefAsProp) { + return { + $$typeof: REACT_ELEMENT_TYPE, + type: type, + key: null, + props: props, + _owner: null, + _store: __DEV__ ? {} : undefined, + }; + } else { + return { + $$typeof: REACT_ELEMENT_TYPE, + type: type, + key: null, + ref: null, + props: props, + _owner: null, + _store: __DEV__ ? {} : undefined, + }; + } + } + function childToJSX(child, text) { if (text !== null) { return text; @@ -818,15 +842,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { if (children !== null) { props.children = children; } - return { - $$typeof: REACT_ELEMENT_TYPE, - type: instance.type, - key: null, - ref: null, - props: props, - _owner: null, - _store: __DEV__ ? {} : undefined, - }; + return createJSXElementForTestComparison(instance.type, props); } // This is a text instance const textInstance: TextInstance = (child: any); @@ -858,15 +874,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { return null; } if (isArray(children)) { - return { - $$typeof: REACT_ELEMENT_TYPE, - type: REACT_FRAGMENT_TYPE, - key: null, - ref: null, - props: {children}, - _owner: null, - _store: __DEV__ ? {} : undefined, - }; + return createJSXElementForTestComparison(REACT_FRAGMENT_TYPE, {children}); } return children; } @@ -877,15 +885,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { return null; } if (isArray(children)) { - return { - $$typeof: REACT_ELEMENT_TYPE, - type: REACT_FRAGMENT_TYPE, - key: null, - ref: null, - props: {children}, - _owner: null, - _store: __DEV__ ? {} : undefined, - }; + return createJSXElementForTestComparison(REACT_FRAGMENT_TYPE, {children}); } return children; } diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index e0feae7b39720..a18795e2345e9 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -42,6 +42,7 @@ import { } from './ReactWorkTags'; import isArray from 'shared/isArray'; import {checkPropStringCoercion} from 'shared/CheckStringCoercion'; +import {enableRefAsProp} from 'shared/ReactFeatureFlags'; import { createWorkInProgress, @@ -153,7 +154,19 @@ function coerceRef( current: Fiber | null, element: ReactElement, ) { - const mixedRef = element.ref; + let mixedRef; + if (enableRefAsProp) { + // TODO: This is a temporary, intermediate step. When enableRefAsProp is on, + // we should resolve the `ref` prop during the begin phase of the component + // it's attached to (HostComponent, ClassComponent, etc). + + const refProp = element.props.ref; + mixedRef = refProp !== undefined ? refProp : null; + } else { + // Old behavior. + mixedRef = element.ref; + } + if ( mixedRef !== null && typeof mixedRef !== 'function' && diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index dff1dc5bc0905..f0084c2d7f258 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -110,6 +110,7 @@ import { enableAsyncActions, enablePostpone, enableRenderableContext, + enableRefAsProp, } from 'shared/ReactFeatureFlags'; import isArray from 'shared/isArray'; import shallowEqual from 'shared/shallowEqual'; @@ -403,6 +404,24 @@ function updateForwardRef( const render = Component.render; const ref = workInProgress.ref; + let propsWithoutRef; + if (enableRefAsProp && 'ref' in nextProps) { + // `ref` is just a prop now, but `forwardRef` expects it to not appear in + // the props object. This used to happen in the JSX runtime, but now we do + // it here. + propsWithoutRef = ({}: {[string]: any}); + for (const key in nextProps) { + // Since `ref` should only appear in props via the JSX transform, we can + // assume that this is a plain object. So we don't need a + // hasOwnProperty check. + if (key !== 'ref') { + propsWithoutRef[key] = nextProps[key]; + } + } + } else { + propsWithoutRef = nextProps; + } + // The rest is a fork of updateFunctionComponent let nextChildren; let hasId; @@ -417,7 +436,7 @@ function updateForwardRef( current, workInProgress, render, - nextProps, + propsWithoutRef, ref, renderLanes, ); @@ -428,7 +447,7 @@ function updateForwardRef( current, workInProgress, render, - nextProps, + propsWithoutRef, ref, renderLanes, ); @@ -1980,7 +1999,7 @@ function validateFunctionComponentInDev(workInProgress: Fiber, Component: any) { ); } } - if (workInProgress.ref !== null) { + if (!enableRefAsProp && workInProgress.ref !== null) { let info = ''; const componentName = getComponentNameFromType(Component) || 'Unknown'; const ownerName = getCurrentFiberOwnerNameInDevOrNull(); diff --git a/packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js b/packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js index a32b826597f22..4f4771413bee4 100644 --- a/packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js @@ -27,7 +27,11 @@ describe('ReactFiberRefs', () => { test('ref is attached even if there are no other updates (class)', async () => { let component; - class Component extends React.PureComponent { + class Component extends React.Component { + shouldComponentUpdate() { + // This component's output doesn't depend on any props or state + return false; + } render() { Scheduler.log('Render'); component = this; diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js index b47897e54d92e..1e34dd9cde5d2 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js @@ -1296,16 +1296,22 @@ describe('ReactIncrementalSideEffects', () => { } ReactNoop.render(); - await expect(async () => await waitForAll([])).toErrorDev( - 'Warning: Function components cannot be given refs. ' + - 'Attempts to access this ref will fail. ' + - 'Did you mean to use React.forwardRef()?\n\n' + - 'Check the render method ' + - 'of `Foo`.\n' + - ' in FunctionComponent (at **)\n' + - ' in div (at **)\n' + - ' in Foo (at **)', - ); + + if (gate(flags => flags.enableRefAsProp)) { + await waitForAll([]); + } else { + await expect(async () => await waitForAll([])).toErrorDev( + 'Warning: Function components cannot be given refs. ' + + 'Attempts to access this ref will fail. ' + + 'Did you mean to use React.forwardRef()?\n\n' + + 'Check the render method ' + + 'of `Foo`.\n' + + ' in FunctionComponent (at **)\n' + + ' in div (at **)\n' + + ' in Foo (at **)', + ); + } + expect(ops).toEqual([ classInstance, // no call for function components diff --git a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js index bc441b8d18213..f15f6aaf4243b 100644 --- a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js @@ -1174,6 +1174,7 @@ describe('ReactLazy', () => { expect(root).toMatchRenderedOutput('2'); }); + // @gate !enableRefAsProp || !__DEV__ it('warns about ref on functions for lazy-loaded components', async () => { const Foo = props =>
; const LazyFoo = lazy(() => { diff --git a/packages/react-reconciler/src/__tests__/ReactMemo-test.js b/packages/react-reconciler/src/__tests__/ReactMemo-test.js index 972c87d3614c8..0212f50bab318 100644 --- a/packages/react-reconciler/src/__tests__/ReactMemo-test.js +++ b/packages/react-reconciler/src/__tests__/ReactMemo-test.js @@ -44,6 +44,7 @@ describe('memo', () => { return {default: result}; } + // @gate !enableRefAsProp || !__DEV__ it('warns when giving a ref (simple)', async () => { // This test lives outside sharedTests because the wrappers don't forward // refs properly, and they end up affecting the current owner which is used @@ -62,6 +63,7 @@ describe('memo', () => { ]); }); + // @gate !enableRefAsProp || !__DEV__ it('warns when giving a ref (complex)', async () => { // defaultProps means this won't use SimpleMemoComponent (as of this writing) // SimpleMemoComponent is unobservable tho, so we can't check :) diff --git a/packages/react-refresh/src/__tests__/ReactFresh-test.js b/packages/react-refresh/src/__tests__/ReactFresh-test.js index 38b74563c30a7..2f0def9063190 100644 --- a/packages/react-refresh/src/__tests__/ReactFresh-test.js +++ b/packages/react-refresh/src/__tests__/ReactFresh-test.js @@ -3919,12 +3919,17 @@ describe('ReactFresh', () => { ReactFreshRuntime = require('react-refresh/runtime'); ReactFreshRuntime.injectIntoGlobalHook(global); + // NOTE: Intentionally using createElement in this test instead of JSX + // because old versions of React are incompatible with the JSX transform + // used by our test suite. const Hello = () => { - return
Hi!
; + const [state] = React.useState('Hi!'); + // Intentionally + return React.createElement('div', null, state); }; $RefreshReg$(Hello, 'Hello'); const Component = Hello; - ReactDOM.render(, container); + ReactDOM.render(React.createElement(Component), container); expect(onCommitFiberRoot).toHaveBeenCalled(); } diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 5faefdc7d0da6..a42ba62302757 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -32,7 +32,6 @@ import type {ContextSnapshot} from './ReactFizzNewContext'; import type {ComponentStackNode} from './ReactFizzComponentStack'; import type {TreeContext} from './ReactFizzTreeContext'; import type {ThenableState} from './ReactFizzThenable'; -import {enableRenderableContext} from 'shared/ReactFeatureFlags'; import {describeObjectForErrorMessage} from 'shared/ReactSerializationErrors'; import { @@ -145,6 +144,8 @@ import { enableFloat, enableCache, enablePostpone, + enableRenderableContext, + enableRefAsProp, } from 'shared/ReactFeatureFlags'; import assign from 'shared/assign'; @@ -1663,12 +1664,31 @@ function renderForwardRef( ): void { const previousComponentStack = task.componentStack; task.componentStack = createFunctionComponentStack(task, type.render); + + let propsWithoutRef; + if (enableRefAsProp && 'ref' in props) { + // `ref` is just a prop now, but `forwardRef` expects it to not appear in + // the props object. This used to happen in the JSX runtime, but now we do + // it here. + propsWithoutRef = ({}: {[string]: any}); + for (const key in props) { + // Since `ref` should only appear in props via the JSX transform, we can + // assume that this is a plain object. So we don't need a + // hasOwnProperty check. + if (key !== 'ref') { + propsWithoutRef[key] = props[key]; + } + } + } else { + propsWithoutRef = props; + } + const children = renderWithHooks( request, task, keyPath, type.render, - props, + propsWithoutRef, ref, ); const hasId = checkDidRenderIdHook(); @@ -2189,7 +2209,18 @@ function renderNodeDestructive( const type = element.type; const key = element.key; const props = element.props; - const ref = element.ref; + + let ref; + if (enableRefAsProp) { + // TODO: This is a temporary, intermediate step. Once the feature + // flag is removed, we should get the ref off the props object right + // before using it. + const refProp = props.ref; + ref = refProp !== undefined ? refProp : null; + } else { + ref = element.ref; + } + const name = getComponentNameFromType(type); const keyOrIndex = key == null ? (childIndex === -1 ? 0 : childIndex) : key; diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index e5a3dda775959..1444b6c6e6099 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -16,6 +16,7 @@ import { enablePostpone, enableTaint, enableServerComponentKeys, + enableRefAsProp, } from 'shared/ReactFeatureFlags'; import { @@ -698,6 +699,8 @@ function renderElement( // When the ref moves to the regular props object this will implicitly // throw for functions. We could probably relax it to a DEV warning for other // cases. + // TODO: `ref` is now just a prop when `enableRefAsProp` is on. Should we + // do what the above comment says? throw new Error( 'Refs cannot be used in Server Components, nor passed to Client Components.', ); @@ -1267,6 +1270,18 @@ function renderModelDestructive( } } + const props = element.props; + let ref; + if (enableRefAsProp) { + // TODO: This is a temporary, intermediate step. Once the feature + // flag is removed, we should get the ref off the props object right + // before using it. + const refProp = props.ref; + ref = refProp !== undefined ? refProp : null; + } else { + ref = element.ref; + } + // Attempt to render the Server Component. return renderElement( request, @@ -1274,8 +1289,8 @@ function renderModelDestructive( element.type, // $FlowFixMe[incompatible-call] the key of an element is null | string element.key, - element.ref, - element.props, + ref, + props, ); } case REACT_LAZY_TYPE: { diff --git a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js index 4d885c9eff3fc..37c17c05b08ba 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js @@ -268,6 +268,7 @@ describe('ReactTestRenderer', () => { expect(log).toEqual([null]); }); + // @gate !enableRefAsProp || !__DEV__ it('warns correctly for refs on SFCs', () => { function Bar() { return
Hello, world
; @@ -981,9 +982,14 @@ describe('ReactTestRenderer', () => {
)); + let refFn; + class App extends React.Component { render() { - return (this.ref = r)} />; + refFn = inst => { + this.ref = inst; + }; + return ; } } @@ -1004,7 +1010,11 @@ describe('ReactTestRenderer', () => { { instance: null, nodeType: 'host', - props: {}, + props: gate(flags => flags.enableRefAsProp) + ? { + ref: refFn, + } + : {}, rendered: [], type: 'span', }, diff --git a/packages/react/src/__tests__/ReactCreateElement-test.js b/packages/react/src/__tests__/ReactCreateElement-test.js index 6d1a16e3bb145..6c52cc2fb84a5 100644 --- a/packages/react/src/__tests__/ReactCreateElement-test.js +++ b/packages/react/src/__tests__/ReactCreateElement-test.js @@ -37,7 +37,11 @@ describe('ReactCreateElement', () => { const element = React.createElement(ComponentClass); expect(element.type).toBe(ComponentClass); expect(element.key).toBe(null); - expect(element.ref).toBe(null); + if (gate(flags => flags.enableRefAsProp)) { + expect(element.ref).toBe(undefined); + } else { + expect(element.ref).toBe(null); + } if (__DEV__) { expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); @@ -86,6 +90,7 @@ describe('ReactCreateElement', () => { ); }); + // @gate !enableRefAsProp it('should warn when `ref` is being accessed', async () => { class Child extends React.Component { render() { @@ -119,7 +124,11 @@ describe('ReactCreateElement', () => { const element = React.createElement('div'); expect(element.type).toBe('div'); expect(element.key).toBe(null); - expect(element.ref).toBe(null); + if (gate(flags => flags.enableRefAsProp)) { + expect(element.ref).toBe(undefined); + } else { + expect(element.ref).toBe(null); + } if (__DEV__) { expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); @@ -150,31 +159,46 @@ describe('ReactCreateElement', () => { expect(element.props.foo).toBe(1); }); - it('extracts key and ref from the config', () => { + it('extracts key from the rest of the props', () => { const element = React.createElement(ComponentClass, { key: '12', - ref: '34', foo: '56', }); expect(element.type).toBe(ComponentClass); expect(element.key).toBe('12'); - expect(element.ref).toBe('34'); - if (__DEV__) { - expect(Object.isFrozen(element)).toBe(true); - expect(Object.isFrozen(element.props)).toBe(true); + const expectation = {foo: '56'}; + Object.freeze(expectation); + expect(element.props).toEqual(expectation); + }); + + it('does not extract ref from the rest of the props', () => { + const ref = React.createRef(); + const element = React.createElement(ComponentClass, { + key: '12', + ref: ref, + foo: '56', + }); + expect(element.type).toBe(ComponentClass); + if (gate(flags => flags.enableRefAsProp)) { + expect(element.ref).toBe(undefined); + const expectation = {foo: '56', ref}; + Object.freeze(expectation); + expect(element.props).toEqual(expectation); + } else { + const expectation = {foo: '56'}; + Object.freeze(expectation); + expect(element.props).toEqual(expectation); + expect(element.ref).toBe(ref); } - expect(element.props).toEqual({foo: '56'}); }); - it('extracts null key and ref', () => { + it('extracts null key', () => { const element = React.createElement(ComponentClass, { key: null, - ref: null, foo: '12', }); expect(element.type).toBe(ComponentClass); expect(element.key).toBe('null'); - expect(element.ref).toBe(null); if (__DEV__) { expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); @@ -191,7 +215,11 @@ describe('ReactCreateElement', () => { const element = React.createElement(ComponentClass, props); expect(element.type).toBe(ComponentClass); expect(element.key).toBe(null); - expect(element.ref).toBe(null); + if (gate(flags => flags.enableRefAsProp)) { + expect(element.ref).toBe(undefined); + } else { + expect(element.ref).toBe(null); + } if (__DEV__) { expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); @@ -203,7 +231,11 @@ describe('ReactCreateElement', () => { const elementA = React.createElement('div'); const elementB = React.createElement('div', elementA.props); expect(elementB.key).toBe(null); - expect(elementB.ref).toBe(null); + if (gate(flags => flags.enableRefAsProp)) { + expect(elementB.ref).toBe(undefined); + } else { + expect(elementB.ref).toBe(null); + } }); it('coerces the key to a string', () => { @@ -213,7 +245,11 @@ describe('ReactCreateElement', () => { }); expect(element.type).toBe(ComponentClass); expect(element.key).toBe('12'); - expect(element.ref).toBe(null); + if (gate(flags => flags.enableRefAsProp)) { + expect(element.ref).toBe(undefined); + } else { + expect(element.ref).toBe(null); + } if (__DEV__) { expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); diff --git a/packages/react/src/__tests__/ReactElementClone-test.js b/packages/react/src/__tests__/ReactElementClone-test.js index 053341013a6ed..c9a88f4929f4e 100644 --- a/packages/react/src/__tests__/ReactElementClone-test.js +++ b/packages/react/src/__tests__/ReactElementClone-test.js @@ -212,7 +212,11 @@ describe('ReactElementClone', () => { ref: this.xyzRef, }); expect(clone.key).toBe('xyz'); - expect(clone.ref).toBe(this.xyzRef); + if (gate(flags => flags.enableRefAsProp)) { + expect(clone.props.ref).toBe(this.xyzRef); + } else { + expect(clone.ref).toBe(this.xyzRef); + } return
{clone}
; } } @@ -368,7 +372,11 @@ describe('ReactElementClone', () => { const elementA = React.createElement('div'); const elementB = React.cloneElement(elementA, elementA.props); expect(elementB.key).toBe(null); - expect(elementB.ref).toBe(null); + if (gate(flags => flags.enableRefAsProp)) { + expect(elementB.ref).toBe(undefined); + } else { + expect(elementB.ref).toBe(null); + } }); it('should ignore undefined key and ref', () => { @@ -385,12 +393,17 @@ describe('ReactElementClone', () => { const clone = React.cloneElement(element, props); expect(clone.type).toBe(ComponentClass); expect(clone.key).toBe('12'); - expect(clone.ref).toBe('34'); + if (gate(flags => flags.enableRefAsProp)) { + expect(clone.props.ref).toBe('34'); + expect(clone.props).toEqual({foo: 'ef', ref: '34'}); + } else { + expect(clone.ref).toBe('34'); + expect(clone.props).toEqual({foo: 'ef'}); + } if (__DEV__) { expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); } - expect(clone.props).toEqual({foo: 'ef'}); }); it('should extract null key and ref', () => { @@ -407,12 +420,19 @@ describe('ReactElementClone', () => { const clone = React.cloneElement(element, props); expect(clone.type).toBe(ComponentClass); expect(clone.key).toBe('null'); - expect(clone.ref).toBe(null); + if (gate(flags => flags.enableRefAsProp)) { + // TODO: Remove `ref` field from the element entirely. + expect(clone.ref).toBe(undefined); + expect(clone.props).toEqual({foo: 'ef', ref: null}); + } else { + expect(clone.ref).toBe(null); + expect(clone.props).toEqual({foo: 'ef'}); + } + if (__DEV__) { expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); } - expect(clone.props).toEqual({foo: 'ef'}); }); it('throws an error if passed null', () => { diff --git a/packages/react/src/__tests__/ReactJSXElementValidator-test.js b/packages/react/src/__tests__/ReactJSXElementValidator-test.js index 53b200462385b..bb8fad60ecfa2 100644 --- a/packages/react/src/__tests__/ReactJSXElementValidator-test.js +++ b/packages/react/src/__tests__/ReactJSXElementValidator-test.js @@ -389,9 +389,15 @@ describe('ReactJSXElementValidator', () => { } } - expect(() => ReactTestUtils.renderIntoDocument()).toErrorDev( - 'Invalid attribute `ref` supplied to `React.Fragment`.', - ); + if (gate(flags => flags.enableRefAsProp)) { + expect(() => ReactTestUtils.renderIntoDocument()).toErrorDev( + 'Invalid prop `ref` supplied to `React.Fragment`.', + ); + } else { + expect(() => ReactTestUtils.renderIntoDocument()).toErrorDev( + 'Invalid attribute `ref` supplied to `React.Fragment`.', + ); + } }); it('does not warn for fragments of multiple elements without keys', () => { diff --git a/packages/react/src/__tests__/ReactJSXRuntime-test.js b/packages/react/src/__tests__/ReactJSXRuntime-test.js index 713430fa5ead9..bb77f22ae03bf 100644 --- a/packages/react/src/__tests__/ReactJSXRuntime-test.js +++ b/packages/react/src/__tests__/ReactJSXRuntime-test.js @@ -220,6 +220,7 @@ describe('ReactJSXRuntime', () => { ); }); + // @gate !enableRefAsProp it('should warn when `ref` is being accessed', async () => { const container = document.createElement('div'); class Child extends React.Component { diff --git a/packages/react/src/__tests__/ReactJSXTransformIntegration-test.js b/packages/react/src/__tests__/ReactJSXTransformIntegration-test.js index 9485db97be4ea..fd58114c0b497 100644 --- a/packages/react/src/__tests__/ReactJSXTransformIntegration-test.js +++ b/packages/react/src/__tests__/ReactJSXTransformIntegration-test.js @@ -55,7 +55,11 @@ describe('ReactJSXTransformIntegration', () => { const element = ; expect(element.type).toBe(Component); expect(element.key).toBe(null); - expect(element.ref).toBe(null); + if (gate(flags => flags.enableRefAsProp)) { + expect(element.ref).toBe(undefined); + } else { + expect(element.ref).toBe(null); + } const expectation = {}; Object.freeze(expectation); expect(element.props).toEqual(expectation); @@ -65,7 +69,11 @@ describe('ReactJSXTransformIntegration', () => { const element =
; expect(element.type).toBe('div'); expect(element.key).toBe(null); - expect(element.ref).toBe(null); + if (gate(flags => flags.enableRefAsProp)) { + expect(element.ref).toBe(undefined); + } else { + expect(element.ref).toBe(null); + } const expectation = {}; Object.freeze(expectation); expect(element.props).toEqual(expectation); @@ -76,7 +84,11 @@ describe('ReactJSXTransformIntegration', () => { const element = ; expect(element.type).toBe('div'); expect(element.key).toBe(null); - expect(element.ref).toBe(null); + if (gate(flags => flags.enableRefAsProp)) { + expect(element.ref).toBe(undefined); + } else { + expect(element.ref).toBe(null); + } const expectation = {}; Object.freeze(expectation); expect(element.props).toEqual(expectation); @@ -99,22 +111,41 @@ describe('ReactJSXTransformIntegration', () => { expect(element.props.foo).toBe(1); }); - it('extracts key and ref from the rest of the props', () => { - const ref = React.createRef(); - const element = ; + it('extracts key from the rest of the props', () => { + const element = ; expect(element.type).toBe(Component); expect(element.key).toBe('12'); - expect(element.ref).toBe(ref); const expectation = {foo: '56'}; Object.freeze(expectation); expect(element.props).toEqual(expectation); }); + it('does not extract ref from the rest of the props', () => { + const ref = React.createRef(); + const element = ; + expect(element.type).toBe(Component); + if (gate(flags => flags.enableRefAsProp)) { + expect(element.ref).toBe(undefined); + const expectation = {foo: '56', ref}; + Object.freeze(expectation); + expect(element.props).toEqual(expectation); + } else { + const expectation = {foo: '56'}; + Object.freeze(expectation); + expect(element.props).toEqual(expectation); + expect(element.ref).toBe(ref); + } + }); + it('coerces the key to a string', () => { const element = ; expect(element.type).toBe(Component); expect(element.key).toBe('12'); - expect(element.ref).toBe(null); + if (gate(flags => flags.enableRefAsProp)) { + expect(element.ref).toBe(undefined); + } else { + expect(element.ref).toBe(null); + } const expectation = {foo: '56'}; Object.freeze(expectation); expect(element.props).toEqual(expectation); diff --git a/packages/react/src/jsx/ReactJSXElement.js b/packages/react/src/jsx/ReactJSXElement.js index 4e546963292a3..1ffb7696b8e24 100644 --- a/packages/react/src/jsx/ReactJSXElement.js +++ b/packages/react/src/jsx/ReactJSXElement.js @@ -21,6 +21,7 @@ import isValidElementType from 'shared/isValidElementType'; import isArray from 'shared/isArray'; import {describeUnknownElementTypeFrameInDEV} from 'shared/ReactComponentStackFrame'; import checkPropTypes from 'shared/checkPropTypes'; +import {enableRefAsProp} from 'shared/ReactFeatureFlags'; const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner; const ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame; @@ -36,15 +37,17 @@ if (__DEV__) { } function hasValidRef(config) { - if (__DEV__) { - if (hasOwnProperty.call(config, 'ref')) { - const getter = Object.getOwnPropertyDescriptor(config, 'ref').get; - if (getter && getter.isReactWarning) { - return false; + if (!enableRefAsProp) { + if (__DEV__) { + if (hasOwnProperty.call(config, 'ref')) { + const getter = Object.getOwnPropertyDescriptor(config, 'ref').get; + if (getter && getter.isReactWarning) { + return false; + } } } + return config.ref !== undefined; } - return config.ref !== undefined; } function hasValidKey(config) { @@ -111,24 +114,26 @@ function defineKeyPropWarningGetter(props, displayName) { } function defineRefPropWarningGetter(props, displayName) { - if (__DEV__) { - const warnAboutAccessingRef = function () { - if (!specialPropRefWarningShown) { - specialPropRefWarningShown = true; - console.error( - '%s: `ref` is not a prop. Trying to access it will result ' + - 'in `undefined` being returned. If you need to access the same ' + - 'value within the child component, you should pass it as a different ' + - 'prop. (https://reactjs.org/link/special-props)', - displayName, - ); - } - }; - warnAboutAccessingRef.isReactWarning = true; - Object.defineProperty(props, 'ref', { - get: warnAboutAccessingRef, - configurable: true, - }); + if (!enableRefAsProp) { + if (__DEV__) { + const warnAboutAccessingRef = function () { + if (!specialPropRefWarningShown) { + specialPropRefWarningShown = true; + console.error( + '%s: `ref` is not a prop. Trying to access it will result ' + + 'in `undefined` being returned. If you need to access the same ' + + 'value within the child component, you should pass it as a different ' + + 'prop. (https://reactjs.org/link/special-props)', + displayName, + ); + } + }; + warnAboutAccessingRef.isReactWarning = true; + Object.defineProperty(props, 'ref', { + get: warnAboutAccessingRef, + configurable: true, + }); + } } } @@ -153,19 +158,30 @@ function defineRefPropWarningGetter(props, displayName) { * @internal */ function ReactElement(type, key, ref, self, source, owner, props) { - const element = { - // This tag allows us to uniquely identify this as a React Element - $$typeof: REACT_ELEMENT_TYPE, - - // Built-in properties that belong on the element - type, - key, - ref, - props, - - // Record the component responsible for creating this element. - _owner: owner, - }; + const element = enableRefAsProp + ? { + // This tag allows us to uniquely identify this as a React Element + $$typeof: REACT_ELEMENT_TYPE, + + // Built-in properties that belong on the element + type, + key, + props, + + // Record the component responsible for creating this element. + _owner: owner, + } + : { + // Old behavior. When enableRefAsProp is off, `ref` is an extra field. + ref, + + // Everything else is the same. + $$typeof: REACT_ELEMENT_TYPE, + type, + key, + props, + _owner: owner, + }; if (__DEV__) { // The validation flag is currently mutative. We put it on @@ -235,7 +251,7 @@ export function jsxProd(type, config, maybeKey) { key = '' + config.key; } - if (hasValidRef(config)) { + if (!enableRefAsProp && hasValidRef(config)) { ref = config.ref; } @@ -245,8 +261,7 @@ export function jsxProd(type, config, maybeKey) { hasOwnProperty.call(config, propName) && // Skip over reserved prop names propName !== 'key' && - // TODO: `ref` will no longer be reserved in the next major - propName !== 'ref' + (enableRefAsProp || propName !== 'ref') ) { props[propName] = config[propName]; } @@ -452,7 +467,7 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) { key = '' + config.key; } - if (hasValidRef(config)) { + if (!enableRefAsProp && hasValidRef(config)) { ref = config.ref; warnIfStringRefCannotBeAutoConverted(config, self); } @@ -463,8 +478,7 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) { hasOwnProperty.call(config, propName) && // Skip over reserved prop names propName !== 'key' && - // TODO: `ref` will no longer be reserved in the next major - propName !== 'ref' + (enableRefAsProp || propName !== 'ref') ) { props[propName] = config[propName]; } @@ -480,7 +494,7 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) { } } - if (key || ref) { + if (key || (!enableRefAsProp && ref)) { const displayName = typeof type === 'function' ? type.displayName || type.name || 'Unknown' @@ -488,7 +502,7 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) { if (key) { defineKeyPropWarningGetter(props, displayName); } - if (ref) { + if (!enableRefAsProp && ref) { defineRefPropWarningGetter(props, displayName); } } @@ -588,7 +602,7 @@ export function createElement(type, config, children) { let ref = null; if (config != null) { - if (hasValidRef(config)) { + if (!enableRefAsProp && hasValidRef(config)) { ref = config.ref; if (__DEV__) { @@ -608,14 +622,11 @@ export function createElement(type, config, children) { hasOwnProperty.call(config, propName) && // Skip over reserved prop names propName !== 'key' && - // TODO: `ref` will no longer be reserved in the next major - propName !== 'ref' && - // ...and maybe these, too, though we currently rely on them for - // warnings and debug information in dev. Need to decide if we're OK - // with dropping them. In the jsx() runtime it's not an issue because - // the data gets passed as separate arguments instead of props, but - // it would be nice to stop relying on them entirely so we can drop - // them from the internal Fiber field. + (enableRefAsProp || propName !== 'ref') && + // Even though we don't use these anymore in the runtime, we don't want + // them to appear as props, so in createElement we filter them out. + // We don't have to do this in the jsx() runtime because the jsx() + // transform never passed these as props; it used separate arguments. propName !== '__self' && propName !== '__source' ) { @@ -652,7 +663,7 @@ export function createElement(type, config, children) { } } if (__DEV__) { - if (key || ref) { + if (key || (!enableRefAsProp && ref)) { const displayName = typeof type === 'function' ? type.displayName || type.name || 'Unknown' @@ -660,7 +671,7 @@ export function createElement(type, config, children) { if (key) { defineKeyPropWarningGetter(props, displayName); } - if (ref) { + if (!enableRefAsProp && ref) { defineRefPropWarningGetter(props, displayName); } } @@ -764,7 +775,7 @@ export function cloneElement(element, config, children) { let owner = element._owner; if (config != null) { - if (hasValidRef(config)) { + if (!enableRefAsProp && hasValidRef(config)) { // Silently steal the ref from the parent. ref = config.ref; owner = ReactCurrentOwner.current; @@ -786,8 +797,7 @@ export function cloneElement(element, config, children) { hasOwnProperty.call(config, propName) && // Skip over reserved prop names propName !== 'key' && - // TODO: `ref` will no longer be reserved in the next major - propName !== 'ref' && + (enableRefAsProp || propName !== 'ref') && // ...and maybe these, too, though we currently rely on them for // warnings and debug information in dev. Need to decide if we're OK // with dropping them. In the jsx() runtime it's not an issue because @@ -795,7 +805,11 @@ export function cloneElement(element, config, children) { // it would be nice to stop relying on them entirely so we can drop // them from the internal Fiber field. propName !== '__self' && - propName !== '__source' + propName !== '__source' && + // Undefined `ref` is ignored by cloneElement. We treat it the same as + // if the property were missing. This is mostly for + // backwards compatibility. + !(enableRefAsProp && propName === 'ref' && config.ref === undefined) ) { if (config[propName] === undefined && defaultProps !== undefined) { // Resolve default props @@ -1016,6 +1030,7 @@ function getCurrentComponentErrorInfo(parentType) { * @param {ReactElement} fragment */ function validateFragmentProps(fragment) { + // TODO: Move this to render phase instead of at element creation. if (__DEV__) { const keys = Object.keys(fragment.props); for (let i = 0; i < keys.length; i++) { @@ -1032,7 +1047,7 @@ function validateFragmentProps(fragment) { } } - if (fragment.ref !== null) { + if (!enableRefAsProp && fragment.ref !== null) { setCurrentlyValidatingElement(fragment); console.error('Invalid attribute `ref` supplied to `React.Fragment`.'); setCurrentlyValidatingElement(null); diff --git a/scripts/jest/TestFlags.js b/scripts/jest/TestFlags.js index 80b9efe4faac7..fd917b00b7068 100644 --- a/scripts/jest/TestFlags.js +++ b/scripts/jest/TestFlags.js @@ -78,12 +78,21 @@ function getTestFlags() { source: !process.env.IS_BUILD, www, - // This isn't a flag, just a useful alias for tests. + // These aren't flags, just a useful aliases for tests. enableActivity: releaseChannel === 'experimental' || www, - enableUseSyncExternalStoreShim: !__VARIANT__, enableSuspenseList: releaseChannel === 'experimental' || www, enableLegacyHidden: www, + // This is used by useSyncExternalStoresShared-test.js to decide whether + // to test the shim or the native implementation of useSES. + // TODO: It's disabled when enableRefAsProp is on because the JSX + // runtime used by our tests is not compatible with older versions of + // React. If we want to keep testing this shim after enableRefIsProp is + // on everywhere, we'll need to find some other workaround. Maybe by + // only using createElement instead of JSX in that test module. + enableUseSyncExternalStoreShim: + !__VARIANT__ && !featureFlags.enableRefAsProp, + // If there's a naming conflict between scheduler and React feature flags, the // React ones take precedence. // TODO: Maybe we should error on conflicts? Or we could namespace