Backwards compatibility for string refs on WWW (#28826)

Seeing errors with undefined string ref values when trying to sync
#28473

Added a test that reproduces the failing pattern.

@acdlite pushed
a786481
with fix

---------

Co-authored-by: Jack Pope <jackpope@meta.com>
Co-authored-by: Andrew Clark <git@andrewclark.io>

Author: 3 people

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

@@ -129,6 +129,55 @@ describe('ReactComponent', () => {
     }
   });
 
+  // @gate !disableStringRefs
+  it('string refs do not detach and reattach on every render', async () => {
+    spyOnDev(console, 'error').mockImplementation(() => {});
+
+    let refVal;
+    class Child extends React.Component {
+      componentDidUpdate() {
+        // The parent ref should still be attached because it hasn't changed
+        // since the last render. If the ref had changed, then this would be
+        // undefined because refs are attached during the same phase (layout)
+        // as componentDidUpdate, in child -> parent order. So the new parent
+        // ref wouldn't have attached yet.
+        refVal = this.props.contextRef();
+      }
+
+      render() {
+        if (this.props.show) {
+          return <div>child</div>;
+        }
+      }
+    }
+
+    class Parent extends React.Component {
+      render() {
+        return (
+          <div id="test-root" ref="root">
+            <Child
+              contextRef={() => this.refs.root}
+              show={this.props.showChild}
+            />
+          </div>
+        );
+      }
+    }
+
+    const container = document.createElement('div');
+    const root = ReactDOMClient.createRoot(container);
+
+    await act(() => {
+      root.render(<Parent />);
+    });
+
+    expect(refVal).toBe(undefined);
+    await act(() => {
+      root.render(<Parent showChild={true} />);
+    });
+    expect(refVal).toBe(container.querySelector('#test-root'));
+  });
+
   // @gate !disableStringRefs
   it('should support string refs on owned components', async () => {
     const innerObj = {};

packages/react-reconciler/src/ReactFiberBeginWork.js

@@ -1048,6 +1048,25 @@ function markRef(current: Fiber | null, workInProgress: Fiber) {
       );
     }
     if (current === null || current.ref !== ref) {
+      if (!disableStringRefs && current !== null) {
+        const oldRef = current.ref;
+        const newRef = ref;
+        if (
+          typeof oldRef === 'function' &&
+          typeof newRef === 'function' &&
+          typeof oldRef.__stringRef === 'string' &&
+          oldRef.__stringRef === newRef.__stringRef &&
+          oldRef.__stringRefType === newRef.__stringRefType &&
+          oldRef.__stringRefOwner === newRef.__stringRefOwner
+        ) {
+          // Although this is a different callback, it represents the same
+          // string ref. To avoid breaking old Meta code that relies on string
+          // refs only being attached once, reuse the old ref. This will
+          // prevent us from detaching and reattaching the ref on each update.
+          workInProgress.ref = oldRef;
+          return;
+        }
+      }
       // Schedule a Ref effect
       workInProgress.flags |= Ref | RefStatic;
     }

packages/react/src/jsx/ReactJSXElement.js

@@ -1189,7 +1189,15 @@ function coerceStringRef(mixedRef, owner, type) {
     }
   }
 
-  return stringRefAsCallbackRef.bind(null, stringRef, type, owner);
+  const callback = stringRefAsCallbackRef.bind(null, stringRef, type, owner);
+  // This is used to check whether two callback refs conceptually represent
+  // the same string ref, and can therefore be reused by the reconciler. Needed
+  // for backwards compatibility with old Meta code that relies on string refs
+  // not being reattached on every render.
+  callback.__stringRef = stringRef;
+  callback.__type = type;
+  callback.__owner = owner;
+  return callback;
 }
 
 function stringRefAsCallbackRef(stringRef, type, owner, value) {