Deprecate findDOMNode in StrictMode

sebmarkbage · sebmarkbage · commit 2fe2ac5352f8 · 2018-10-12T14:25:26.000-07:00
There are two scenarios. One is that we pass a component instance that is
already in strict mode or the node that we find is in strict mode if
an outer component renders into strict mode.

I use a separate method findHostInstanceWithWarning for this so that
a) I can pass the method name (findDOMNode/findNodeHandle).
b) Can ignore this warning in React Native mixins/NativeComponent that use this helper.

I don't want to expose the fiber to the renderers themselves.
diff --git a/packages/react-dom/src/__tests__/findDOMNode-test.js b/packages/react-dom/src/__tests__/findDOMNode-test.js
@@ -12,6 +12,7 @@
 const React = require('react');
 const ReactDOM = require('react-dom');
 const ReactTestUtils = require('react-dom/test-utils');
+const StrictMode = React.StrictMode;
 
 describe('findDOMNode', () => {
   it('findDOMNode should return null if passed null', () => {
@@ -94,7 +95,72 @@ describe('findDOMNode', () => {
         return <div />;
       }
     }
-
     expect(() => ReactTestUtils.renderIntoDocument(<Bar />)).not.toThrow();
   });
+
+  it('findDOMNode should warn if used to find a host component inside StrictMode', () => {
+    let parent = undefined;
+    let child = undefined;
+
+    class ContainsStrictModeChild extends React.Component {
+      render() {
+        return (
+          <StrictMode>
+            <div ref={n => (child = n)} />
+          </StrictMode>
+        );
+      }
+    }
+
+    ReactTestUtils.renderIntoDocument(
+      <ContainsStrictModeChild ref={n => (parent = n)} />,
+    );
+
+    let match;
+    expect(() => (match = ReactDOM.findDOMNode(parent))).toWarnDev([
+      'Warning: findDOMNode is deprecated in StrictMode. ' +
+        'findDOMNode was passed an instance of ContainsStrictModeChild which renders a StrictMode subtree. ' +
+        'The nearest child is in StrictMode. ' +
+        'Use an explicit ref directly on the element you want to get a handle on.' +
+        '\n' +
+        '\n    in div (at **)' +
+        '\n    in StrictMode (at **)' +
+        '\n    in ContainsStrictModeChild (at **)' +
+        '\n' +
+        '\nLearn more about using refs safely here:' +
+        '\nhttps://fb.me/react-strict-mode-find-node',
+    ]);
+    expect(match).toBe(child);
+  });
+
+  it('findDOMNode should warn if passed a component that is inside StrictMode', () => {
+    let parent = undefined;
+    let child = undefined;
+
+    class IsInStrictMode extends React.Component {
+      render() {
+        return <div ref={n => (child = n)} />;
+      }
+    }
+
+    ReactTestUtils.renderIntoDocument(
+      <StrictMode>
+        <IsInStrictMode ref={n => (parent = n)} />
+      </StrictMode>,
+    );
+
+    let match;
+    expect(() => (match = ReactDOM.findDOMNode(parent))).toWarnDev([
+      'Warning: findDOMNode is deprecated in StrictMode. ' +
+        'findDOMNode was passed an instance of IsInStrictMode which is in a StrictMode subtree. ' +
+        'Use an explicit ref directly on the element you want to get a handle on.' +
+        '\n' +
+        '\n    in IsInStrictMode (at **)' +
+        '\n    in StrictMode (at **)' +
+        '\n' +
+        '\nLearn more about using refs safely here:' +
+        '\nhttps://fb.me/react-strict-mode-find-node',
+    ]);
+    expect(match).toBe(child);
+  });
 });
diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js
@@ -614,7 +614,12 @@ const ReactDOM: Object = {
     if ((componentOrElement: any).nodeType === ELEMENT_NODE) {
       return (componentOrElement: any);
     }
-
+    if (__DEV__) {
+      return DOMRenderer.findHostInstanceWithWarning(
+        componentOrElement,
+        'findDOMNode',
+      );
+    }
     return DOMRenderer.findHostInstance(componentOrElement);
   },
 
