Warn for callback refs on functional components (Stack + Fiber) (#8635)

* Fiber: warn for refs on SFCs

* Stateless refs: update warning to use component stack

* Warn for function refs (stack and fiber)

* Add owner reference to ref warnings

* Centralize stack ref warnings in ReactRef.attachRef

* Fiber stateless comp ref warning should only do work when necessary

* ReactDebugCurrentFiber maybe FunctionalComponents should act this way instead

* (chore) scripts/fiber/record-tests

* Add component._compositeType to ReactInstance Flow definition

* Don't handle 'stack inside fiber' case in the warning

We don't have a test for it. It's easy to mess it up and print the wrong thing so instead of verifying it I'll just remove this bit.

* Revert the change to getCurrentFiberOwnerName

This happened to work, but it is just a coincidence. This change didn’t really match what the function was supposed to be doing.

I’m not sure what the correct fix would be yet so this commit breaks tests.

* Add component indirection to the tests using owner name

This passes in Stack. It helps ensure we supply the correct owner name.

* Invalid type invariant should read owner from element

This brings the Fiber behavior in line with how Stack does it. The Fiber test was passing accidentally but broke down in more complicated cases (such as when we have an <Indirection> between the owner and the failing element).

Now we can also remove the weird cases from getCurrentFiberOwnerName() that didn't really make sense but helped get the (incomplete) tests pass in the past.

* Fiber should throw on a string ref inside functional component

scripts/fiber/tests-failing.txt

@@ -66,7 +66,6 @@ src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js
 
 src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js
 * should warn when stateless component returns array
-* should throw on string refs in pure functions
 * should warn when using non-React functions in JSX
 
 src/renderers/shared/shared/__tests__/ReactUpdates-test.js

scripts/fiber/tests-passing-except-dev.txt

@@ -116,4 +116,3 @@ src/renderers/shared/shared/__tests__/ReactMultiChild-test.js
 
 src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js
 * should warn for childContextTypes on a functional component
-* should warn when given a ref

scripts/fiber/tests-passing.txt

@@ -1535,6 +1535,9 @@ src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js
 * should update stateless component
 * should unmount stateless component
 * should pass context thru stateless component
+* should throw on string refs in pure functions
+* should warn when given a string ref
+* should warn when given a function ref
 * should provide a null ref
 * should use correct name in key warning
 * should support default props and prop types

src/renderers/shared/fiber/ReactChildFiber.js

@@ -78,14 +78,16 @@ function coerceRef(current: ?Fiber, element: ReactElement) {
   let mixedRef = element.ref;
   if (mixedRef != null && typeof mixedRef !== 'function') {
     if (element._owner) {
-      const ownerFiber : ?(Fiber | ReactInstance) = (element._owner : any);
+      const owner : ?(Fiber | ReactInstance) = (element._owner : any);
       let inst;
-      if (ownerFiber) {
-        if ((ownerFiber : any).tag === ClassComponent) {
-          inst = (ownerFiber : any).stateNode;
+      if (owner) {
+        if (typeof owner.tag === 'number') {
+          const ownerFiber = ((owner : any) : Fiber);
+          invariant(ownerFiber.tag === ClassComponent, 'Stateless function components cannot have refs.');
+          inst = ownerFiber.stateNode;
         } else {
           // Stack
-          inst = (ownerFiber : any).getPublicInstance();
+          inst = (owner : any).getPublicInstance();
         }
       }
       invariant(inst, 'Missing owner for string ref %s', mixedRef);

src/renderers/shared/fiber/ReactDebugCurrentFiber.js

@@ -15,12 +15,6 @@
 import type { Fiber } from 'ReactFiber';
 
 if (__DEV__) {
-  var {
-    IndeterminateComponent,
-    FunctionalComponent,
-    ClassComponent,
-    HostComponent,
-  } = require('ReactTypeOfWork');
   var getComponentName = require('getComponentName');
   var { getStackAddendumByWorkInProgressFiber } = require('ReactComponentTreeHook');
 }
@@ -31,18 +25,8 @@ function getCurrentFiberOwnerName() : string | null {
     if (fiber == null) {
       return null;
     }
-    switch (fiber.tag) {
-      case IndeterminateComponent:
-      case FunctionalComponent:
-      case ClassComponent:
-        return getComponentName(fiber);
-      case HostComponent:
-        if (fiber._debugOwner != null) {
-          return getComponentName(fiber._debugOwner);
-        }
-        return null;
-      default:
-        return null;
+    if (fiber._debugOwner != null) {
+      return getComponentName(fiber._debugOwner);
     }
   }
   return null;

src/renderers/shared/fiber/ReactFiber.js

@@ -50,7 +50,7 @@ var {
 var invariant = require('invariant');
 
 if (__DEV__) {
-  var { getCurrentFiberOwnerName } = require('ReactDebugCurrentFiber');
+  var getComponentName = require('getComponentName');
 }
 
 // A Fiber is work on a Component that needs to be done or was done. There can
@@ -300,7 +300,12 @@ exports.createHostRootFiber = function() : Fiber {
 };
 
 exports.createFiberFromElement = function(element : ReactElement, priorityLevel : PriorityLevel) : Fiber {
-  const fiber = createFiberFromElementType(element.type, element.key);
+  let owner = null;
+  if (__DEV__) {
+    owner = element._owner;
+  }
+
+  const fiber = createFiberFromElementType(element.type, element.key, owner);
   fiber.pendingProps = element.props;
   fiber.pendingWorkPriority = priorityLevel;
 
@@ -328,7 +333,7 @@ exports.createFiberFromText = function(content : string, priorityLevel : Priorit
   return fiber;
 };
 
-function createFiberFromElementType(type : mixed, key : null | string) : Fiber {
+function createFiberFromElementType(type : mixed, key : null | string, debugOwner : null | Fiber | ReactInstance) : Fiber {
   let fiber;
   if (typeof type === 'function') {
     fiber = shouldConstruct(type) ?
@@ -363,7 +368,7 @@ function createFiberFromElementType(type : mixed, key : null | string) : Fiber {
           ' You likely forgot to export your component from the file ' +
           'it\'s defined in.';
       }
-      const ownerName = getCurrentFiberOwnerName();
+      const ownerName = debugOwner ? getComponentName(debugOwner) : null;
       if (ownerName) {
         info += ' Check the render method of `' + ownerName + '`.';
       }

src/renderers/shared/fiber/ReactFiberBeginWork.js

@@ -63,6 +63,7 @@ var {
 } = require('ReactTypeOfSideEffect');
 var ReactCurrentOwner = require('ReactCurrentOwner');
 var ReactFiberClassComponent = require('ReactFiberClassComponent');
+var warning = require('warning');
 
 if (__DEV__) {
   var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');
@@ -455,6 +456,23 @@ module.exports = function<T, P, I, TI, C, CX, CI>(
     } else {
       // Proceed under the assumption that this is a functional component
       workInProgress.tag = FunctionalComponent;
+      if (__DEV__) {
+        if (workInProgress.ref != null) {
+          let info = '';
+          const ownerName = ReactDebugCurrentFiber.getCurrentFiberOwnerName();
+          if (ownerName) {
+            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()
+          );
+        }
+      }
       reconcileChildren(current, workInProgress, value);
       return workInProgress.child;
     }

src/renderers/shared/fiber/ReactReifiedYield.js

@@ -22,7 +22,8 @@ export type ReifiedYield = { continuation: Fiber, props: Object };
 exports.createReifiedYield = function(yieldNode : ReactYield) : ReifiedYield {
   var fiber = createFiberFromElementType(
     yieldNode.continuation,
-    yieldNode.key
+    yieldNode.key,
+    null // debugOwner
   );
   return {
     continuation: fiber,
@@ -35,7 +36,8 @@ exports.createUpdatedReifiedYield = function(previousYield : ReifiedYield, yield
   if (fiber.type !== yieldNode.continuation) {
     fiber = createFiberFromElementType(
       yieldNode.continuation,
-      yieldNode.key
+      yieldNode.key,
+      null // debugOwner
     );
   }
   return {

src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js

@@ -739,9 +739,8 @@ describe('ReactIncrementalErrorHandling', () => {
       }
     }
     const InvalidType = undefined;
-    const brokenElement = <InvalidType />;
     function BrokenRender(props) {
-      return brokenElement;
+      return <InvalidType />;
     }
 
     ReactNoop.render(
@@ -776,9 +775,8 @@ describe('ReactIncrementalErrorHandling', () => {
     }
 
     const InvalidType = undefined;
-    const brokenElement = <InvalidType />;
     function BrokenRender(props) {
-      return props.fail ? brokenElement : <span />;
+      return props.fail ? <InvalidType /> : <span />;
     }
 
     ReactNoop.render(

src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js

@@ -21,6 +21,10 @@ describe('ReactIncrementalSideEffects', () => {
     ReactNoop = require('ReactNoop');
   });
 
+  function normalizeCodeLocInfo(str) {
+    return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
+  }
+
   function div(...children) {
     children = children.map(c => typeof c === 'string' ? { text: c } : c);
     return { type: 'div', children, prop: undefined };
@@ -1069,7 +1073,7 @@ describe('ReactIncrementalSideEffects', () => {
   });
 
   it('invokes ref callbacks after insertion/update/unmount', () => {
-
+    spyOn(console, 'error');
     var classInstance = null;
 
     var ops = [];
@@ -1129,6 +1133,14 @@ describe('ReactIncrementalSideEffects', () => {
       null,
     ]);
 
+    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 `Foo`.\n' +
+      '    in FunctionalComponent (at **)\n' +
+      '    in div (at **)\n' +
+      '    in Foo (at **)'
+    );
   });
 
   // TODO: Test that mounts, updates, refs, unmounts and deletions happen in the

src/renderers/shared/shared/__tests__/ReactComponent-test.js

@@ -352,16 +352,25 @@ describe('ReactComponent', () => {
   it('includes owner name in the error about badly-typed elements', () => {
     spyOn(console, 'error');
 
+    var X = undefined;
+
+    function Indirection(props) {
+      return <div>{props.children}</div>;
+    }
+
+    function Bar() {
+      return <Indirection><X /></Indirection>;
+    }
+
     function Foo() {
-      var X = undefined;
-      return <X />;
+      return <Bar />;
     }
 
     expect(() => ReactTestUtils.renderIntoDocument(<Foo />)).toThrowError(
       'Element type is invalid: expected a string (for built-in components) ' +
       'or a class/function (for composite components) but got: undefined. ' +
       'You likely forgot to export your component from the file it\'s ' +
-      'defined in. Check the render method of `Foo`.'
+      'defined in. Check the render method of `Bar`.'
     );
 
     // One warning for each element creation

src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js

@@ -20,6 +20,9 @@ function StatelessComponent(props) {
 }
 
 describe('ReactStatelessComponent', () => {
+  function normalizeCodeLocInfo(str) {
+    return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
+  }
 
   beforeEach(() => {
     React = require('React');
@@ -146,24 +149,60 @@ describe('ReactStatelessComponent', () => {
     );
   });
 
-  it('should warn when given a ref', () => {
+  it('should warn when given a string ref', () => {
     spyOn(console, 'error');
 
-    class Parent extends React.Component {
-      static displayName = 'Parent';
+    function Indirection(props) {
+      return <div>{props.children}</div>;
+    }
 
+    class Parent extends React.Component {
       render() {
-        return <StatelessComponent name="A" ref="stateless"/>;
+        return <Indirection><StatelessComponent name="A" ref="stateless"/></Indirection>;
       }
     }
 
     ReactTestUtils.renderIntoDocument(<Parent/>);
 
     expectDev(console.error.calls.count()).toBe(1);
-    expectDev(console.error.calls.argsFor(0)[0]).toContain(
-      'Stateless function components cannot be given refs ' +
-      '(See ref "stateless" in StatelessComponent created by Parent). ' +
-      'Attempts to access this ref will fail.'
+    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' +
+      '    in StatelessComponent (at **)\n' +
+      '    in div (at **)\n' +
+      '    in Indirection (at **)\n' +
+      '    in Parent (at **)'
+    );
+  });
+
+  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 {
+      render() {
+        return <Indirection><StatelessComponent name="A" ref={ref} /></Indirection>;
+      }
+    }
+
+    ReactTestUtils.renderIntoDocument(<Parent/>);
+
+    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' +
+      '    in StatelessComponent (at **)\n' +
+      '    in div (at **)\n' +
+      '    in Indirection (at **)\n' +
+      '    in Parent (at **)'
     );
   });
 

src/renderers/shared/stack/reconciler/ReactCompositeComponent.js

@@ -13,6 +13,7 @@
 
 var React = require('React');
 var ReactComponentEnvironment = require('ReactComponentEnvironment');
+var ReactCompositeComponentTypes = require('ReactCompositeComponentTypes');
 var ReactCurrentOwner = require('ReactCurrentOwner');
 var ReactErrorUtils = require('ReactErrorUtils');
 var ReactFeatureFlags = require('ReactFeatureFlags');
@@ -33,12 +34,6 @@ var warning = require('warning');
 
 import type { ReactPropTypeLocations } from 'ReactPropTypeLocations';
 
-var CompositeTypes = {
-  ImpureClass: 0,
-  PureClass: 1,
-  StatelessFunctional: 2,
-};
-
 function StatelessComponent(Component) {
 }
 StatelessComponent.prototype.render = function() {
@@ -220,12 +215,12 @@ var ReactCompositeComponent = {
         Component.displayName || Component.name || 'Component'
       );
       inst = new StatelessComponent(Component);
-      this._compositeType = CompositeTypes.StatelessFunctional;
+      this._compositeType = ReactCompositeComponentTypes.StatelessFunctional;
     } else {
       if (isPureComponent(Component)) {
-        this._compositeType = CompositeTypes.PureClass;
+        this._compositeType = ReactCompositeComponentTypes.PureClass;
       } else {
-        this._compositeType = CompositeTypes.ImpureClass;
+        this._compositeType = ReactCompositeComponentTypes.ImpureClass;
       }
     }
 
@@ -881,7 +876,7 @@ var ReactCompositeComponent = {
           shouldUpdate = inst.shouldComponentUpdate(nextProps, nextState, nextContext);
         }
       } else {
-        if (this._compositeType === CompositeTypes.PureClass) {
+        if (this._compositeType === ReactCompositeComponentTypes.PureClass) {
           shouldUpdate =
             !shallowEqual(prevProps, nextProps) ||
             !shallowEqual(inst.state, nextState);
@@ -1215,7 +1210,7 @@ var ReactCompositeComponent = {
    */
   _renderValidatedComponent: function() {
     var renderedElement;
-    if (__DEV__ || this._compositeType !== CompositeTypes.StatelessFunctional) {
+    if (__DEV__ || this._compositeType !== ReactCompositeComponentTypes.StatelessFunctional) {
       ReactCurrentOwner.current = this;
       try {
         renderedElement =
@@ -1251,20 +1246,6 @@ var ReactCompositeComponent = {
     var inst = this.getPublicInstance();
     invariant(inst != null, 'Stateless function components cannot have refs.');
     var publicComponentInstance = component.getPublicInstance();
-    if (__DEV__) {
-      var componentName = component && component.getName ?
-        component.getName() : 'a component';
-      warning(
-        publicComponentInstance != null ||
-        component._compositeType !== CompositeTypes.StatelessFunctional,
-        'Stateless function components cannot be given refs ' +
-        '(See ref "%s" in %s created by %s). ' +
-        'Attempts to access this ref will fail.',
-        ref,
-        componentName,
-        this.getName()
-      );
-    }
     var refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs;
     refs[ref] = publicComponentInstance;
   },
@@ -1307,7 +1288,7 @@ var ReactCompositeComponent = {
    */
   getPublicInstance: function() {
     var inst = this._instance;
-    if (this._compositeType === CompositeTypes.StatelessFunctional) {
+    if (this._compositeType === ReactCompositeComponentTypes.StatelessFunctional) {
       return null;
     }
     return inst;

src/renderers/shared/stack/reconciler/ReactCompositeComponentTypes.js

@@ -0,0 +1,19 @@
+/**
+ * Copyright 2013-present, Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD-style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ *
+ * @providesModule ReactCompositeComponentTypes
+ * @flow
+ */
+
+export type CompositeComponentTypes = 0 | 1 | 2;
+
+module.exports = {
+  ImpureClass: 0,
+  PureClass: 1,
+  StatelessFunctional: 2,
+};

src/renderers/shared/stack/reconciler/ReactInstanceType.js

@@ -13,6 +13,7 @@
 'use strict';
 
 import type {ReactElement} from 'ReactElementType';
+import type {CompositeComponentTypes} from 'ReactCompositeComponentTypes';
 
 export type DebugID = number;
 
@@ -34,6 +35,7 @@ export type ReactInstance = {
   attachRef: (ref: string, component: ReactInstance) => void,
   detachRef: (ref: string) => void,
   _rootNodeID: number,
+  _compositeType: CompositeComponentTypes,
 
   // ReactDOMComponent
   _tag: string,

src/renderers/shared/stack/reconciler/ReactRef.js

@@ -19,7 +19,34 @@ import type { ReactElement } from 'ReactElementType';
 
 var ReactRef = {};
 
+if (__DEV__) {
+  var ReactCompositeComponentTypes = require('ReactCompositeComponentTypes');
+  var ReactComponentTreeHook = require('ReactComponentTreeHook');
+  var warning = require('warning');
+}
+
 function attachRef(ref, component, owner) {
+  if (__DEV__) {
+    let info = '';
+    if (owner) {
+      let ownerName;
+      if (typeof owner.getName === 'function') {
+        ownerName = owner.getName();
+      }
+      if (ownerName) {
+        info += ' Check the render method of `' + ownerName + '`.';
+      }
+    }
+
+    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') {
     ref(component.getPublicInstance());
   } else {

src/renderers/testing/__tests__/ReactTestRenderer-test.js

@@ -15,6 +15,9 @@ var React = require('React');
 var ReactTestRenderer = require('ReactTestRenderer');
 
 describe('ReactTestRenderer', () => {
+  function normalizeCodeLocInfo(str) {
+    return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
+  }
 
   it('renders a simple component', () => {
     function Link() {
@@ -231,10 +234,11 @@ describe('ReactTestRenderer', () => {
     ReactTestRenderer.create(<Baz />);
     ReactTestRenderer.create(<Foo />);
     expectDev(console.error.calls.count()).toBe(1);
-    expectDev(console.error.calls.argsFor(0)[0]).toContain(
-      'Stateless function components cannot be given refs ' +
-      '(See ref "foo" in Bar created by Foo). ' +
-      'Attempts to access this ref will fail.'
+    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 `Foo`.\n' +
+      '    in Bar (at **)\n' +
+      '    in Foo (at **)'
     );
   });