Skip to content

Commit

Permalink
Fix cloneElement using string ref w no owner (#28797)
Browse files Browse the repository at this point in the history
Fix for an issue introduced in #28473 where cloneElement() with a string
ref fails due to lack of an owner. We should use the current owner in
this case.

---------

Co-authored-by: Rick Hanlon <rickhanlonii@fb.com>
  • Loading branch information
josephsavona and rickhanlonii committed Apr 11, 2024
1 parent e3ebcd5 commit b4b10c9
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 3 deletions.
61 changes: 59 additions & 2 deletions packages/react/src/__tests__/ReactElementClone-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -274,8 +274,56 @@ describe('ReactElementClone', () => {

const root = ReactDOMClient.createRoot(document.createElement('div'));
await act(() => root.render(<Grandparent />));
expect(component.childRef).toEqual({current: null});
expect(component.parentRef.current.xyzRef.current.tagName).toBe('SPAN');
if (gate(flags => flags.enableRefAsProp && flags.disableStringRefs)) {
expect(component.childRef).toEqual({current: null});
expect(component.parentRef.current.xyzRef.current.tagName).toBe('SPAN');
} else if (
gate(flags => !flags.enableRefAsProp && !flags.disableStringRefs)
) {
expect(component.childRef).toEqual({current: null});
expect(component.parentRef.current.xyzRef.current.tagName).toBe('SPAN');
} else if (
gate(flags => flags.enableRefAsProp && !flags.disableStringRefs)
) {
expect(component.childRef).toEqual({current: null});
expect(component.parentRef.current.xyzRef.current.tagName).toBe('SPAN');
} else {
// Not going to bother testing every possible combination.
}
});

// @gate !disableStringRefs
it('should steal the ref if a new string ref is specified without an owner', async () => {
// Regression test for this specific feature combination calling cloneElement on an element
// without an owner
await expect(async () => {
// create an element without an owner
const element = React.createElement('div', {id: 'some-id'});
class Parent extends React.Component {
render() {
return <Child>{element}</Child>;
}
}
let child;
class Child extends React.Component {
render() {
child = this;
const clone = React.cloneElement(this.props.children, {
ref: 'xyz',
});
return <div>{clone}</div>;
}
}

const root = ReactDOMClient.createRoot(document.createElement('div'));
await act(() => root.render(<Parent />));
expect(child.refs.xyz.tagName).toBe('DIV');
}).toErrorDev([
'Warning: Component "Child" contains the string ref "xyz". Support for ' +
'string refs will be removed in a future major release. We recommend ' +
'using useRef() or createRef() instead. Learn more about using refs ' +
'safely here: https://react.dev/link/strict-mode-string-ref',
]);
});

it('should overwrite props', async () => {
Expand Down Expand Up @@ -371,6 +419,15 @@ describe('ReactElementClone', () => {
) {
expect(clone.ref).toBe(element.ref);
expect(clone.props).toEqual({foo: 'ef'});
} else if (
gate(flags => flags.enableRefAsProp && !flags.disableStringRefs)
) {
expect(() => {
expect(clone.ref).toBe(element.ref);
}).toErrorDev('Accessing element.ref was removed in React 19', {
withoutStack: true,
});
expect(clone.props).toEqual({foo: 'ef', ref: element.ref});
} else {
// Not going to bother testing every possible combination.
}
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/jsx/ReactJSXElement.js
Original file line number Diff line number Diff line change
Expand Up @@ -853,14 +853,14 @@ export function cloneElement(element, config, children) {

if (config != null) {
if (hasValidRef(config)) {
owner = ReactSharedInternals.owner;
if (!enableRefAsProp) {
// Silently steal the ref from the parent.
ref = config.ref;
if (!disableStringRefs) {
ref = coerceStringRef(ref, owner, element.type);
}
}
owner = ReactCurrentOwner.current;
}
if (hasValidKey(config)) {
if (__DEV__) {
Expand Down

0 comments on commit b4b10c9

Please sign in to comment.