Skip to content

Commit e3ebcd5

Browse files
acdliteacdlite
authored
Move string ref coercion to JSX runtime (#28473)
Based on: - #28464 --- This moves the entire string ref implementation out Fiber and into the JSX runtime. The string is converted to a callback ref during element creation. This is a subtle change in behavior, because it will have already been converted to a callback ref if you access element.prop.ref or element.ref. But this is only for Meta, because string refs are disabled entirely in open source. And if it leads to an issue in practice, the solution is to switch to a different ref type, which Meta is going to do regardless.
1 parent fd0da3e commit e3ebcd5

14 files changed

+199
-245
lines changed

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

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -47,10 +47,9 @@ describe('ReactComponent', () => {
4747
act(() => {
4848
root.render(<div ref="badDiv" />);
4949
}),
50-
).rejects.toThrow(
51-
'Element ref was specified as a string (badDiv) but no owner ' +
52-
'was set',
53-
);
50+
// TODO: This throws an AggregateError. Need to update test infra to
51+
// support matching against AggregateError.
52+
).rejects.toThrow();
5453
});
5554

5655
it('should throw (in dev) when children are mutated during render', async () => {
@@ -169,18 +168,14 @@ describe('ReactComponent', () => {
169168
root.render(<Component />);
170169
});
171170
}).toErrorDev([
172-
'Warning: Component "div" contains the string ref "inner". ' +
171+
'Warning: Component "Component" contains the string ref "inner". ' +
173172
'Support for string refs will be removed in a future major release. ' +
174173
'We recommend using useRef() or createRef() instead. ' +
175174
'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' +
175+
' in Wrapper (at **)\n' +
176176
' in div (at **)\n' +
177177
' in Wrapper (at **)\n' +
178178
' in Component (at **)',
179-
'Warning: Component "Component" contains the string ref "outer". ' +
180-
'Support for string refs will be removed in a future major release. ' +
181-
'We recommend using useRef() or createRef() instead. ' +
182-
'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' +
183-
' in Component (at **)',
184179
]);
185180
});
186181

‎packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ describe('ReactDOMServerIntegration', () => {
100100
'Support for string refs will be removed in a future major release. ' +
101101
'We recommend using useRef() or createRef() instead. ' +
102102
'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' +
103+
' in div (at **)\n' +
103104
' in RefsComponent (at **)',
104105
]);
105106
expect(component.refs.myDiv).toBe(root.firstChild);

‎packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ describe('ReactDeprecationWarnings', () => {
8686
'We recommend using useRef() or createRef() instead. ' +
8787
'Learn more about using refs safely here: ' +
8888
'https://react.dev/link/strict-mode-string-ref' +
89+
'\n in RefComponent (at **)' +
8990
'\n in Component (at **)',
9091
);
9192
});
@@ -137,10 +138,6 @@ describe('ReactDeprecationWarnings', () => {
137138
'We ask you to manually fix this case by using useRef() or createRef() instead. ' +
138139
'Learn more about using refs safely here: ' +
139140
'https://react.dev/link/strict-mode-string-ref',
140-
'Warning: Component "Component" contains the string ref "refComponent". ' +
141-
'Support for string refs will be removed in a future major release. We recommend ' +
142-
'using useRef() or createRef() instead. Learn more about using refs safely here: ' +
143-
'https://react.dev/link/strict-mode-string-ref',
144141
]);
145142
});
146143

@@ -173,10 +170,6 @@ describe('ReactDeprecationWarnings', () => {
173170
'We ask you to manually fix this case by using useRef() or createRef() instead. ' +
174171
'Learn more about using refs safely here: ' +
175172
'https://react.dev/link/strict-mode-string-ref',
176-
'Warning: Component "Component" contains the string ref "refComponent". ' +
177-
'Support for string refs will be removed in a future major release. We recommend ' +
178-
'using useRef() or createRef() instead. Learn more about using refs safely here: ' +
179-
'https://react.dev/link/strict-mode-string-ref',
180173
]);
181174
});
182175
});

