Pass ref as normal prop

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.
facebook · Feb 20, 2024 · a54df41 · a54df41
1 parent 7b636d2
commit a54df41
Show file tree

Hide file tree

Showing 28 changed files with 544 additions and 247 deletions.
diff --git 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
@@ -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<any> {
-  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
@@ -753,37 +753,43 @@ describe('Store (legacy)', () => {
       `);
     });
 
-    it('should support expanding deep parts of the tree', () => {
-      const Wrapper = ({forwardedRef}) => (
-        <Nested depth={3} forwardedRef={forwardedRef} />
-      );
-      const Nested = ({depth, forwardedRef}) =>
-        depth > 0 ? (
-          <Nested depth={depth - 1} forwardedRef={forwardedRef} />
-        ) : (
-          <div ref={forwardedRef} />
+    // 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}) => (
+          <Nested depth={3} forwardedRef={forwardedRef} />
         );
-
-      let ref = null;
-      const refSetter = value => {
-        ref = value;
-      };
-
-      act(() =>
-        ReactDOM.render(
-          <Wrapper forwardedRef={refSetter} />,
-          document.createElement('div'),
-        ),
-      );
-      expect(store).toMatchInlineSnapshot(`
+        const Nested = ({depth, forwardedRef}) =>
+          depth > 0 ? (
+            <Nested depth={depth - 1} forwardedRef={forwardedRef} />
+          ) : (
+            <div ref={forwardedRef} />
+          );
+
+        let ref = null;
+        const refSetter = value => {
+          ref = value;
+        };
+
+        act(() =>
+          ReactDOM.render(
+            <Wrapper forwardedRef={refSetter} />,
+            document.createElement('div'),
+          ),
+        );
+        expect(store).toMatchInlineSnapshot(`
         [root]
           ▸ <Wrapper>
       `);
 
-      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]
           ▾ <Wrapper>
             ▾ <Nested>
@@ -793,16 +799,16 @@ describe('Store (legacy)', () => {
                       <div>
       `);
 
-      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]
           ▸ <Wrapper>
       `);
 
-      act(() => store.toggleIsCollapsed(rootID, false));
-      expect(store).toMatchInlineSnapshot(`
+        act(() => store.toggleIsCollapsed(rootID, false));
+        expect(store).toMatchInlineSnapshot(`
         [root]
           ▾ <Wrapper>
             ▾ <Nested>
@@ -812,17 +818,17 @@ describe('Store (legacy)', () => {
                       <div>
       `);
 
-      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]
           ▾ <Wrapper>
             ▸ <Nested>
       `);
 
-      act(() => store.toggleIsCollapsed(id, false));
-      expect(store).toMatchInlineSnapshot(`
+        act(() => store.toggleIsCollapsed(id, false));
+        expect(store).toMatchInlineSnapshot(`
         [root]
           ▾ <Wrapper>
             ▾ <Nested>
@@ -831,7 +837,8 @@ describe('Store (legacy)', () => {
                   ▾ <Nested>
                       <div>
       `);
-    });
+      });
+    }
 
     it('should support reordering of children', () => {
       const Root = ({children}) => <div>{children}</div>;

diff --git 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
@@ -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
@@ -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