Skip to content

Commit

Permalink
Reuse string ref if conceptually identical
Browse files Browse the repository at this point in the history
Now that string refs are coerced to callback refs in the JSX runtime,
a new callback ref is recreated each time. This is a subtle behavior
difference from the old behavior, because it means React will reattach
the ref on every render. While this is mostly not a huge issue, it is
technically observable because a child component can observe a parent
component's ref inside a layout effect or componentDidUpdate before the
parent ref is able to update (because layout effects and refs run in
child -> parent order).

To preserve the old behavior, I added the string refs "deps" as
extra properties on the callback. Then in the reconciler, we can compare
the deps to check whether the old callback ref can be reused.

This is similar to what we did before but in addition to checking the
string itself, we also need to check the other and the type, since those
are bound earlier than they were before.
  • Loading branch information
acdlite committed Apr 11, 2024
1 parent bed8885 commit a786481
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 2 deletions.
7 changes: 6 additions & 1 deletion packages/react-dom/src/__tests__/ReactComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,17 @@ describe('ReactComponent', () => {
});

// @gate !disableStringRefs
it('should support accessing string refs from parent components', async () => {
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();
}

Expand Down
19 changes: 19 additions & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
10 changes: 9 additions & 1 deletion packages/react/src/jsx/ReactJSXElement.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit a786481

Please sign in to comment.