SimpleMemoComponent should warn if a ref is given (#14178)

Fixes #13964.
facebook · Nov 9, 2018 · 1a6ab1e · 1a6ab1e
1 parent 8ae867e
commit 1a6ab1e
Show file tree

Hide file tree

Showing 2 changed files with 100 additions and 63 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js
@@ -250,6 +250,9 @@ function updateMemoComponent(
       // to a SimpleMemoComponent to allow fast path updates.
       workInProgress.tag = SimpleMemoComponent;
       workInProgress.type = type;
+      if (__DEV__) {
+        validateFunctionComponentInDev(workInProgress, type);
+      }
       return updateSimpleMemoComponent(
         current,
         workInProgress,
@@ -990,68 +993,72 @@ function mountIndeterminateComponent(
     // Proceed under the assumption that this is a function component
     workInProgress.tag = FunctionComponent;
     value = finishHooks(Component, props, value, context);
+    reconcileChildren(null, workInProgress, value, renderExpirationTime);
     if (__DEV__) {
-      if (Component) {
-        warningWithoutStack(
-          !Component.childContextTypes,
-          '%s(...): childContextTypes cannot be defined on a function component.',
-          Component.displayName || Component.name || 'Component',
-        );
-      }
-      if (workInProgress.ref !== null) {
-        let info = '';
-        const ownerName = ReactCurrentFiber.getCurrentFiberOwnerNameInDevOrNull();
-        if (ownerName) {
-          info += '\n\nCheck the render method of `' + ownerName + '`.';
-        }
+      validateFunctionComponentInDev(workInProgress, Component);
+    }
+    return workInProgress.child;
+  }
+}
 
-        let warningKey = ownerName || workInProgress._debugID || '';
-        const debugSource = workInProgress._debugSource;
-        if (debugSource) {
-          warningKey = debugSource.fileName + ':' + debugSource.lineNumber;
-        }
-        if (!didWarnAboutFunctionRefs[warningKey]) {
-          didWarnAboutFunctionRefs[warningKey] = true;
-          warning(
-            false,
-            'Function components cannot be given refs. ' +
-              'Attempts to access this ref will fail.%s',
-            info,
-          );
-        }
-      }
+function validateFunctionComponentInDev(workInProgress: Fiber, Component: any) {
+  if (Component) {
+    warningWithoutStack(
+      !Component.childContextTypes,
+      '%s(...): childContextTypes cannot be defined on a function component.',
+      Component.displayName || Component.name || 'Component',
+    );
+  }
+  if (workInProgress.ref !== null) {
+    let info = '';
+    const ownerName = ReactCurrentFiber.getCurrentFiberOwnerNameInDevOrNull();
+    if (ownerName) {
+      info += '\n\nCheck the render method of `' + ownerName + '`.';
+    }
+
+    let warningKey = ownerName || workInProgress._debugID || '';
+    const debugSource = workInProgress._debugSource;
+    if (debugSource) {
+      warningKey = debugSource.fileName + ':' + debugSource.lineNumber;
+    }
+    if (!didWarnAboutFunctionRefs[warningKey]) {
+      didWarnAboutFunctionRefs[warningKey] = true;
+      warning(
+        false,
+        'Function components cannot be given refs. ' +
+          'Attempts to access this ref will fail.%s',
+        info,
+      );
+    }
+  }
 
-      if (typeof Component.getDerivedStateFromProps === 'function') {
-        const componentName = getComponentName(Component) || 'Unknown';
+  if (typeof Component.getDerivedStateFromProps === 'function') {
+    const componentName = getComponentName(Component) || 'Unknown';
 
-        if (!didWarnAboutGetDerivedStateOnFunctionComponent[componentName]) {
-          warningWithoutStack(
-            false,
-            '%s: Function components do not support getDerivedStateFromProps.',
-            componentName,
-          );
-          didWarnAboutGetDerivedStateOnFunctionComponent[componentName] = true;
-        }
-      }
+    if (!didWarnAboutGetDerivedStateOnFunctionComponent[componentName]) {
+      warningWithoutStack(
+        false,
+        '%s: Function components do not support getDerivedStateFromProps.',
+        componentName,
+      );
+      didWarnAboutGetDerivedStateOnFunctionComponent[componentName] = true;
+    }
+  }
 
-      if (
-        typeof Component.contextType === 'object' &&
-        Component.contextType !== null
-      ) {
-        const componentName = getComponentName(Component) || 'Unknown';
+  if (
+    typeof Component.contextType === 'object' &&
+    Component.contextType !== null
+  ) {
+    const componentName = getComponentName(Component) || 'Unknown';
 
-        if (!didWarnAboutContextTypeOnFunctionComponent[componentName]) {
-          warningWithoutStack(
-            false,
-            '%s: Function components do not support contextType.',
-            componentName,
-          );
-          didWarnAboutContextTypeOnFunctionComponent[componentName] = true;
-        }
-      }
+    if (!didWarnAboutContextTypeOnFunctionComponent[componentName]) {
+      warningWithoutStack(
+        false,
+        '%s: Function components do not support contextType.',
+        componentName,
+      );
+      didWarnAboutContextTypeOnFunctionComponent[componentName] = true;
     }
-    reconcileChildren(null, workInProgress, value, renderExpirationTime);
-    return workInProgress.child;
   }
 }
 

diff --git a/packages/react-reconciler/src/__tests__/ReactMemo-test.internal.js b/packages/react-reconciler/src/__tests__/ReactMemo-test.internal.js
@@ -15,6 +15,7 @@
 let React;
 let ReactFeatureFlags;
 let ReactNoop;
+let Suspense;
 
 describe('memo', () => {
   beforeEach(() => {
@@ -23,6 +24,7 @@ describe('memo', () => {
     ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
     React = require('react');
     ReactNoop = require('react-noop-renderer');
+    ({Suspense} = React);
   });
 
   function span(prop) {
@@ -38,6 +40,42 @@ describe('memo', () => {
     return {default: result};
   }
 
+  it('warns when giving a ref (simple)', async () => {
+    // This test lives outside sharedTests because the wrappers don't forward
+    // refs properly, and they end up affecting the current owner which is used
+    // by the warning (making the messages not line up).
+    function App() {
+      return null;
+    }
+    App = React.memo(App);
+    function Outer() {
+      return <App ref={() => {}} />;
+    }
+    ReactNoop.render(<Outer />);
+    expect(ReactNoop.flush).toWarnDev([
+      'Warning: Function components cannot be given refs. Attempts to access ' +
+        'this ref will fail.',
+    ]);
+  });
+
+  it('warns when giving a ref (complex)', async () => {
+    // defaultProps means this won't use SimpleMemoComponent (as of this writing)
+    // SimpleMemoComponent is unobservable tho, so we can't check :)
+    function App() {
+      return null;
+    }
+    App.defaultProps = {};
+    App = React.memo(App);
+    function Outer() {
+      return <App ref={() => {}} />;
+    }
+    ReactNoop.render(<Outer />);
+    expect(ReactNoop.flush).toWarnDev([
+      'Warning: Function components cannot be given refs. Attempts to access ' +
+        'this ref will fail.',
+    ]);
+  });
+
   // Tests should run against both the lazy and non-lazy versions of `memo`.
   // To make the tests work for both versions, we wrap the non-lazy version in
   // a lazy function component.
@@ -56,8 +94,6 @@ describe('memo', () => {
   function sharedTests(label, memo) {
     describe(`${label}`, () => {
       it('bails out on props equality', async () => {
-        const {Suspense} = React;
-
         function Counter({count}) {
           return <Text text={count} />;
         }
@@ -93,8 +129,6 @@ describe('memo', () => {
       });
 
       it("does not bail out if there's a context change", async () => {
-        const {Suspense} = React;
-
         const CountContext = React.createContext(0);
 
         function readContext(Context) {
@@ -142,8 +176,6 @@ describe('memo', () => {
       });
 
       it('accepts custom comparison function', async () => {
-        const {Suspense} = React;
-
         function Counter({count}) {
           return <Text text={count} />;
         }
@@ -184,8 +216,6 @@ describe('memo', () => {
       });
 
       it('supports non-pure class components', async () => {
-        const {Suspense} = React;
-
         class CounterInner extends React.Component {
           static defaultProps = {suffix: '!'};
           render() {