From bd23aa2712531dedbb907ff087c19e2868bb57ee Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 10 Jan 2017 09:54:48 -0800 Subject: [PATCH] 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. --- scripts/fiber/tests-passing.txt | 1 + .../shared/fiber/ReactFiberBeginWork.js | 24 ++-- .../__tests__/ReactStatelessComponent-test.js | 108 +++++++++++++++--- .../shared/stack/reconciler/ReactRef.js | 40 ++++--- 4 files changed, 137 insertions(+), 36 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index e8d3b4ae75..8370d37105 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1538,6 +1538,7 @@ src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js * should throw on string refs in pure functions * should warn when given a string ref * should warn when given a function ref +* deduplicates ref warnings based on element or owner * should provide a null ref * should use correct name in key warning * should support default props and prop types diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index db863f1783..9401be93d1 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -67,6 +67,8 @@ var warning = require('warning'); if (__DEV__) { var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); + + var warnedAboutStatelessRefs = {}; } module.exports = function( @@ -464,13 +466,21 @@ module.exports = function( info += ' Check the render method of `' + ownerName + '`.'; } - warning( - false, - 'Stateless function components cannot be given refs. ' + - 'Attempts to access this ref will fail.%s%s', - info, - ReactDebugCurrentFiber.getCurrentFiberStackAddendum() - ); + let warningKey = ownerName || workInProgress._debugID || ''; + const debugSource = workInProgress._debugSource; + if (debugSource) { + warningKey = debugSource.fileName + ':' + debugSource.lineNumber; + } + if (!warnedAboutStatelessRefs[warningKey]) { + warnedAboutStatelessRefs[warningKey] = true; + warning( + false, + 'Stateless function components cannot be given refs. ' + + 'Attempts to access this ref will fail.%s%s', + info, + ReactDebugCurrentFiber.getCurrentFiberStackAddendum() + ); + } } } reconcileChildren(current, workInProgress, value); diff --git a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js index ec63bbd70b..060ec46137 100644 --- a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js @@ -25,6 +25,7 @@ describe('ReactStatelessComponent', () => { } beforeEach(() => { + jest.resetModuleRegistry(); React = require('React'); ReactDOM = require('ReactDOM'); ReactTestUtils = require('ReactTestUtils'); @@ -156,54 +157,131 @@ describe('ReactStatelessComponent', () => { return
{props.children}
; } - class Parent extends React.Component { + class ParentUsingStringRef extends React.Component { render() { - return ; + return ( + + + + ); } } - ReactTestUtils.renderIntoDocument(); - + ReactTestUtils.renderIntoDocument(); expectDev(console.error.calls.count()).toBe(1); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( 'Warning: Stateless function components cannot be given refs. ' + 'Attempts to access this ref will fail. Check the render method ' + - 'of `Parent`.\n' + + 'of `ParentUsingStringRef`.\n' + ' in StatelessComponent (at **)\n' + ' in div (at **)\n' + ' in Indirection (at **)\n' + - ' in Parent (at **)' + ' in ParentUsingStringRef (at **)' ); + + ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(1); }); it('should warn when given a function ref', () => { spyOn(console, 'error'); - var ref = jasmine.createSpy().and.callFake((arg) => { - expect(arg).toBe(null); - }); function Indirection(props) { return
{props.children}
; } - class Parent extends React.Component { + class ParentUsingFunctionRef extends React.Component { render() { - return ; + return ( + + { + expect(arg).toBe(null); + }} /> + + ); } } - ReactTestUtils.renderIntoDocument(); - + ReactTestUtils.renderIntoDocument(); expectDev(console.error.calls.count()).toBe(1); expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( 'Warning: Stateless function components cannot be given refs. ' + 'Attempts to access this ref will fail. Check the render method ' + - 'of `Parent`.\n' + + 'of `ParentUsingFunctionRef`.\n' + ' in StatelessComponent (at **)\n' + ' in div (at **)\n' + ' in Indirection (at **)\n' + - ' in Parent (at **)' + ' in ParentUsingFunctionRef (at **)' ); + + ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(1); + }); + + it('deduplicates ref warnings based on element or owner', () => { + spyOn(console, 'error'); + + // Prevent the Babel transform adding a displayName. + var createClassWithoutDisplayName = React.createClass; + + // When owner uses JSX, we can use exact line location to dedupe warnings + var AnonymousParentUsingJSX = createClassWithoutDisplayName({ + render() { + return {}} />; + }, + }); + const instance1 = ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Stateless function components cannot be given refs.' + ); + // Should be deduped (offending element is on the same line): + instance1.forceUpdate(); + // Should also be deduped (offending element is on the same line): + ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(1); + console.error.calls.reset(); + + // When owner doesn't use JSX, and is anonymous, we warn once per internal instance. + var AnonymousParentNotUsingJSX = createClassWithoutDisplayName({ + render() { + return React.createElement(StatelessComponent, {name: 'A', 'ref': () => {}}); + }, + }); + const instance2 = ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Stateless function components cannot be given refs.' + ); + // Should be deduped (same internal instance): + instance2.forceUpdate(); + expectDev(console.error.calls.count()).toBe(1); + // Could not be deduped (different internal instance): + ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(2); + expectDev(console.error.calls.argsFor(1)[0]).toContain( + 'Warning: Stateless function components cannot be given refs.' + ); + console.error.calls.reset(); + + // When owner doesn't use JSX, but is named, we warn once per owner name + class NamedParentNotUsingJSX extends React.Component { + render() { + return React.createElement(StatelessComponent, {name: 'A', 'ref': () => {}}); + } + } + const instance3 = ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Warning: Stateless function components cannot be given refs.' + ); + // Should be deduped (same owner name): + instance3.forceUpdate(); + expectDev(console.error.calls.count()).toBe(1); + // Should also be deduped (same owner name): + ReactTestUtils.renderIntoDocument(); + expectDev(console.error.calls.count()).toBe(1); + console.error.calls.reset(); }); it('should provide a null ref', () => { diff --git a/src/renderers/shared/stack/reconciler/ReactRef.js b/src/renderers/shared/stack/reconciler/ReactRef.js index 762e29685c..6e1d3681b9 100644 --- a/src/renderers/shared/stack/reconciler/ReactRef.js +++ b/src/renderers/shared/stack/reconciler/ReactRef.js @@ -23,28 +23,40 @@ if (__DEV__) { var ReactCompositeComponentTypes = require('ReactCompositeComponentTypes'); var ReactComponentTreeHook = require('ReactComponentTreeHook'); var warning = require('warning'); + + var warnedAboutStatelessRefs = {}; } function attachRef(ref, component, owner) { if (__DEV__) { - let info = ''; - if (owner) { + if (component._compositeType === ReactCompositeComponentTypes.StatelessFunctional) { + let info = ''; let ownerName; - if (typeof owner.getName === 'function') { - ownerName = owner.getName(); + if (owner) { + if (typeof owner.getName === 'function') { + ownerName = owner.getName(); + } + if (ownerName) { + info += ' Check the render method of `' + ownerName + '`.'; + } } - if (ownerName) { - info += ' Check the render method of `' + ownerName + '`.'; + + let warningKey = ownerName || component._debugID; + let element = component._currentElement; + if (element && element._source) { + warningKey = element._source.fileName + ':' + element._source.lineNumber; + } + if (!warnedAboutStatelessRefs[warningKey]) { + warnedAboutStatelessRefs[warningKey] = true; + warning( + false, + 'Stateless function components cannot be given refs. ' + + 'Attempts to access this ref will fail.%s%s', + info, + ReactComponentTreeHook.getStackAddendumByID(component._debugID) + ); } } - - warning( - component._compositeType !== ReactCompositeComponentTypes.StatelessFunctional, - 'Stateless function components cannot be given refs. ' + - 'Attempts to access this ref will fail.%s%s', - info, - ReactComponentTreeHook.getStackAddendumByID(component._debugID) - ); } if (typeof ref === 'function') {