diff --git a/packages/jest-react/src/JestReact.js b/packages/jest-react/src/JestReact.js index b660ad49f94b1..f94a035f869b0 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,34 @@ function assertYieldsWereCleared(root) { } } +function createJSXElementForTestComparison(type, props) { + if (__DEV__ && enableRefAsProp) { + const element = { + $$typeof: REACT_ELEMENT_TYPE, + type: type, + key: null, + props: props, + _owner: null, + _store: __DEV__ ? {} : undefined, + }; + Object.defineProperty(element, 'ref', { + enumerable: false, + value: null, + }); + return element; + } 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 +84,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 +103,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..a7476f5d972e2 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, @@ -463,24 +467,46 @@ export function reportGlobalError(response: Response, error: Error): void { }); } +function nullRefGetter() { + if (__DEV__) { + return null; + } +} + function createElement( type: mixed, 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, - }; + let element: any; + if (__DEV__ && enableRefAsProp) { + // `ref` is non-enumerable in dev + element = ({ + $$typeof: REACT_ELEMENT_TYPE, + type, + key, + props, + _owner: null, + }: any); + Object.defineProperty(element, 'ref', { + enumerable: false, + get: nullRefGetter, + }); + } else { + element = ({ + // This tag allows us to uniquely identify this as a React Element + $$typeof: REACT_ELEMENT_TYPE, + + type, + key, + ref: null, + props, + + // Record the component responsible for creating this element. + _owner: null, + }: any); + } + 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__/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..77c386457e1d2 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,34 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { let currentEventPriority = DefaultEventPriority; + function createJSXElementForTestComparison(type, props) { + if (__DEV__ && enableRefAsProp) { + const element = { + type: type, + $$typeof: REACT_ELEMENT_TYPE, + key: null, + props: props, + _owner: null, + _store: __DEV__ ? {} : undefined, + }; + Object.defineProperty(element, 'ref', { + enumerable: false, + value: null, + }); + return element; + } 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 +847,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 +879,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 +890,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..8eb5f8c0e13f2 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(null); + } 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(null); + } else { + expect(element.ref).toBe(null); + } if (__DEV__) { expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); @@ -150,31 +159,49 @@ 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(() => expect(element.ref).toBe(ref)).toErrorDev( + 'Accessing element.ref is no longer supported', + {withoutStack: true}, + ); + 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 +218,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(null); + } else { + expect(element.ref).toBe(null); + } if (__DEV__) { expect(Object.isFrozen(element)).toBe(true); expect(Object.isFrozen(element.props)).toBe(true); @@ -203,7 +234,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(null); + } else { + expect(elementB.ref).toBe(null); + } }); it('coerces the key to a string', () => { @@ -213,7 +248,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(null); + } 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..2d72859433499 100644 --- a/packages/react/src/__tests__/ReactElementClone-test.js +++ b/packages/react/src/__tests__/ReactElementClone-test.js @@ -18,6 +18,8 @@ describe('ReactElementClone', () => { let ComponentClass; beforeEach(() => { + jest.resetModules(); + act = require('internal-test-utils').act; PropTypes = require('prop-types'); @@ -212,7 +214,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 +374,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(null); + } else { + expect(elementB.ref).toBe(null); + } }); it('should ignore undefined key and ref', () => { @@ -385,12 +395,21 @@ 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(() => expect(clone.ref).toBe('34')).toErrorDev( + 'Accessing element.ref is no longer supported', + {withoutStack: true}, + ); + 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 +426,18 @@ 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)) { + expect(clone.ref).toBe(null); + 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..19d3458804f84 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(null); + } 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(null); + } 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(null); + } else { + expect(element.ref).toBe(null); + } const expectation = {}; Object.freeze(expectation); expect(element.props).toEqual(expectation); @@ -99,22 +111,44 @@ 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(() => expect(element.ref).toBe(ref)).toErrorDev( + 'Accessing element.ref is no longer supported', + {withoutStack: true}, + ); + 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(null); + } 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..dded86e3e7781 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; @@ -30,9 +31,11 @@ const REACT_CLIENT_REFERENCE = Symbol.for('react.client.reference'); let specialPropKeyWarningShown; let specialPropRefWarningShown; let didWarnAboutStringRefs; +let didWarnAboutElementRef; if (__DEV__) { didWarnAboutStringRefs = {}; + didWarnAboutElementRef = {}; } function hasValidRef(config) { @@ -111,24 +114,45 @@ function defineKeyPropWarningGetter(props, displayName) { } function defineRefPropWarningGetter(props, displayName) { + 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, + }); + } + } +} + +function elementRefGetterWithDeprecationWarning() { 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, - }); + const componentName = getComponentNameFromType(this.type); + if (!didWarnAboutElementRef[componentName]) { + didWarnAboutElementRef[componentName] = true; + console.error( + 'Accessing element.ref is no longer supported. ref is now a ' + + 'regular prop. It will be removed from the JSX Element ' + + 'type in a future release.', + ); + } + + // An undefined `element.ref` is coerced to `null` for + // backwards compatibility. + const refProp = this.props.ref; + return refProp !== undefined ? refProp : null; } } @@ -152,20 +176,85 @@ function defineRefPropWarningGetter(props, displayName) { * indicating filename, line number, and/or other information. * @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, +function ReactElement(type, key, _ref, self, source, owner, props) { + let ref; + if (enableRefAsProp) { + // When enableRefAsProp is on, ignore whatever was passed as the ref + // argument and treat `props.ref` as the source of truth. The only thing we + // use this for is `element.ref`, which will log a deprecation warning on + // access. In the next release, we can remove `element.ref` as well as the + // `ref` argument. + const refProp = props.ref; + + // An undefined `element.ref` is coerced to `null` for + // backwards compatibility. + ref = refProp !== undefined ? refProp : null; + } else { + ref = _ref; + } - // Built-in properties that belong on the element - type, - key, - ref, - props, + let element; + if (__DEV__ && enableRefAsProp) { + // In dev, make `ref` a non-enumerable property with a warning. It's non- + // enumerable so that test matchers and serializers don't access it and + // trigger the warning. + // + // `ref` will be removed from the element completely in a future release. + 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, - // Record the component responsible for creating this element. - _owner: owner, - }; + props, + + // Record the component responsible for creating this element. + _owner: owner, + }; + if (ref !== null) { + Object.defineProperty(element, 'ref', { + enumerable: false, + get: elementRefGetterWithDeprecationWarning, + }); + } else { + // Don't warn on access if a ref is not given. This reduces false + // positives in cases where a test serializer uses + // getOwnPropertyDescriptors to compare objects, like Jest does, which is + // a problem because it bypasses non-enumerability. + // + // So unfortunately this will trigger a false positive warning in Jest + // when the diff is printed: + // + // expect(
).toEqual(); + // + // A bit sketchy, but this is what we've done for the `props.key` and + // `props.ref` accessors for years, which implies it will be good enough + // for `element.ref`, too. Let's see if anyone complains. + Object.defineProperty(element, 'ref', { + enumerable: false, + value: null, + }); + } + } else { + // In prod, `ref` is a regular property. It will be removed in a + // future release. + 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, + }; + } if (__DEV__) { // The validation flag is currently mutative. We put it on @@ -236,7 +325,9 @@ export function jsxProd(type, config, maybeKey) { } if (hasValidRef(config)) { - ref = config.ref; + if (!enableRefAsProp) { + ref = config.ref; + } } // Remaining properties are added to a new props object @@ -245,8 +336,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]; } @@ -453,7 +543,9 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) { } if (hasValidRef(config)) { - ref = config.ref; + if (!enableRefAsProp) { + ref = config.ref; + } warnIfStringRefCannotBeAutoConverted(config, self); } @@ -463,8 +555,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 +571,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 +579,7 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) { if (key) { defineKeyPropWarningGetter(props, displayName); } - if (ref) { + if (!enableRefAsProp && ref) { defineRefPropWarningGetter(props, displayName); } } @@ -589,7 +680,9 @@ export function createElement(type, config, children) { if (config != null) { if (hasValidRef(config)) { - ref = config.ref; + if (!enableRefAsProp) { + ref = config.ref; + } if (__DEV__) { warnIfStringRefCannotBeAutoConverted(config, config.__self); @@ -608,14 +701,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 +742,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 +750,7 @@ export function createElement(type, config, children) { if (key) { defineKeyPropWarningGetter(props, displayName); } - if (ref) { + if (!enableRefAsProp && ref) { defineRefPropWarningGetter(props, displayName); } } @@ -732,7 +822,9 @@ export function cloneAndReplaceKey(oldElement, newKey) { return ReactElement( oldElement.type, newKey, - oldElement.ref, + // When enableRefAsProp is on, this argument is ignored. This check only + // exists to avoid the `ref` access warning. + enableRefAsProp ? null : oldElement.ref, undefined, undefined, oldElement._owner, @@ -758,15 +850,17 @@ export function cloneElement(element, config, children) { // Reserved names are extracted let key = element.key; - let ref = element.ref; + let ref = enableRefAsProp ? null : element.ref; // Owner will be preserved, unless ref is overridden let owner = element._owner; if (config != null) { if (hasValidRef(config)) { - // Silently steal the ref from the parent. - ref = config.ref; + if (!enableRefAsProp) { + // Silently steal the ref from the parent. + ref = config.ref; + } owner = ReactCurrentOwner.current; } if (hasValidKey(config)) { @@ -786,8 +880,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 +888,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 +1113,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 +1130,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/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index c2ffe2a652692..8a503e433ff30 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -178,6 +178,13 @@ export const enableServerComponentKeys = __NEXT_MAJOR__; */ export const enableInfiniteRenderLoopDetection = true; +// Subtle breaking changes to JSX runtime to make it faster, like passing `ref` +// as a normal prop instead of stripping it from the props object. + +// Passes `ref` as a normal prop instead of stripping it from the props object +// during element creation. +export const enableRefAsProp = __NEXT_MAJOR__; + // ----------------------------------------------------------------------------- // Chopping Block // diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 3a95817810fac..3a7dad1b9c8a5 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -97,5 +97,9 @@ export const disableClientCache = true; export const enableServerComponentKeys = true; export const enableInfiniteRenderLoopDetection = false; +// TODO: Roll out with GK. Don't keep as dynamic flag for too long, though, +// because JSX is an extremely hot path. +export const enableRefAsProp = false; + // 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 78c591c0aebf1..ac09110a4a517 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -89,5 +89,8 @@ export const disableClientCache = true; export const enableServerComponentKeys = true; +// TODO: Should turn this on in next "major" RN release. +export const enableRefAsProp = false; + // 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 93018e5c091da..e5dc5fe25cfee 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -89,5 +89,14 @@ export const disableClientCache = true; export const enableServerComponentKeys = true; export const enableInfiniteRenderLoopDetection = false; +// TODO: This must be in sync with the main ReactFeatureFlags file because +// the Test Renderer's value must be the same as the one used by the +// react package. +// +// We really need to get rid of this whole module. Any test renderer specific +// flags should be handled by the Fiber config. +const __NEXT_MAJOR__ = __EXPERIMENTAL__; +export const enableRefAsProp = __NEXT_MAJOR__; + // 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 4390303eeb181..6081e57da8ef4 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -86,5 +86,7 @@ export const disableClientCache = true; export const enableServerComponentKeys = true; +export const enableRefAsProp = false; + // 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 8e21c3b1a6286..6694b6b8afeff 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -89,5 +89,7 @@ export const disableClientCache = true; export const enableServerComponentKeys = true; export const enableInfiniteRenderLoopDetection = false; +export const enableRefAsProp = false; + // 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 0a18e4300be43..2babd5bcd695c 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -116,5 +116,9 @@ export const disableClientCache = true; export const enableServerComponentKeys = true; +// TODO: Roll out with GK. Don't keep as dynamic flag for too long, though, +// because JSX is an extremely hot path. +export const enableRefAsProp = false; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); 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