Dedupe warnings about refs on functional components (#8739)

gaearon · web-flow · commit bd23aa271253 · 2017-01-10T09:54:48.000-08:00
* 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.
diff --git 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
@@ -67,6 +67,8 @@ var warning = require('warning');
 
 if (__DEV__) {
   var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');
+
+  var warnedAboutStatelessRefs = {};
 }
 
 module.exports = function<T, P, I, TI, C, CX, CI>(
@@ -464,13 +466,21 @@ module.exports = function<T, P, I, TI, C, CX, CI>(
             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
@@ -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 <div>{props.children}</div>;
     }
 
-    class Parent extends React.Component {
+    class ParentUsingStringRef extends React.Component {
       render() {
-        return <Indirection><StatelessComponent name="A" ref="stateless"/></Indirection>;
+        return (
+          <Indirection>
+            <StatelessComponent name="A" ref="stateless" />
+          </Indirection>
+        );
       }
     }
 
-    ReactTestUtils.renderIntoDocument(<Parent/>);
-
+    ReactTestUtils.renderIntoDocument(<ParentUsingStringRef />);
     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(<ParentUsingStringRef />);
+    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 <div>{props.children}</div>;
     }
 
-    class Parent extends React.Component {
+    class ParentUsingFunctionRef extends React.Component {
       render() {
-        return <Indirection><StatelessComponent name="A" ref={ref} /></Indirection>;
+        return (
+          <Indirection>
+            <StatelessComponent name="A" ref={(arg) => {
+              expect(arg).toBe(null);
+            }} />
+          </Indirection>
+        );
       }
     }
 
-    ReactTestUtils.renderIntoDocument(<Parent/>);
-
+    ReactTestUtils.renderIntoDocument(<ParentUsingFunctionRef />);
     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(<ParentUsingFunctionRef />);
+    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 <StatelessComponent name="A" ref={() => {}} />;
+      },
+    });
+    const instance1 = ReactTestUtils.renderIntoDocument(<AnonymousParentUsingJSX />);
+    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(<AnonymousParentUsingJSX />);
+    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(<AnonymousParentNotUsingJSX />);
+    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(<AnonymousParentNotUsingJSX />);
+    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(<NamedParentNotUsingJSX />);
+    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(<NamedParentNotUsingJSX />);
+    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
@@ -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 + '`.';
+        }
+      }
+
+      let warningKey = ownerName || component._debugID;
+      let element = component._currentElement;
+      if (element && element._source) {
+        warningKey = element._source.fileName + ':' + element._source.lineNumber;
       }
-      if (ownerName) {
-        info += ' Check the render method of `' + ownerName + '`.';
+      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') {