diff --git a/packages/react-native-renderer/src/ReactFabric.js b/packages/react-native-renderer/src/ReactFabric.js
@@ -29,6 +29,8 @@ import warningWithoutStack from 'shared/warningWithoutStack';
 
 const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;
 const findHostInstance = ReactFabricRenderer.findHostInstance;
+const findHostInstanceWithWarning =
+  ReactFabricRenderer.findHostInstanceWithWarning;
 
 function findNodeHandle(componentOrHandle: any): ?number {
   if (__DEV__) {
@@ -60,7 +62,16 @@ function findNodeHandle(componentOrHandle: any): ?number {
   if (componentOrHandle.canonical && componentOrHandle.canonical._nativeTag) {
     return componentOrHandle.canonical._nativeTag;
   }
-  const hostInstance = findHostInstance(componentOrHandle);
+  let hostInstance;
+  if (__DEV__) {
+    hostInstance = findHostInstanceWithWarning(
+      componentOrHandle,
+      'findNodeHandle',
+    );
+  } else {
+    hostInstance = findHostInstance(componentOrHandle);
+  }
+
   if (hostInstance == null) {
     return hostInstance;
   }
diff --git a/packages/react-native-renderer/src/ReactNativeRenderer.js b/packages/react-native-renderer/src/ReactNativeRenderer.js
@@ -32,6 +32,8 @@ import warningWithoutStack from 'shared/warningWithoutStack';
 
 const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;
 const findHostInstance = ReactNativeFiberRenderer.findHostInstance;
+const findHostInstanceWithWarning =
+  ReactNativeFiberRenderer.findHostInstanceWithWarning;
 
 function findNodeHandle(componentOrHandle: any): ?number {
   if (__DEV__) {
@@ -63,7 +65,16 @@ function findNodeHandle(componentOrHandle: any): ?number {
   if (componentOrHandle.canonical && componentOrHandle.canonical._nativeTag) {
     return componentOrHandle.canonical._nativeTag;
   }
-  const hostInstance = findHostInstance(componentOrHandle);
+  let hostInstance;
+  if (__DEV__) {
+    hostInstance = findHostInstanceWithWarning(
+      componentOrHandle,
+      'findNodeHandle',
+    );
+  } else {
+    hostInstance = findHostInstance(componentOrHandle);
+  }
+
   if (hostInstance == null) {
     return hostInstance;
   }
diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js
@@ -522,6 +522,12 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
       if (typeof component.id === 'number') {
         return component;
       }
+      if (__DEV__) {
+        return NoopRenderer.findHostInstanceWithWarning(
+          component,
+          'findInstance',
+        );
+      }
       return NoopRenderer.findHostInstance(component);
     },
 
diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js
@@ -61,6 +61,8 @@ import {
 import {createUpdate, enqueueUpdate} from './ReactUpdateQueue';
 import ReactFiberInstrumentation from './ReactFiberInstrumentation';
 import * as ReactCurrentFiber from './ReactCurrentFiber';
+import {getStackByFiberInDevAndProd} from './ReactCurrentFiber';
+import {StrictMode} from './ReactTypeOfMode';
 
 type OpaqueRoot = FiberRoot;
 
@@ -82,9 +84,11 @@ type DevToolsConfig = {|
 |};
 
 let didWarnAboutNestedUpdates;
+let didWarnAboutFindNodeInStrictMode;
 
 if (__DEV__) {
   didWarnAboutNestedUpdates = false;
+  didWarnAboutFindNodeInStrictMode = {};
 }
 
 function getContextForSubtree(
@@ -209,6 +213,68 @@ function findHostInstance(component: Object): PublicInstance | null {
   return hostFiber.stateNode;
 }
 
+function findHostInstanceWithWarning(
+  component: Object,
+  methodName: string,
+): PublicInstance | null {
+  if (__DEV__) {
+    const fiber = ReactInstanceMap.get(component);
+    if (fiber === undefined) {
+      if (typeof component.render === 'function') {
+        invariant(false, 'Unable to find node on an unmounted component.');
+      } else {
+        invariant(
+          false,
+          'Argument appears to not be a ReactComponent. Keys: %s',
+          Object.keys(component),
+        );
+      }
+    }
+    const hostFiber = findCurrentHostFiber(fiber);
+    if (hostFiber === null) {
+      return null;
+    }
+    if (hostFiber.mode & StrictMode) {
+      const componentName = getComponentName(fiber.type) || 'Component';
+      if (!didWarnAboutFindNodeInStrictMode[componentName]) {
+        didWarnAboutFindNodeInStrictMode[componentName] = true;
+        if (fiber.mode & StrictMode) {
+          warningWithoutStack(
+            false,
+            '%s is deprecated in StrictMode. ' +
+              '%s was passed an instance of %s which is in a StrictMode subtree. ' +
+              'Use an explicit ref directly on the element you want to get a handle on.' +
+              '\n%s' +
+              '\n\nLearn more about using refs safely here:' +
+              '\nhttps://fb.me/react-strict-mode-find-node',
+            methodName,
+            methodName,
+            componentName,
+            getStackByFiberInDevAndProd(fiber),
+          );
+        } else {
+          warningWithoutStack(
+            false,
+            '%s is deprecated in StrictMode. ' +
+              '%s was passed an instance of %s which renders a StrictMode subtree. ' +
+              'The nearest child is in StrictMode. ' +
+              'Use an explicit ref directly on the element you want to get a handle on.' +
+              '\n%s' +
+              '\n\nLearn more about using refs safely here:' +
+              '\nhttps://fb.me/react-strict-mode-find-node',
+            methodName,
+            methodName,
+            componentName,
+            getStackByFiberInDevAndProd(hostFiber),
+          );
+        }
+      }
+    }
+    return hostFiber.stateNode;
+  }
+  return findHostInstance(component);
+}
+
 export function createContainer(
   containerInfo: Container,
   isConcurrent: boolean,
@@ -266,6 +332,8 @@ export function getPublicRootInstance(
 
 export {findHostInstance};
 
+export {findHostInstanceWithWarning};
+
 export function findHostInstanceWithNoPortals(
   fiber: Fiber,
 ): PublicInstance | null {
diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalReflection-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalReflection-test.internal.js
@@ -138,22 +138,35 @@ describe('ReactIncrementalReflection', () => {
 
     let classInstance = null;
 
+    function findInstance(inst) {
+      // We ignore warnings fired by findInstance because we are testing
+      // that the actual behavior still works as expected even though it
+      // is deprecated.
+      let oldConsoleError = console.error;
+      console.error = jest.fn();
+      try {
+        return ReactNoop.findInstance(inst);
+      } finally {
+        console.error = oldConsoleError;
+      }
+    }
+
     class Component extends React.Component {
       UNSAFE_componentWillMount() {
         classInstance = this;
-        ops.push('componentWillMount', ReactNoop.findInstance(this));
+        ops.push('componentWillMount', findInstance(this));
       }
       componentDidMount() {
-        ops.push('componentDidMount', ReactNoop.findInstance(this));
+        ops.push('componentDidMount', findInstance(this));
       }
       UNSAFE_componentWillUpdate() {
-        ops.push('componentWillUpdate', ReactNoop.findInstance(this));
+        ops.push('componentWillUpdate', findInstance(this));
       }
       componentDidUpdate() {
-        ops.push('componentDidUpdate', ReactNoop.findInstance(this));
+        ops.push('componentDidUpdate', findInstance(this));
       }
       componentWillUnmount() {
-        ops.push('componentWillUnmount', ReactNoop.findInstance(this));
+        ops.push('componentWillUnmount', findInstance(this));
       }
       render() {
         ops.push('render');
@@ -193,7 +206,7 @@ describe('ReactIncrementalReflection', () => {
     expect(classInstance).toBeDefined();
     // The instance has been complete but is still not committed so it should
     // not find any host nodes in it.
-    expect(ReactNoop.findInstance(classInstance)).toBe(null);
+    expect(findInstance(classInstance)).toBe(null);
 
     expect(ReactNoop.flush).toWarnDev(
       'componentWillMount: Please update the following components ' +
@@ -206,7 +219,7 @@ describe('ReactIncrementalReflection', () => {
     const hostSpan = classInstance.span;
     expect(hostSpan).toBeDefined();
 
-    expect(ReactNoop.findInstance(classInstance)).toBe(hostSpan);
+    expect(findInstance(classInstance)).toBe(hostSpan);
 
     expect(ops).toEqual(['componentDidMount', hostSpan]);
 
