Merge pull request #8949 from acdlite/fibercomponentlifecycletests

[Fiber] Component lifecycle tests

scripts/fiber/tests-failing.txt

@@ -1,9 +1,6 @@
 src/isomorphic/classic/__tests__/ReactContextValidator-test.js
 * should pass previous context to lifecycles
 
-src/renderers/__tests__/ReactComponentLifeCycle-test.js
-* should carry through each of the phases of setup
-
 src/renderers/__tests__/ReactComponentTreeHook-test.js
 * can be retrieved by ID
 

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

@@ -1,8 +1,3 @@
-src/renderers/__tests__/ReactComponentLifeCycle-test.js
-* should correctly determine if a component is mounted
-* should correctly determine if a null component is mounted
-* warns if findDOMNode is used inside render
-
 src/renderers/__tests__/ReactComponentTreeHook-test.js
 * uses displayName or Unknown for classic components
 * uses displayName, name, or ReactComponent for modern components
@@ -111,8 +106,6 @@ src/renderers/__tests__/ReactComponentTreeHook-test.native.js
 * does not report top-level wrapper as a root
 
 src/renderers/__tests__/ReactCompositeComponent-test.js
-* should warn about `forceUpdate` on unmounted components
-* should warn about `setState` on unmounted components
 * should warn about `setState` in render
 * should warn about `setState` in getChildContext
 * should disallow nested render calls

scripts/fiber/tests-passing.txt

@@ -546,7 +546,11 @@ src/renderers/__tests__/ReactComponentLifeCycle-test.js
 * throws when accessing state in componentWillMount
 * should allow update state inside of componentWillMount
 * should not allow update state inside of getInitialState
+* should correctly determine if a component is mounted
+* should correctly determine if a null component is mounted
 * isMounted should return false when unmounted
+* warns if findDOMNode is used inside render
+* should carry through each of the phases of setup
 * should not throw when updating an auxiliary component
 * should allow state updates in componentDidMount
 * should call nested lifecycle methods in the right order
@@ -568,6 +572,8 @@ src/renderers/__tests__/ReactCompositeComponent-test.js
 * should not pass this to getDefaultProps
 * should use default values for undefined props
 * should not mutate passed-in props object
+* should warn about `forceUpdate` on unmounted components
+* should warn about `setState` on unmounted components
 * should silently allow `setState`, not call cb on unmounting components
 * should cleanup even if render() fatals
 * should call componentWillUnmount before unmounting

src/renderers/__tests__/ReactComponentLifeCycle-test.js

@@ -13,7 +13,6 @@
 
 var React;
 var ReactDOM;