‎packages/react-dom/src/__tests__/ReactFunctionComponent-test.js‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -185,18 +185,10 @@ describe('ReactFunctionComponent', () => {
185185
act(() => {
186186
root.render(<Child test="test" />);
187187
}),
188-
).rejects.toThrowError(
189-
__DEV__
190-
? 'Function components cannot have string refs. We recommend using useRef() instead.'
191-
: // It happens because we don't save _owner in production for
192-
// function components.
193-
'Element ref was specified as a string (me) but no owner was set. This could happen for one of' +
194-
' the following reasons:\n' +
195-
'1. You may be adding a ref to a function component\n' +
196-
"2. You may be adding a ref to a component that was not created inside a component's render method\n" +
197-
'3. You have multiple copies of React loaded\n' +
198-
'See https://react.dev/link/refs-must-have-owner for more information.',
199-
);
188+
)
189+
// TODO: This throws an AggregateError. Need to update test infra to
190+
// support matching against AggregateError.
191+
.rejects.toThrowError();
200192
});
201193

202194
// @gate !enableRefAsProp || !__DEV__

‎packages/react-dom/src/__tests__/multiple-copies-of-react-test.js‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,9 @@ describe('when different React version is used with string ref', () => {
3030
act(() => {
3131
root.render(<TextWithStringRef />);
3232
}),
33-
).rejects.toThrow(
34-
'Element ref was specified as a string (foo) but no owner was set. This could happen for one of' +
35-
' the following reasons:\n' +
36-
'1. You may be adding a ref to a function component\n' +
37-
"2. You may be adding a ref to a component that was not created inside a component's render method\n" +
38-
'3. You have multiple copies of React loaded\n' +
39-
'See https://react.dev/link/refs-must-have-owner for more information.',
40-
);
33+
)
34+
// TODO: This throws an AggregateError. Need to update test infra to
35+
// support matching against AggregateError.
36+
.rejects.toThrow();
4137
});
4238
});

‎packages/react-dom/src/__tests__/refs-test.js‎

Lines changed: 30 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -129,22 +129,22 @@ describe('reactiverefs', () => {
129129
);
130130
});
131131
}).toErrorDev([
132-
'Warning: Component "div" contains the string ref "resetDiv". ' +
133-
'Support for string refs will be removed in a future major release. ' +
134-
'We recommend using useRef() or createRef() instead. ' +
135-
'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' +
136-
' in div (at **)\n' +
137-
' in TestRefsComponent (at **)',
138-
'Warning: Component "span" contains the string ref "clickLog0". ' +
139-
'Support for string refs will be removed in a future major release. ' +
140-
'We recommend using useRef() or createRef() instead. ' +
141-
'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' +
142-
' in span (at **)\n' +
143-
' in ClickCounter (at **)\n' +
132+
'Warning: Component "TestRefsComponent" contains the string ' +
133+
'ref "resetDiv". Support for string refs will be removed in a ' +
134+
'future major release. We recommend using useRef() or createRef() ' +
135+
'instead. Learn more about using refs safely ' +
136+
'here: https://react.dev/link/strict-mode-string-ref\n' +
144137
' in div (at **)\n' +
145-
' in GeneralContainerComponent (at **)\n' +
146138
' in div (at **)\n' +
147139
' in TestRefsComponent (at **)',
140+
'Warning: Component "ClickCounter" contains the string ' +
141+
'ref "clickLog0". Support for string refs will be removed in a ' +
142+
'future major release. We recommend using useRef() or createRef() ' +
143+
'instead. Learn more about using refs safely ' +
144+
'here: https://react.dev/link/strict-mode-string-ref\n' +
145+
' in div (at **)\n' +
146+
' in span (at **)\n' +
147+
' in ClickCounter (at **)',
148148
]);
149149

