Fix: Class components should "consume" ref prop

When a ref is passed to a class component, the class instance is
attached to the ref's current property automatically. This different
from function components, where you have to do something extra to
attach a ref to an instance, like passing the ref
to `useImperativeHandle`.

Existing class component code is written with the assumption that a
ref will not be passed through as a prop. For example, class components
that act as indirections often spread `this.props` onto a child
component. To maintain this expectation, we should remove the ref from
the props object ("consume" it) before passing it to lifecycle methods.
Without this change, much existing code will break because the ref will
attach to the inner component instead of the outer one.

This is not an issue for function components because we used to warn if
you passed a ref to a function component. Instead, you had to use
`forwardRef`, which also implements this "consuming" behavior.

Co-authored-by: Jan Kassens <jan@kassens.net>

Author: acdlite

packages/react-dom/src/__tests__/ReactCompositeComponent-test.js

@@ -268,29 +268,17 @@ describe('ReactCompositeComponent', () => {
     await act(() => {
       root.render(<Component ref={refFn1} />);
     });
-    if (gate(flags => flags.enableRefAsProp)) {
-      expect(instance1.props).toEqual({prop: 'testKey', ref: refFn1});
-    } else {
-      expect(instance1.props).toEqual({prop: 'testKey'});
-    }
+    expect(instance1.props).toEqual({prop: 'testKey'});
 
     await act(() => {
       root.render(<Component ref={refFn2} prop={undefined} />);
     });
-    if (gate(flags => flags.enableRefAsProp)) {
-      expect(instance2.props).toEqual({prop: 'testKey', ref: refFn2});
-    } else {
-      expect(instance2.props).toEqual({prop: 'testKey'});
-    }
+    expect(instance2.props).toEqual({prop: 'testKey'});
 
     await act(() => {
       root.render(<Component ref={refFn3} prop={null} />);
     });
-    if (gate(flags => flags.enableRefAsProp)) {
-      expect(instance3.props).toEqual({prop: null, ref: refFn3});
-    } else {
-      expect(instance3.props).toEqual({prop: null});
-    }
+    expect(instance3.props).toEqual({prop: null});
   });
 
   it('should not mutate passed-in props object', async () => {

packages/react-reconciler/src/ReactFiberClassComponent.js

@@ -23,6 +23,7 @@ import {
   enableDebugTracing,
   enableSchedulingProfiler,
   enableLazyContextPropagation,
+  enableRefAsProp,
 } from 'shared/ReactFeatureFlags';
 import ReactStrictModeWarnings from './ReactStrictModeWarnings';
 import {isMounted} from './ReactFiberTreeReflection';
@@ -1252,7 +1253,15 @@ export function resolveClassComponentProps(
     }
   }
 
-  // TODO: Remove ref from props object
+  if (enableRefAsProp) {
+    // Remove ref from the props object, if it exists.
+    if ('ref' in newProps) {
+      if (newProps === baseProps) {
+        newProps = assign({}, newProps);
+      }
+      delete newProps.ref;
+    }
+  }
 
   return newProps;
 }

packages/react-reconciler/src/ReactFiberCommitWork.js

