Dedupe warnings about refs on functional components (#8739)
* Verify that functional component ref warning is deduplicated It's not a big problem for string refs because the ref stays the same and the warning code path runs once per mount. However, it is super annoying for functional refs since they're often written as arrows, and thus the warning fires for every render. Both tests are currently failing since we're mounting twice, so even the string ref case prints warnings twice. * Extract the warning condition to the top level We don't want to run getStackAddendumByID() unless we actually know we're going to print the warning. This doesn't affect correctness. Just a performance fix. * Deduplicate warnings about refs on functional components This fixes the duplicate warnings and adds an additional test for corner cases. Our goal is to print one warning per one offending call site, when possible. We try to use the element source for deduplication first because it gives us the exact JSX call site location. If the element source is not available, we try to use the owner name for deduplication. If even owner name is unavailable, we try to use the functional component unique identifier for deduplication so that at least the warning is seen once per mounted component.
Dan Abramov committed
Jan 10, 2017 at 09:54 UTC
bd23aa2712531dedbb907ff087c19e2868bb57ee
4 files changed
+137
-36
scripts/fiber/tests-passing.txt
+1
@@ -1538,6 +1538,7 @@ src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js
1538
* should throw on string refs in pure functions
1539
* should warn when given a string ref
1540
* should warn when given a function ref
1541
+* deduplicates ref warnings based on element or owner
1542
* should provide a null ref
1543
* should use correct name in key warning
1544
* should support default props and prop types
src/renderers/shared/fiber/ReactFiberBeginWork.js
+17
-7
@@ -67,6 +67,8 @@ var warning = require('warning');
67
68
if (__DEV__) {
69
var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');
70
+
71
+ var warnedAboutStatelessRefs = {};
72
}
73
74
module.exports = function<T, P, I, TI, C, CX, CI>(
@@ -464,13 +466,21 @@ module.exports = function<T, P, I, TI, C, CX, CI>(
466
info += ' Check the render method of `' + ownerName + '`.';
467
}
468
467
- warning(
468
- false,
469
- 'Stateless function components cannot be given refs. ' +
470
- 'Attempts to access this ref will fail.%s%s',
471
- info,
472
- ReactDebugCurrentFiber.getCurrentFiberStackAddendum()
473
- );
469
+ let warningKey = ownerName || workInProgress._debugID || '';
470
+ const debugSource = workInProgress._debugSource;
471
+ if (debugSource) {
472
+ warningKey = debugSource.fileName + ':' + debugSource.lineNumber;
473
+ }
474
+ if (!warnedAboutStatelessRefs[warningKey]) {
475
+ warnedAboutStatelessRefs[warningKey] = true;
476
+ warning(
477
+ false,
478
+ 'Stateless function components cannot be given refs. ' +
479
+ 'Attempts to access this ref will fail.%s%s',
480
+ info,
481
+ ReactDebugCurrentFiber.getCurrentFiberStackAddendum()
482
+ );
483
+ }
484
}
485
}
486
reconcileChildren(current, workInProgress, value);
src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js
+93
-15
@@ -25,6 +25,7 @@ describe('ReactStatelessComponent', () => {
25
}
26
27
beforeEach(() => {
28
+ jest.resetModuleRegistry();
29
React = require('React');
30
ReactDOM = require('ReactDOM');
31
ReactTestUtils = require('ReactTestUtils');
@@ -156,54 +157,131 @@ describe('ReactStatelessComponent', () => {
157
return <div>{props.children}</div>;
158
}
159
159
- class Parent extends React.Component {
160
+ class ParentUsingStringRef extends React.Component {
161
render() {
161
- return <Indirection><StatelessComponent name="A" ref="stateless"/></Indirection>;
162
+ return (
163
+ <Indirection>
164
+ <StatelessComponent name="A" ref="stateless" />
165
+ </Indirection>
166
+ );
167
}
168
}
169
165
- ReactTestUtils.renderIntoDocument(<Parent/>);
166
-
170
+ ReactTestUtils.renderIntoDocument(<ParentUsingStringRef />);
171
expectDev(console.error.calls.count()).toBe(1);
172
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
173
'Warning: Stateless function components cannot be given refs. ' +
174
'Attempts to access this ref will fail. Check the render method ' +
171
- 'of `Parent`.\n' +
175
+ 'of `ParentUsingStringRef`.\n' +
176
' in StatelessComponent (at **)\n' +
177
' in div (at **)\n' +
178
' in Indirection (at **)\n' +
175
- ' in Parent (at **)'
179
+ ' in ParentUsingStringRef (at **)'
180
);
181
+
182
+ ReactTestUtils.renderIntoDocument(<ParentUsingStringRef />);
183
+ expectDev(console.error.calls.count()).toBe(1);
184
});
185
186
it('should warn when given a function ref', () => {
187
spyOn(console, 'error');
181
- var ref = jasmine.createSpy().and.callFake((arg) => {
182
- expect(arg).toBe(null);
183
- });
188
189
function Indirection(props) {
190
return <div>{props.children}</div>;
191
}
192
189
- class Parent extends React.Component {
193
+ class ParentUsingFunctionRef extends React.Component {
194
render() {
191
- return <Indirection><StatelessComponent name="A" ref={ref} /></Indirection>;
195
+ return (
196
+ <Indirection>
197
+ <StatelessComponent name="A" ref={(arg) => {
198
+ expect(arg).toBe(null);
199
+ }} />
200
+ </Indirection>
201
+ );
202
}
203
}
204
195
- ReactTestUtils.renderIntoDocument(<Parent/>);
196
-
205
+ ReactTestUtils.renderIntoDocument(<ParentUsingFunctionRef />);
206
expectDev(console.error.calls.count()).toBe(1);
207
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
208
'Warning: Stateless function components cannot be given refs. ' +
209
'Attempts to access this ref will fail. Check the render method ' +
201
- 'of `Parent`.\n' +
210
+ 'of `ParentUsingFunctionRef`.\n' +
211
' in StatelessComponent (at **)\n' +
212
' in div (at **)\n' +
213
' in Indirection (at **)\n' +
205
- ' in Parent (at **)'
214
+ ' in ParentUsingFunctionRef (at **)'
215
+ );
216
+
217
+ ReactTestUtils.renderIntoDocument(<ParentUsingFunctionRef />);
218
+ expectDev(console.error.calls.count()).toBe(1);
219
+ });
220
+
221
+ it('deduplicates ref warnings based on element or owner', () => {
222
+ spyOn(console, 'error');
223
+
224
+ // Prevent the Babel transform adding a displayName.
225
+ var createClassWithoutDisplayName = React.createClass;
226
+
227
+ // When owner uses JSX, we can use exact line location to dedupe warnings
228
+ var AnonymousParentUsingJSX = createClassWithoutDisplayName({
229
+ render() {
230
+ return <StatelessComponent name="A" ref={() => {}} />;
231
+ },
232
+ });
233
+ const instance1 = ReactTestUtils.renderIntoDocument(<AnonymousParentUsingJSX />);
234
+ expectDev(console.error.calls.count()).toBe(1);
235
+ expectDev(console.error.calls.argsFor(0)[0]).toContain(
236
+ 'Warning: Stateless function components cannot be given refs.'
237
);
238
+ // Should be deduped (offending element is on the same line):
239
+ instance1.forceUpdate();
240
+ // Should also be deduped (offending element is on the same line):
241
+ ReactTestUtils.renderIntoDocument(<AnonymousParentUsingJSX />);
242
+ expectDev(console.error.calls.count()).toBe(1);
243
+ console.error.calls.reset();
244
+
245
+ // When owner doesn't use JSX, and is anonymous, we warn once per internal instance.
246
+ var AnonymousParentNotUsingJSX = createClassWithoutDisplayName({
247
+ render() {
248
+ return React.createElement(StatelessComponent, {name: 'A', 'ref': () => {}});
249
+ },
250
+ });
251
+ const instance2 = ReactTestUtils.renderIntoDocument(<AnonymousParentNotUsingJSX />);
252
+ expectDev(console.error.calls.count()).toBe(1);
253
+ expectDev(console.error.calls.argsFor(0)[0]).toContain(
254
+ 'Warning: Stateless function components cannot be given refs.'
255
+ );
256
+ // Should be deduped (same internal instance):
257
+ instance2.forceUpdate();
258
+ expectDev(console.error.calls.count()).toBe(1);
259
+ // Could not be deduped (different internal instance):
260
+ ReactTestUtils.renderIntoDocument(<AnonymousParentNotUsingJSX />);
261
+ expectDev(console.error.calls.count()).toBe(2);
262
+ expectDev(console.error.calls.argsFor(1)[0]).toContain(
263
+ 'Warning: Stateless function components cannot be given refs.'
264
+ );
265
+ console.error.calls.reset();
266
+
267
+ // When owner doesn't use JSX, but is named, we warn once per owner name
268
+ class NamedParentNotUsingJSX extends React.Component {
269
+ render() {
270
+ return React.createElement(StatelessComponent, {name: 'A', 'ref': () => {}});
271
+ }
272
+ }
273
+ const instance3 = ReactTestUtils.renderIntoDocument(<NamedParentNotUsingJSX />);
274
+ expectDev(console.error.calls.count()).toBe(1);
275
+ expectDev(console.error.calls.argsFor(0)[0]).toContain(
276
+ 'Warning: Stateless function components cannot be given refs.'
277
+ );
278
+ // Should be deduped (same owner name):
279
+ instance3.forceUpdate();
280
+ expectDev(console.error.calls.count()).toBe(1);
281
+ // Should also be deduped (same owner name):
282
+ ReactTestUtils.renderIntoDocument(<NamedParentNotUsingJSX />);
283
+ expectDev(console.error.calls.count()).toBe(1);
284
+ console.error.calls.reset();
285
});
286
287
it('should provide a null ref', () => {
src/renderers/shared/stack/reconciler/ReactRef.js
+26
-14
@@ -23,28 +23,40 @@ if (__DEV__) {
23
var ReactCompositeComponentTypes = require('ReactCompositeComponentTypes');
24
var ReactComponentTreeHook = require('ReactComponentTreeHook');
25
var warning = require('warning');
26
+
27
+ var warnedAboutStatelessRefs = {};
28
}
29
30
function attachRef(ref, component, owner) {
31
if (__DEV__) {
30
- let info = '';
31
- if (owner) {
32
+ if (component._compositeType === ReactCompositeComponentTypes.StatelessFunctional) {
33
+ let info = '';
34
let ownerName;
33
- if (typeof owner.getName === 'function') {
34
- ownerName = owner.getName();
35
+ if (owner) {
36
+ if (typeof owner.getName === 'function') {
37
+ ownerName = owner.getName();
38
+ }
39
+ if (ownerName) {
40
+ info += ' Check the render method of `' + ownerName + '`.';
41
+ }
42
+ }
43
+
44
+ let warningKey = ownerName || component._debugID;
45
+ let element = component._currentElement;
46
+ if (element && element._source) {
47
+ warningKey = element._source.fileName + ':' + element._source.lineNumber;
48
}
36
- if (ownerName) {
37
- info += ' Check the render method of `' + ownerName + '`.';
49
+ if (!warnedAboutStatelessRefs[warningKey]) {
50
+ warnedAboutStatelessRefs[warningKey] = true;
51
+ warning(
52
+ false,
53
+ 'Stateless function components cannot be given refs. ' +
54
+ 'Attempts to access this ref will fail.%s%s',
55
+ info,
56
+ ReactComponentTreeHook.getStackAddendumByID(component._debugID)
57
+ );
58
}
59
}
40
-
41
- warning(
42
- component._compositeType !== ReactCompositeComponentTypes.StatelessFunctional,
43
- 'Stateless function components cannot be given refs. ' +
44
- 'Attempts to access this ref will fail.%s%s',
45
- info,
46
- ReactComponentTreeHook.getStackAddendumByID(component._debugID)
47
- );
60
}
61
62
if (typeof ref === 'function') {