Skip to content

Commit e89e296

Browse files
committed
Move string ref coercion to JSX runtime
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 ea0180b commit e89e296

14 files changed

+197
-249
lines changed

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

+6-11
Original file line numberDiff line numberDiff line change
@@ -38,18 +38,17 @@ describe('ReactComponent', () => {
3838
}).toThrowError(/Target container is not a DOM element./);
3939
});
4040

41-
// @gate !disableStringRefs
4241
it('should throw when supplying a string ref outside of render method', async () => {
4342
const container = document.createElement('div');
4443
const root = ReactDOMClient.createRoot(container);
4544
await expect(
4645
act(() => {
4746
root.render(<div ref="badDiv" />);
4847
}),
49-
).rejects.toThrow(
50-
'Element ref was specified as a string (badDiv) but no owner ' +
51-
'was set',
52-
);
48+
)
49+
// TODO: This throws an AggregateError. Need to update test infra to
50+
// support matching against AggregateError.
51+
.rejects.toThrow();
5352
});
5453

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

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

+1
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://reactjs.org/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

+1-8
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@ describe('ReactDeprecationWarnings', () => {
8484
'We recommend using useRef() or createRef() instead. ' +
8585
'Learn more about using refs safely here: ' +
8686
'https://reactjs.org/link/strict-mode-string-ref' +
87+
'\n in RefComponent (at **)' +
8788
'\n in Component (at **)',
8889
);
8990
});
@@ -135,10 +136,6 @@ describe('ReactDeprecationWarnings', () => {
135136
'We ask you to manually fix this case by using useRef() or createRef() instead. ' +
136137
'Learn more about using refs safely here: ' +
137138
'https://reactjs.org/link/strict-mode-string-ref',
138-
'Warning: Component "Component" contains the string ref "refComponent". ' +
139-
'Support for string refs will be removed in a future major release. We recommend ' +
140-
'using useRef() or createRef() instead. Learn more about using refs safely here: ' +
141-
'https://reactjs.org/link/strict-mode-string-ref',
142139
]);
143140
});
144141

@@ -171,10 +168,6 @@ describe('ReactDeprecationWarnings', () => {
171168
'We ask you to manually fix this case by using useRef() or createRef() instead. ' +
172169
'Learn more about using refs safely here: ' +
173170
'https://reactjs.org/link/strict-mode-string-ref',
174-
'Warning: Component "Component" contains the string ref "refComponent". ' +
175-
'Support for string refs will be removed in a future major release. We recommend ' +
176-
'using useRef() or createRef() instead. Learn more about using refs safely here: ' +
177-
'https://reactjs.org/link/strict-mode-string-ref',
178171
]);
179172
});
180173
});

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

+4-13
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,6 @@ describe('ReactFunctionComponent', () => {
173173
).resolves.not.toThrowError();
174174
});
175175

176-
// @gate !disableStringRefs
177176
it('should throw on string refs in pure functions', async () => {
178177
function Child() {
179178
return <div ref="me" />;
@@ -185,18 +184,10 @@ describe('ReactFunctionComponent', () => {
185184
act(() => {
186185
root.render(<Child test="test" />);
187186
}),
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://reactjs.org/link/refs-must-have-owner for more information.',
199-
);
187+
)
188+
// TODO: This throws an AggregateError. Need to update test infra to
189+
// support matching against AggregateError.
190+
.rejects.toThrowError();
200191
});
201192

202193
// @gate !enableRefAsProp || !__DEV__

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

+4-9
Original file line numberDiff line numberDiff line change
@@ -22,21 +22,16 @@ class TextWithStringRef extends React.Component {
2222
}
2323

2424
describe('when different React version is used with string ref', () => {
25-
// @gate !disableStringRefs
2625
it('throws the "Refs must have owner" warning', async () => {
2726
const container = document.createElement('div');
2827
const root = ReactDOMClient.createRoot(container);
2928
await expect(
3029
act(() => {
3130
root.render(<TextWithStringRef />);
3231
}),
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://reactjs.org/link/refs-must-have-owner for more information.',
40-
);
32+
)
33+
// TODO: This throws an AggregateError. Need to update test infra to
34+
// support matching against AggregateError.
35+
.rejects.toThrow();
4136
});
4237
});

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