-var ReactInstanceMap;
 var ReactTestUtils;
 
 var clone = function(o) {
@@ -78,12 +77,9 @@ type ComponentLifeCycle =
   'UNMOUNTED';
 
 function getLifeCycleState(instance): ComponentLifeCycle {
-  var internalInstance = ReactInstanceMap.get(instance);
-  // Once a component gets mounted, it has an internal instance, once it
-  // gets unmounted, it loses that internal instance.
-  return internalInstance ?
-         'MOUNTED' :
-         'UNMOUNTED';
+  return instance.updater.isMounted(instance) ?
+    'MOUNTED' :
+    'UNMOUNTED';
 }
 
 /**
@@ -99,7 +95,6 @@ describe('ReactComponentLifeCycle', () => {
     React = require('React');
     ReactDOM = require('ReactDOM');
     ReactTestUtils = require('ReactTestUtils');
-    ReactInstanceMap = require('ReactInstanceMap');
   });
 
   it('should not reuse an instance when it has been unmounted', () => {
@@ -336,6 +331,9 @@ describe('ReactComponentLifeCycle', () => {
   });
 
   it('should carry through each of the phases of setup', () => {
+    spyOn(console, 'error');
+
+
     class LifeCycleComponent extends React.Component {
       constructor(props, context) {
         super(props, context);
@@ -410,7 +408,7 @@ describe('ReactComponentLifeCycle', () => {
       instance._testJournal.returnedFromGetInitialState
     );
     expect(instance._testJournal.lifeCycleAtStartOfWillMount)
-      .toBe('MOUNTED');
+      .toBe('UNMOUNTED');
 
     // componentDidMount
     expect(instance._testJournal.stateAtStartOfDidMount)
@@ -419,11 +417,11 @@ describe('ReactComponentLifeCycle', () => {
       'MOUNTED'
     );
 
-    // render
+    // initial render
     expect(instance._testJournal.stateInInitialRender)
       .toEqual(INIT_RENDER_STATE);
     expect(instance._testJournal.lifeCycleInInitialRender).toBe(
-      'MOUNTED'
+      'UNMOUNTED'
     );
 
     expect(getLifeCycleState(instance)).toBe('MOUNTED');
@@ -452,6 +450,11 @@ describe('ReactComponentLifeCycle', () => {
     // But the current lifecycle of the component is unmounted.
     expect(getLifeCycleState(instance)).toBe('UNMOUNTED');
     expect(instance.state).toEqual(POST_WILL_UNMOUNT_STATE);
+
+    expectDev(console.error.calls.count()).toBe(1);
+    expectDev(console.error.calls.argsFor(0)[0]).toContain(
+      'LifeCycleComponent is accessing isMounted inside its render() function'
+    );
   });
 
   it('should not throw when updating an auxiliary component', () => {

src/renderers/__tests__/ReactCompositeComponent-test.js

@@ -277,10 +277,10 @@ describe('ReactCompositeComponent', () => {
 
     instance.forceUpdate();
     expectDev(console.error.calls.count()).toBe(1);
-    expectDev(console.error.calls.argsFor(0)[0]).toBe(
-      'Warning: forceUpdate(...): Can only update a mounted or ' +
-      'mounting component. This usually means you called forceUpdate() on an ' +
-      'unmounted component. This is a no-op.\n\nPlease check the code for the ' +
+    expectDev(console.error.calls.argsFor(0)[0]).toContain(
+      'Can only update a mounted or mounting component. This usually means ' +
+      'you called setState, replaceState, or forceUpdate on an unmounted ' +
+      'component. This is a no-op.\n\nPlease check the code for the ' +
       'Component component.'
     );
   });
@@ -321,10 +321,10 @@ describe('ReactCompositeComponent', () => {
     expect(renders).toBe(2);
 
     expectDev(console.error.calls.count()).toBe(1);
-    expectDev(console.error.calls.argsFor(0)[0]).toBe(
-      'Warning: setState(...): Can only update a mounted or ' +
-      'mounting component. This usually means you called setState() on an ' +
-      'unmounted component. This is a no-op.\n\nPlease check the code for the ' +
+    expectDev(console.error.calls.argsFor(0)[0]).toContain(
+      'Can only update a mounted or mounting component. This usually means ' +
+      'you called setState, replaceState, or forceUpdate on an unmounted ' +
+      'component. This is a no-op.\n\nPlease check the code for the ' +
       'Component component.'
     );
   });
@@ -384,12 +384,11 @@ describe('ReactCompositeComponent', () => {
     var instance = ReactDOM.render(<Component />, container);
 
     expectDev(console.error.calls.count()).toBe(1);
-    expectDev(console.error.calls.argsFor(0)[0]).toBe(
-      'Warning: setState(...): Cannot update during an existing state ' +
-      'transition (such as within `render` or another component\'s ' +
-      'constructor). Render methods should be a pure function of props and ' +
-      'state; constructor side-effects are an anti-pattern, but can be moved ' +
-      'to `componentWillMount`.'
+    expectDev(console.error.calls.argsFor(0)[0]).toContain(
+      'Cannot update during an existing state transition (such as within ' +
+      '`render` or another component\'s constructor). Render methods should ' +
+      'be a pure function of props and state; constructor side-effects are ' +
+      'an anti-pattern, but can be moved to `componentWillMount`.'
     );
 
     // The setState call is queued and then executed as a second pass. This

src/renderers/dom/shared/findDOMNode.js

@@ -26,18 +26,26 @@ let findStack = function(arg) {
 
 const findDOMNode = function(componentOrElement : Element | ?ReactComponent<any, any, any>) : null | Element | Text {
   if (__DEV__) {
-    var owner = ReactCurrentOwner.current;
-    if (owner !== null && '_warnedAboutRefsInRender' in owner) {
+    var owner = (ReactCurrentOwner.current : any);
+    if (owner !== null) {
+      var isFiber = typeof owner.tag === 'number';
+      var warnedAboutRefsInRender = isFiber ?
+        owner.stateNode._warnedAboutRefsInRender :
+        owner._warnedAboutRefsInRender;
       warning(
-        (owner: any)._warnedAboutRefsInRender,
+        warnedAboutRefsInRender,
         '%s is accessing findDOMNode inside its render(). ' +
         'render() should be a pure function of props and state. It should ' +
         'never access something that requires stale data from the previous ' +
         'render, such as refs. Move this logic to componentDidMount and ' +
         'componentDidUpdate instead.',
         getComponentName(owner) || 'A component'
       );
-      (owner: any)._warnedAboutRefsInRender = true;
+      if (isFiber) {
+        owner.stateNode._warnedAboutRefsInRender = true;
+      } else {
+        owner._warnedAboutRefsInRender = true;
+      }
     }
   }
   if (componentOrElement == null) {

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -86,8 +86,21 @@ var {
 var invariant = require('invariant');
 
 if (__DEV__) {
+  var warning = require('warning');
   var ReactFiberInstrumentation = require('ReactFiberInstrumentation');
   var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');
+
+  var warnAboutUpdateOnUnmounted = function(instance : ReactClass<any>) {
+    const ctor = instance.constructor;
+    warning(
+      false,
+      'Can only update a mounted or mounting component. This usually means ' +
+      'you called setState, replaceState, or forceUpdate on an unmounted ' +
+      'component. This is a no-op.\n\nPlease check the code for the ' +
+      '%s component.',
+      ctor && (ctor.displayName || ctor.name) || 'ReactClass'
+    );
+  };
 }
 
 var timeHeuristicForUnitOfWork = 1;
@@ -349,6 +362,9 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(config : HostConfig<T, P,
       'in React. Please file an issue.'
     );
 
+    // Reset this to null before calling lifecycles
+    ReactCurrentOwner.current = null;
+
     // Updates that occur during the commit phase should have Task priority
     const previousPriorityContext = priorityContext;
     priorityContext = TaskPriority;
@@ -1129,7 +1145,11 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(config : HostConfig<T, P,
               return;
           }
         } else {
-          // TODO: Warn about setting state on an unmounted component.
+          if (__DEV__) {
+            if (fiber.tag === ClassComponent) {
+              warnAboutUpdateOnUnmounted(fiber.stateNode);
+            }
+          }
           return;
         }
       }

src/renderers/shared/fiber/ReactFiberTreeReflection.js

@@ -15,13 +15,19 @@
 import type { Fiber } from 'ReactFiber';
 
 var ReactInstanceMap = require('ReactInstanceMap');
+var ReactCurrentOwner = require('ReactCurrentOwner');
 
 var invariant = require('invariant');
 
+if (__DEV__) {
+  var warning = require('warning');
+}
+
 var {
   HostRoot,
   HostComponent,
   HostText,
+  ClassComponent,
 } = require('ReactTypeOfWork');
 
 var {
@@ -66,6 +72,24 @@ exports.isFiberMounted = function(fiber : Fiber) : boolean {
 };
 
 exports.isMounted = function(component : ReactComponent<any, any, any>) : boolean {
+  if (__DEV__) {
+    const owner = (ReactCurrentOwner.current : any);
+    if (owner !== null && owner.tag === ClassComponent) {
+      const ownerFiber : Fiber = owner;
+      const instance = ownerFiber.stateNode;
+      warning(
+        instance._warnedAboutRefsInRender,
+        '%s is accessing isMounted inside its render() function. ' +
+        'render() should be a pure function of props and state. It should ' +
+        'never access something that requires stale data from the previous ' +
+        'render, such as refs. Move this logic to componentDidMount and ' +
+        'componentDidUpdate instead.',
+        getComponentName(ownerFiber)
+      );
+      instance._warnedAboutRefsInRender = true;
+    }
+  }
+
   var fiber : ?Fiber = ReactInstanceMap.get(component);
   if (!fiber) {
     return false;
@@ -243,7 +267,7 @@ exports.findCurrentHostFiber = function(parent : Fiber) : Fiber | null {
   return null;
 };
 
-exports.getComponentName = function(fiber: Fiber): string {
+function getComponentName(fiber: Fiber): string {
   const type = fiber.type;
   const instance = fiber.stateNode;
   const constructor = instance && instance.constructor;
@@ -252,4 +276,5 @@ exports.getComponentName = function(fiber: Fiber): string {
     type.name || (constructor && constructor.name) ||
     'A Component'
   );
-};
+}
+exports.getComponentName = getComponentName;

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

@@ -38,16 +38,12 @@ function getInternalInstanceReadyForUpdate(publicInstance, callerName) {
   if (!internalInstance) {
     if (__DEV__) {
       var ctor = publicInstance.constructor;
-      // Only warn when we have a callerName. Otherwise we should be silent.
-      // We're probably calling from enqueueCallback. We don't want to warn
-      // there because we already warned for the corresponding lifecycle method.
       warning(
-        !callerName,
-        '%s(...): Can only update a mounted or mounting component. ' +
-        'This usually means you called %s() on an unmounted component. ' +
-        'This is a no-op.\n\nPlease check the code for the %s component.',
-        callerName,
-        callerName,
+        false,
+        'Can only update a mounted or mounting component. This usually means ' +
+        'you called setState, replaceState, or forceUpdate on an unmounted ' +
+        'component. This is a no-op.\n\nPlease check the code for the ' +
+        '%s component.',
         ctor && (ctor.displayName || ctor.name) || 'ReactClass'
       );
     }
@@ -57,12 +53,10 @@ function getInternalInstanceReadyForUpdate(publicInstance, callerName) {
   if (__DEV__) {
     warning(
       ReactCurrentOwner.current == null,
-      '%s(...): Cannot update during an existing state transition (such as ' +
-      'within `render` or another component\'s constructor). Render methods ' +
-      'should be a pure function of props and state; constructor ' +
-      'side-effects are an anti-pattern, but can be moved to ' +
-      '`componentWillMount`.',
-      callerName
+      'Cannot update during an existing state transition (such as within ' +
+      '`render` or another component\'s constructor). Render methods should ' +
+      'be a pure function of props and state; constructor side-effects are ' +
+      'an anti-pattern, but can be moved to `componentWillMount`.',
     );
   }
 
@@ -134,10 +128,7 @@ var ReactUpdateQueue = {
    * @internal
    */
   enqueueForceUpdate: function(publicInstance, callback, callerName) {
-    var internalInstance = getInternalInstanceReadyForUpdate(
-      publicInstance,
-      'forceUpdate'
-    );
+    var internalInstance = getInternalInstanceReadyForUpdate(publicInstance);
 
     if (!internalInstance) {
       return;
@@ -174,10 +165,7 @@ var ReactUpdateQueue = {
    * @internal
    */
   enqueueReplaceState: function(publicInstance, completeState, callback, callerName) {
-    var internalInstance = getInternalInstanceReadyForUpdate(
-      publicInstance,
-      'replaceState'
-    );
+    var internalInstance = getInternalInstanceReadyForUpdate(publicInstance);
 
     if (!internalInstance) {
       return;
@@ -223,10 +211,7 @@ var ReactUpdateQueue = {
       );
     }
 
-    var internalInstance = getInternalInstanceReadyForUpdate(
-      publicInstance,
-      'setState'
-    );
+    var internalInstance = getInternalInstanceReadyForUpdate(publicInstance);
 
     if (!internalInstance) {
       return;