Fix cloneElement using string ref w no owner (#28797)
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>
Joseph Savona committed
Apr 9, 2024 at 13:43 UTC
7f5d25e23e82adef262493794010385f1955f0b6
2 files changed
+60
-3
packages/react/src/__tests__/ReactElementClone-test.js
+59
-2
@@ -274,8 +274,56 @@ describe('ReactElementClone', () => {
274
275
const root = ReactDOMClient.createRoot(document.createElement('div'));
276
await act(() => root.render(<Grandparent />));
277
- expect(component.childRef).toEqual({current: null});
278
- expect(component.parentRef.current.xyzRef.current.tagName).toBe('SPAN');
277
+ if (gate(flags => flags.enableRefAsProp && flags.disableStringRefs)) {
278
+ expect(component.childRef).toEqual({current: null});
279
+ expect(component.parentRef.current.xyzRef.current.tagName).toBe('SPAN');
280
+ } else if (
281
+ gate(flags => !flags.enableRefAsProp && !flags.disableStringRefs)
282
+ ) {
283
+ expect(component.childRef).toEqual({current: null});
284
+ expect(component.parentRef.current.xyzRef.current.tagName).toBe('SPAN');
285
+ } else if (
286
+ gate(flags => flags.enableRefAsProp && !flags.disableStringRefs)
287
+ ) {
288
+ expect(component.childRef).toEqual({current: null});
289
+ expect(component.parentRef.current.xyzRef.current.tagName).toBe('SPAN');
290
+ } else {
291
+ // Not going to bother testing every possible combination.
292
+ }
293
+ });
294
+
295
+ // @gate !disableStringRefs
296
+ it('should steal the ref if a new string ref is specified without an owner', async () => {
297
+ // Regression test for this specific feature combination calling cloneElement on an element
298
+ // without an owner
299
+ await expect(async () => {
300
+ // create an element without an owner
301
+ const element = React.createElement('div', {id: 'some-id'});
302
+ class Parent extends React.Component {
303
+ render() {
304
+ return <Child>{element}</Child>;
305
+ }
306
+ }
307
+ let child;
308
+ class Child extends React.Component {
309
+ render() {
310
+ child = this;
311
+ const clone = React.cloneElement(this.props.children, {
312
+ ref: 'xyz',
313
+ });
314
+ return <div>{clone}</div>;
315
+ }
316
+ }
317
+
318
+ const root = ReactDOMClient.createRoot(document.createElement('div'));
319
+ await act(() => root.render(<Parent />));
320
+ expect(child.refs.xyz.tagName).toBe('DIV');
321
+ }).toErrorDev([
322
+ 'Warning: Component "Child" contains the string ref "xyz". Support for ' +
323
+ 'string refs will be removed in a future major release. We recommend ' +
324
+ 'using useRef() or createRef() instead. Learn more about using refs ' +
325
+ 'safely here: https://react.dev/link/strict-mode-string-ref',
326
+ ]);
327
});
328
329
it('should overwrite props', async () => {
@@ -371,6 +419,15 @@ describe('ReactElementClone', () => {
419
) {
420
expect(clone.ref).toBe(element.ref);
421
expect(clone.props).toEqual({foo: 'ef'});
422
+ } else if (
423
+ gate(flags => flags.enableRefAsProp && !flags.disableStringRefs)
424
+ ) {
425
+ expect(() => {
426
+ expect(clone.ref).toBe(element.ref);
427
+ }).toErrorDev('Accessing element.ref was removed in React 19', {
428
+ withoutStack: true,
429
+ });
430
+ expect(clone.props).toEqual({foo: 'ef', ref: element.ref});
431
} else {
432
// Not going to bother testing every possible combination.
433
}
packages/react/src/jsx/ReactJSXElement.js
+1
-1
@@ -866,6 +866,7 @@ export function cloneElement(element, config, children) {
866
867
if (config != null) {
868
if (hasValidRef(config)) {
869
+ owner = ReactSharedInternals.owner;
870
if (!enableRefAsProp) {
871
// Silently steal the ref from the parent.
872
ref = config.ref;
@@ -873,7 +874,6 @@ export function cloneElement(element, config, children) {
874
ref = coerceStringRef(ref, owner, element.type);
875
}
876
}
876
- owner = ReactSharedInternals.owner;
877
}
878
if (hasValidKey(config)) {
879
if (__DEV__) {