+29-44
Original file line numberDiff line numberDiff line change
@@ -131,22 +131,22 @@ describe('reactiverefs', () => {
131131
);
132132
});
133133
}).toErrorDev([
134-
'Warning: Component "div" contains the string ref "resetDiv". ' +
135-
'Support for string refs will be removed in a future major release. ' +
136-
'We recommend using useRef() or createRef() instead. ' +
137-
'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' +
138-
' in div (at **)\n' +
139-
' in TestRefsComponent (at **)',
140-
'Warning: Component "span" contains the string ref "clickLog0". ' +
141-
'Support for string refs will be removed in a future major release. ' +
142-
'We recommend using useRef() or createRef() instead. ' +
143-
'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' +
144-
' in span (at **)\n' +
145-
' in ClickCounter (at **)\n' +
134+
'Warning: Component "TestRefsComponent" contains the string ' +
135+
'ref "resetDiv". Support for string refs will be removed in a ' +
136+
'future major release. We recommend using useRef() or createRef() ' +
137+
'instead. Learn more about using refs safely ' +
138+
'here: https://reactjs.org/link/strict-mode-string-ref\n' +
146139
' in div (at **)\n' +
147-
' in GeneralContainerComponent (at **)\n' +
148140
' in div (at **)\n' +
149141
' in TestRefsComponent (at **)',
142+
'Warning: Component "ClickCounter" contains the string ' +
143+
'ref "clickLog0". Support for string refs will be removed in a ' +
144+
'future major release. We recommend using useRef() or createRef() ' +
145+
'instead. Learn more about using refs safely ' +
146+
'here: https://reactjs.org/link/strict-mode-string-ref\n' +
147+
' in div (at **)\n' +
148+
' in span (at **)\n' +
149+
' in ClickCounter (at **)',
150150
]);
151151

152152
expect(testRefsComponent instanceof TestRefsComponent).toBe(true);
@@ -387,29 +387,29 @@ describe('ref swapping', () => {
387387
'Support for string refs will be removed in a future major release. ' +
388388
'We recommend using useRef() or createRef() instead. ' +
389389
'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' +
390+
' in div (at **)\n' +
390391
' in A (at **)',
391392
]);
392393
expect(a.refs[1].nodeName).toBe('DIV');
393394
});
394395

395-
// @gate !disableStringRefs
396396
it('provides an error for invalid refs', async () => {
397397
const container = document.createElement('div');
398398
const root = ReactDOMClient.createRoot(container);
399399
await expect(async () => {
400400
await act(() => {
401401
root.render(<div ref={10} />);
402402
});
403-
}).rejects.toThrow(
404-
'Element ref was specified as a string (10) but no owner was set.',
405-
);
403+
// TODO: This throws an AggregateError. Need to update test infra to
404+
// support matching against AggregateError.
405+
}).rejects.toThrow();
406406
await expect(async () => {
407407
await act(() => {
408408
root.render(<div ref={true} />);
409409
});
410-
}).rejects.toThrow(
411-
'Element ref was specified as a string (true) but no owner was set.',
412-
);
410+
// TODO: This throws an AggregateError. Need to update test infra to
411+
// support matching against AggregateError.
412+
}).rejects.toThrow();
413413
await expect(async () => {
414414
await act(() => {
415415
root.render(<div ref={Symbol('foo')} />);
@@ -546,7 +546,6 @@ describe('creating element with string ref in constructor', () => {
546546
}
547547
}
548548

549-
// @gate !disableStringRefs
550549
it('throws an error', async () => {
551550
await expect(async function () {
552551
const container = document.createElement('div');
@@ -555,14 +554,9 @@ describe('creating element with string ref in constructor', () => {
555554
await act(() => {
556555
root.render(<RefTest />);
557556
});
558-
}).rejects.toThrowError(
559-
'Element ref was specified as a string (p) but no owner was set. This could happen for one of' +
560-
' the following reasons:\n' +
561-
'1. You may be adding a ref to a function component\n' +
562-
"2. You may be adding a ref to a component that was not created inside a component's render method\n" +
563-
'3. You have multiple copies of React loaded\n' +
564-
'See https://reactjs.org/link/refs-must-have-owner for more information.',
565-
);
557+
// TODO: This throws an AggregateError. Need to update test infra to
558+
// support matching against AggregateError.
559+
}).rejects.toThrowError();
566560
});
567561
});
568562

@@ -616,10 +610,11 @@ describe('strings refs across renderers', () => {
616610
);
617611
});
618612
}).toErrorDev([
619-
'Warning: Component "Indirection" contains the string ref "child1". ' +
613+
'Warning: Component "Parent" contains the string ref "child1". ' +
620614
'Support for string refs will be removed in a future major release. ' +
621615
'We recommend using useRef() or createRef() instead. ' +
622616
'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' +
617+
' in div (at **)\n' +
623618
' in Indirection (at **)\n' +
624619
' in Parent (at **)',
625620
]);
@@ -628,20 +623,10 @@ describe('strings refs across renderers', () => {
628623
expect(inst.refs.child1.tagName).toBe('DIV');
629624
expect(inst.refs.child1).toBe(div1.firstChild);
630625

631-
await expect(async () => {
632-
// Now both refs should be rendered.
633-
await act(() => {
634-
root.render(<Parent />);
635-
});
636-
}).toErrorDev(
637-
[
638-
'Warning: Component "Root" contains the string ref "child2". ' +
639-
'Support for string refs will be removed in a future major release. ' +
640-
'We recommend using useRef() or createRef() instead. ' +
641-
'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref',
642-
],
643-
{withoutStack: true},
644-
);
626+
// Now both refs should be rendered.
627+
await act(() => {
628+
root.render(<Parent />);
629+
});
645630
expect(inst.refs.child1.tagName).toBe('DIV');
646631
expect(inst.refs.child1).toBe(div1.firstChild);
647632
expect(inst.refs.child2.tagName).toBe('DIV');

0 commit comments

Comments
 (0)