@@ -471,6 +471,7 @@ function commitBeforeMutationEffectsOnFiber(finishedWork: Fiber) {
           if (__DEV__) {
             if (
               !finishedWork.type.defaultProps &&
+              !('ref' in finishedWork.memoizedProps) &&
               !didWarnAboutReassigningProps
             ) {
               if (instance.props !== finishedWork.memoizedProps) {
@@ -807,7 +808,11 @@ function commitClassLayoutLifecycles(
     // but instead we rely on them being set during last render.
     // TODO: revisit this when we implement resuming.
     if (__DEV__) {
-      if (!finishedWork.type.defaultProps && !didWarnAboutReassigningProps) {
+      if (
+        !finishedWork.type.defaultProps &&
+        !('ref' in finishedWork.memoizedProps) &&
+        !didWarnAboutReassigningProps
+      ) {
         if (instance.props !== finishedWork.memoizedProps) {
           console.error(
             'Expected %s props to match memoized props before ' +
@@ -856,7 +861,11 @@ function commitClassLayoutLifecycles(
     // but instead we rely on them being set during last render.
     // TODO: revisit this when we implement resuming.
     if (__DEV__) {
-      if (!finishedWork.type.defaultProps && !didWarnAboutReassigningProps) {
+      if (
+        !finishedWork.type.defaultProps &&
+        !('ref' in finishedWork.memoizedProps) &&
+        !didWarnAboutReassigningProps
+      ) {
         if (instance.props !== finishedWork.memoizedProps) {
           console.error(
             'Expected %s props to match memoized props before ' +
@@ -913,7 +922,11 @@ function commitClassCallbacks(finishedWork: Fiber) {
   if (updateQueue !== null) {
     const instance = finishedWork.stateNode;
     if (__DEV__) {
-      if (!finishedWork.type.defaultProps && !didWarnAboutReassigningProps) {
+      if (
+        !finishedWork.type.defaultProps &&
+        !('ref' in finishedWork.memoizedProps) &&
+        !didWarnAboutReassigningProps
+      ) {
         if (instance.props !== finishedWork.memoizedProps) {
           console.error(
             'Expected %s props to match memoized props before ' +

packages/react-reconciler/src/__tests__/ReactClassComponentPropResolution-test.js

@@ -0,0 +1,92 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @emails react-core
+ */
+
+'use strict';
+
+let React;
+let ReactNoop;
+let Scheduler;
+let act;
+let assertLog;
+
+describe('ReactClassComponentPropResolution', () => {
+  beforeEach(() => {
+    jest.resetModules();
+
+    React = require('react');
+    ReactNoop = require('react-noop-renderer');
+    Scheduler = require('scheduler');
+    act = require('internal-test-utils').act;
+    assertLog = require('internal-test-utils').assertLog;
+  });
+
+  function Text({text}) {
+    Scheduler.log(text);
+    return text;
+  }
+
+  test('resolves ref and default props before calling lifecycle methods', async () => {
+    const root = ReactNoop.createRoot();
+
+    function getPropKeys(props) {
+      return Object.keys(props).join(', ');
+    }
+
+    class Component extends React.Component {
+      shouldComponentUpdate(props) {
+        Scheduler.log(
+          'shouldComponentUpdate (prev props): ' + getPropKeys(this.props),
+        );
+        Scheduler.log(
+          'shouldComponentUpdate (next props): ' + getPropKeys(props),
+        );
+        return true;
+      }
+      componentDidUpdate(props) {
+        Scheduler.log('componentDidUpdate (prev props): ' + getPropKeys(props));
+        Scheduler.log(
+          'componentDidUpdate (next props): ' + getPropKeys(this.props),
+        );
+        return true;
+      }
+      componentDidMount() {
+        Scheduler.log('componentDidMount: ' + getPropKeys(this.props));
+        return true;
+      }
+      render() {
+        return <Text text={'render: ' + getPropKeys(this.props)} />;
+      }
+    }
+
+    Component.defaultProps = {
+      default: 'yo',
+    };
+
+    // `ref` should never appear as a prop. `default` always should.
+
+    // Mount
+    const ref = React.createRef();
+    await act(async () => {
+      root.render(<Component text="Yay" ref={ref} />);
+    });
+    assertLog(['render: text, default', 'componentDidMount: text, default']);
+
+    // Update
+    await act(async () => {
+      root.render(<Component text="Yay (again)" ref={ref} />);
+    });
+    assertLog([
+      'shouldComponentUpdate (prev props): text, default',
+      'shouldComponentUpdate (next props): text, default',
+      'render: text, default',
+      'componentDidUpdate (prev props): text, default',
+      'componentDidUpdate (next props): text, default',
+    ]);
+  });
+});

packages/react/src/__tests__/ReactCreateElement-test.js

@@ -90,7 +90,7 @@ describe('ReactCreateElement', () => {
     );
   });
 
-  // @gate !enableRefAsProp
+  // @gate !enableRefAsProp || !__DEV__
   it('should warn when `ref` is being accessed', async () => {
     class Child extends React.Component {
       render() {

packages/react/src/__tests__/ReactJSXRuntime-test.js

@@ -249,7 +249,7 @@ describe('ReactJSXRuntime', () => {
     );
   });
 
-  // @gate !enableRefAsProp
+  // @gate !enableRefAsProp || !__DEV__
   it('should warn when `ref` is being accessed', async () => {
     const container = document.createElement('div');
     class Child extends React.Component {