Skip to content

Commit

Permalink
Backwards compatibility for string refs on WWW (#28826)
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
3 people authored and rickhanlonii committed Apr 11, 2024
1 parent 80bbdd5 commit b6bcb92
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 1 deletion.
49 changes: 49 additions & 0 deletions packages/react-dom/src/__tests__/ReactComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {};
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 b6bcb92

Please sign in to comment.