150150
expect(testRefsComponent instanceof TestRefsComponent).toBe(true);
@@ -352,29 +352,29 @@ describe('ref swapping', () => {
352352
'Support for string refs will be removed in a future major release. ' +
353353
'We recommend using useRef() or createRef() instead. ' +
354354
'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' +
355+
' in div (at **)\n' +
355356
' in A (at **)',
356357
]);
357358
expect(a.refs[1].nodeName).toBe('DIV');
358359
});
359360

360-
// @gate !disableStringRefs
361361
it('provides an error for invalid refs', async () => {
362362
const container = document.createElement('div');
363363
const root = ReactDOMClient.createRoot(container);
364364
await expect(async () => {
365365
await act(() => {
366366
root.render(<div ref={10} />);
367367
});
368-
}).rejects.toThrow(
369-
'Element ref was specified as a string (10) but no owner was set.',
370-
);
368+
// TODO: This throws an AggregateError. Need to update test infra to
369+
// support matching against AggregateError.
370+
}).rejects.toThrow();
371371
await expect(async () => {
372372
await act(() => {
373373
root.render(<div ref={true} />);
374374
});
375-
}).rejects.toThrow(
376-
'Element ref was specified as a string (true) but no owner was set.',
377-
);
375+
// TODO: This throws an AggregateError. Need to update test infra to
376+
// support matching against AggregateError.
377+
}).rejects.toThrow();
378378
await expect(async () => {
379379
await act(() => {
380380
root.render(<div ref={Symbol('foo')} />);
@@ -520,14 +520,10 @@ describe('creating element with string ref in constructor', () => {
520520
await act(() => {
521521
root.render(<RefTest />);
522522
});
523-
}).rejects.toThrowError(
524-
'Element ref was specified as a string (p) but no owner was set. This could happen for one of' +
525-
' the following reasons:\n' +
526-
'1. You may be adding a ref to a function component\n' +
527-
"2. You may be adding a ref to a component that was not created inside a component's render method\n" +
528-
'3. You have multiple copies of React loaded\n' +
529-
'See https://react.dev/link/refs-must-have-owner for more information.',
530-
);
523+
})
524+
// TODO: This throws an AggregateError. Need to update test infra to
525+
// support matching against AggregateError.
526+
.rejects.toThrowError();
531527
});
532528
});
533529

@@ -581,10 +577,11 @@ describe('strings refs across renderers', () => {
581577
);
582578
});
583579
}).toErrorDev([
584-
'Warning: Component "Indirection" contains the string ref "child1". ' +
580+
'Warning: Component "Parent" contains the string ref "child1". ' +
585581
'Support for string refs will be removed in a future major release. ' +
586582
'We recommend using useRef() or createRef() instead. ' +
587583
'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref\n' +
584+
' in div (at **)\n' +
588585
' in Indirection (at **)\n' +
589586
' in Parent (at **)',
590587
]);
@@ -593,20 +590,10 @@ describe('strings refs across renderers', () => {
593590
expect(inst.refs.child1.tagName).toBe('DIV');
594591
expect(inst.refs.child1).toBe(div1.firstChild);
595592

596-
await expect(async () => {
597-
// Now both refs should be rendered.
598-
await act(() => {
599-
root.render(<Parent />);
600-
});
601-
}).toErrorDev(
602-
[
603-
'Warning: Component "Root" contains the string ref "child2". ' +
604-
'Support for string refs will be removed in a future major release. ' +
605-
'We recommend using useRef() or createRef() instead. ' +
606-
'Learn more about using refs safely here: https://react.dev/link/strict-mode-string-ref',
607-
],
608-
{withoutStack: true},
609-
);
593+
// Now both refs should be rendered.
594+
await act(() => {
595+
root.render(<Parent />);
596+
});
610597
expect(inst.refs.child1.tagName).toBe('DIV');
611598
expect(inst.refs.child1).toBe(div1.firstChild);
612599
expect(inst.refs.child2.tagName).toBe('DIV');

0 commit comments

Comments
 (0)