diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 67febc8fb363..8401824c78f8 100644 --- a/scripts/fiber/tests-failing.txt +++ b/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 diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt index 53804699beee..a935f970dd82 100644 --- a/scripts/fiber/tests-passing-except-dev.txt +++ b/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 @@ -62,8 +57,6 @@ src/renderers/__tests__/ReactComponentTreeHook-test.js * works 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 diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 0dadd33329ea..802edeac09c3 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -547,7 +547,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 @@ -618,6 +622,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 diff --git a/src/renderers/__tests__/ReactComponentLifeCycle-test.js b/src/renderers/__tests__/ReactComponentLifeCycle-test.js index 8497d33a7069..7c6c06cf7597 100644 --- a/src/renderers/__tests__/ReactComponentLifeCycle-test.js +++ b/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', () => { diff --git a/src/renderers/__tests__/ReactCompositeComponent-test.js b/src/renderers/__tests__/ReactCompositeComponent-test.js index 48250101c030..ff58a2465ccb 100644 --- a/src/renderers/__tests__/ReactCompositeComponent-test.js +++ b/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(, 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 diff --git a/src/renderers/dom/shared/findDOMNode.js b/src/renderers/dom/shared/findDOMNode.js index f932f0898740..0986c7290ede 100644 --- a/src/renderers/dom/shared/findDOMNode.js +++ b/src/renderers/dom/shared/findDOMNode.js @@ -26,10 +26,14 @@ let findStack = function(arg) { const findDOMNode = function(componentOrElement : Element | ?ReactComponent) : 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 ' + @@ -37,7 +41,11 @@ const findDOMNode = function(componentOrElement : Element | ?ReactComponent) { + 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(config : HostConfig(config : HostConfig) : 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; diff --git a/src/renderers/shared/stack/reconciler/ReactUpdateQueue.js b/src/renderers/shared/stack/reconciler/ReactUpdateQueue.js index a9bed9832c2b..84fb2d517d88 100644 --- a/src/renderers/shared/stack/reconciler/ReactUpdateQueue.js +++ b/src/renderers/shared/stack/reconciler/ReactUpdateQueue.js @@ -28,16 +28,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' ); } @@ -47,12 +43,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`.', ); } @@ -124,10 +118,7 @@ var ReactUpdateQueue = { * @internal */ enqueueForceUpdate: function(publicInstance, callback, callerName) { - var internalInstance = getInternalInstanceReadyForUpdate( - publicInstance, - 'forceUpdate' - ); + var internalInstance = getInternalInstanceReadyForUpdate(publicInstance); if (!internalInstance) { return; @@ -161,10 +152,7 @@ var ReactUpdateQueue = { * @internal */ enqueueReplaceState: function(publicInstance, completeState, callback, callerName) { - var internalInstance = getInternalInstanceReadyForUpdate( - publicInstance, - 'replaceState' - ); + var internalInstance = getInternalInstanceReadyForUpdate(publicInstance); if (!internalInstance) { return; @@ -207,10 +195,7 @@ var ReactUpdateQueue = { ); } - var internalInstance = getInternalInstanceReadyForUpdate( - publicInstance, - 'setState' - ); + var internalInstance = getInternalInstanceReadyForUpdate(publicInstance); if (!internalInstance) { return;