From f68a77b3ed6c65277041aa1b74d100989d1961d4 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 19 Mar 2018 14:14:50 -0700 Subject: [PATCH 01/11] Initial pass at getSnapshotBeforeUpdate. More to do. --- .../__tests__/ReactComponentLifeCycle-test.js | 80 +++++++++++++++++++ .../ReactErrorBoundaries-test.internal.js | 40 ++++++++++ .../src/ReactDebugFiberPerf.js | 3 +- packages/react-reconciler/src/ReactFiber.js | 5 ++ .../src/ReactFiberClassComponent.js | 22 ++++- .../src/ReactFiberCommitWork.js | 52 +++++++++++- .../src/ReactFiberScheduler.js | 53 ++++++++++++ .../__tests__/ReactContextValidator-test.js | 37 --------- packages/shared/ReactTypeOfSideEffect.js | 29 +++---- 9 files changed, 266 insertions(+), 55 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js index ef0ee8ac13bfc..f36bc88e3fa2f 100644 --- a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js +++ b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js @@ -591,6 +591,7 @@ describe('ReactComponentLifeCycle', () => { } componentDidMount = logger('outer componentDidMount'); shouldComponentUpdate = logger('outer shouldComponentUpdate'); + getSnapshotBeforeUpdate = logger('outer getSnapshotBeforeUpdate'); componentDidUpdate = logger('outer componentDidUpdate'); componentWillUnmount = logger('outer componentWillUnmount'); render() { @@ -610,6 +611,7 @@ describe('ReactComponentLifeCycle', () => { } componentDidMount = logger('inner componentDidMount'); shouldComponentUpdate = logger('inner shouldComponentUpdate'); + getSnapshotBeforeUpdate = logger('inner getSnapshotBeforeUpdate'); componentDidUpdate = logger('inner componentDidUpdate'); componentWillUnmount = logger('inner componentWillUnmount'); render() { @@ -635,6 +637,8 @@ describe('ReactComponentLifeCycle', () => { 'outer shouldComponentUpdate', 'inner getDerivedStateFromProps', 'inner shouldComponentUpdate', + 'inner getSnapshotBeforeUpdate', + 'outer getSnapshotBeforeUpdate', 'inner componentDidUpdate', 'outer componentDidUpdate', ]); @@ -875,4 +879,80 @@ describe('ReactComponentLifeCycle', () => { ReactTestUtils.Simulate.click(divRef.current); expect(divRef.current.textContent).toBe('remote:2, local:2'); }); + + it('should pass the return value from getSnapshotBeforeUpdate to componentDidUpdate', () => { + const log = []; + + class MyComponent extends React.Component { + state = { + value: 0, + }; + static getDerivedStateFromProps(nextProps, prevState) { + return { + value: prevState.value + 1, + }; + } + getSnapshotBeforeUpdate(prevProps, prevState) { + log.push( + `getSnapshotBeforeUpdate() prevProps:${prevProps.value} prevState:${ + prevState.value + }`, + ); + return 'abc'; + } + componentDidUpdate(prevProps, prevState, snapshot) { + log.push( + `componentDidUpdate() prevProps:${prevProps.value} prevState:${ + prevState.value + } snapshot:${snapshot}`, + ); + } + render() { + log.push('render'); + return null; + } + } + + const div = document.createElement('div'); + ReactDOM.render( +
+ +
, + div, + ); + expect(log).toEqual(['render']); + log.length = 0; + + ReactDOM.render( +
+ +
, + div, + ); + expect(log).toEqual([ + 'render', + 'getSnapshotBeforeUpdate() prevProps:foo prevState:1', + 'componentDidUpdate() prevProps:foo prevState:1 snapshot:abc', + ]); + log.length = 0; + + ReactDOM.render( +
+ +
, + div, + ); + expect(log).toEqual([ + 'render', + 'getSnapshotBeforeUpdate() prevProps:bar prevState:2', + 'componentDidUpdate() prevProps:bar prevState:2 snapshot:abc', + ]); + log.length = 0; + + ReactDOM.render(
, div); + expect(log).toEqual([]); + + // De-duped + ReactDOM.render(, div); + }); }); diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index bec420aeabc97..e8186a79977f8 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -2081,4 +2081,44 @@ describe('ReactErrorBoundaries', () => { }); }).toThrow('foo error'); }); + + it('handles errors that occur in before-mutation commit hook', () => { + const errors = []; + let caughtError; + class Parent extends React.Component { + getSnapshotBeforeUpdate() { + errors.push('parent sad'); + throw new Error('parent sad'); + } + componentDidUpdate() {} + render() { + return ; + } + } + class Child extends React.Component { + getSnapshotBeforeUpdate() { + errors.push('child sad'); + throw new Error('child sad'); + } + componentDidUpdate() {} + render() { + return
; + } + } + + const container = document.createElement('div'); + ReactDOM.render(, container); + try { + ReactDOM.render(, container); + } catch (e) { + if (e.message !== 'parent sad' && e.message !== 'child sad') { + throw e; + } + caughtError = e; + } + + expect(errors).toEqual(['child sad', 'parent sad']); + // Error should be the first thrown + expect(caughtError.message).toBe('child sad'); + }); }); diff --git a/packages/react-reconciler/src/ReactDebugFiberPerf.js b/packages/react-reconciler/src/ReactDebugFiberPerf.js index a23446f3cc48e..da7cae5e26826 100644 --- a/packages/react-reconciler/src/ReactDebugFiberPerf.js +++ b/packages/react-reconciler/src/ReactDebugFiberPerf.js @@ -31,7 +31,8 @@ type MeasurementPhase = | 'componentWillUpdate' | 'componentDidUpdate' | 'componentDidMount' - | 'getChildContext'; + | 'getChildContext' + | 'getSnapshotBeforeUpdate'; // Prefix measurements so that it's possible to filter them. // Longer prefixes are hard to read in DevTools. diff --git a/packages/react-reconciler/src/ReactFiber.js b/packages/react-reconciler/src/ReactFiber.js index 6c88c0aa18b02..5b6524972e569 100644 --- a/packages/react-reconciler/src/ReactFiber.js +++ b/packages/react-reconciler/src/ReactFiber.js @@ -150,6 +150,9 @@ export type Fiber = {| // memory if we need to. alternate: Fiber | null, + // Stores getSnapshotBeforeUpdate return value to be passed to componentDidUpdate + snapshot: any | null, + // Conceptual aliases // workInProgress : Fiber -> alternate The alternate used for reuse happens // to be the same as work in progress. @@ -204,6 +207,8 @@ function FiberNode( this.alternate = null; + this.snapshot = null; + if (__DEV__) { this._debugID = debugCounter++; this._debugSource = null; diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 092ff7b149c69..4e83705fba7f5 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -12,7 +12,7 @@ import type {ExpirationTime} from './ReactFiberExpirationTime'; import type {LegacyContext} from './ReactFiberContext'; import type {CapturedValue} from './ReactCapturedValue'; -import {Update} from 'shared/ReactTypeOfSideEffect'; +import {Update, Snapshot} from 'shared/ReactTypeOfSideEffect'; import { enableGetDerivedStateFromCatch, debugRenderPhaseSideEffects, @@ -873,6 +873,7 @@ export default function( // TODO: Previous state can be null. let newState; let derivedStateFromCatch; + if (workInProgress.updateQueue !== null) { newState = processUpdateQueue( current, @@ -954,6 +955,14 @@ export default function( workInProgress.effectTag |= Update; } } + if (typeof instance.getSnapshotBeforeUpdate === 'function') { + if ( + oldProps !== current.memoizedProps || + oldState !== current.memoizedState + ) { + workInProgress.effectTag |= Snapshot; + } + } return false; } @@ -986,6 +995,9 @@ export default function( if (typeof instance.componentDidUpdate === 'function') { workInProgress.effectTag |= Update; } + if (typeof instance.getSnapshotBeforeUpdate === 'function') { + workInProgress.effectTag |= Snapshot; + } } else { // If an update was already in progress, we should schedule an Update // effect even though we're bailing out, so that cWU/cDU are called. @@ -997,6 +1009,14 @@ export default function( workInProgress.effectTag |= Update; } } + if (typeof instance.getSnapshotBeforeUpdate === 'function') { + if ( + oldProps !== current.memoizedProps || + oldState !== current.memoizedState + ) { + workInProgress.effectTag |= Snapshot; + } + } // If shouldComponentUpdate returned false, we should still update the // memoized props/state to indicate that this work can be reused. diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index d120c10956ec2..106867a60b482 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -27,7 +27,12 @@ import { CallComponent, } from 'shared/ReactTypeOfWork'; import ReactErrorUtils from 'shared/ReactErrorUtils'; -import {Placement, Update, ContentReset} from 'shared/ReactTypeOfSideEffect'; +import { + Placement, + Update, + ContentReset, + Snapshot, +} from 'shared/ReactTypeOfSideEffect'; import invariant from 'fbjs/lib/invariant'; import warning from 'fbjs/lib/warning'; @@ -153,6 +158,47 @@ export default function( } } + function commitBeforeMutationLifeCycles( + current: Fiber | null, + finishedWork: Fiber, + ): void { + switch (finishedWork.tag) { + case ClassComponent: { + const instance = finishedWork.stateNode; + if (finishedWork.effectTag & Snapshot) { + if (current !== null) { + const prevProps = current.memoizedProps; + const prevState = current.memoizedState; + startPhaseTimer(finishedWork, 'getSnapshotBeforeUpdate'); + instance.props = finishedWork.memoizedProps; + instance.state = finishedWork.memoizedState; + const snapshot = instance.getSnapshotBeforeUpdate( + prevProps, + prevState, + ); + // TODO Warn about undefined return value + current.snapshot = snapshot != null ? snapshot : null; + stopPhaseTimer(); + } + } + return; + } + case HostRoot: + case HostComponent: + case HostText: + case HostPortal: + // Nothing to do for these component types + return; + default: { + invariant( + false, + 'This unit of work tag should not have side-effects. This error is ' + + 'likely caused by a bug in React. Please file an issue.', + ); + } + } + } + function commitLifeCycles( finishedRoot: FiberRoot, current: Fiber | null, @@ -176,7 +222,7 @@ export default function( startPhaseTimer(finishedWork, 'componentDidUpdate'); instance.props = finishedWork.memoizedProps; instance.state = finishedWork.memoizedState; - instance.componentDidUpdate(prevProps, prevState); + instance.componentDidUpdate(prevProps, prevState, current.snapshot); stopPhaseTimer(); } } @@ -494,6 +540,7 @@ export default function( commitContainer(finishedWork); }, commitLifeCycles, + commitBeforeMutationLifeCycles, commitErrorLogging, commitAttachRef, commitDetachRef, @@ -816,6 +863,7 @@ export default function( if (enableMutatingReconciler) { return { + commitBeforeMutationLifeCycles, commitResetTextContent, commitPlacement, commitDeletion, diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index ea5df6dc84128..e1750dd2d5064 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -21,6 +21,7 @@ import { PerformedWork, Placement, Update, + Snapshot, PlacementAndUpdate, Deletion, ContentReset, @@ -197,6 +198,7 @@ export default function( isAlreadyFailedLegacyErrorBoundary, ); const { + commitBeforeMutationLifeCycles, commitResetTextContent, commitPlacement, commitDeletion, @@ -376,6 +378,22 @@ export default function( } } + function commitBeforeMutationLifecycles() { + while (nextEffect !== null) { + const effectTag = nextEffect.effectTag; + + if (effectTag & Snapshot) { + recordEffect(); + const current = nextEffect.alternate; + commitBeforeMutationLifeCycles(current, nextEffect); + } + + // Don't cleanup effects yet; + // This will be done by commitAllLifeCycles() + nextEffect = nextEffect.nextEffect; + } + } + function commitAllLifeCycles( finishedRoot: FiberRoot, currentTime: ExpirationTime, @@ -483,6 +501,41 @@ export default function( prepareForCommit(root.containerInfo); + // Invoke instances of getSnapshotBeforeUpdate before mutation. + nextEffect = firstEffect; + // TODO Start new commit phase timer from ReactDebugFiberPerf + while (nextEffect !== null) { + let didError = false; + let error; + if (__DEV__) { + invokeGuardedCallback(null, commitBeforeMutationLifecycles, null); + if (hasCaughtError()) { + didError = true; + error = clearCaughtError(); + } + } else { + try { + commitBeforeMutationLifecycles(); + } catch (e) { + didError = true; + error = e; + } + } + if (didError) { + invariant( + nextEffect !== null, + 'Should have next effect. This error is likely caused by a bug ' + + 'in React. Please file an issue.', + ); + onCommitPhaseError(nextEffect, error); + // Clean-up + if (nextEffect !== null) { + nextEffect = nextEffect.nextEffect; + } + } + } + // TODO Stop new commit phase timer from ReactDebugFiberPerf + // Commit all the side-effects within a tree. We'll do this in two passes. // The first pass performs all the host insertions, updates, deletions and // ref unmounts. diff --git a/packages/react/src/__tests__/ReactContextValidator-test.js b/packages/react/src/__tests__/ReactContextValidator-test.js index f762b6b21b6b3..d3b9e5b240b24 100644 --- a/packages/react/src/__tests__/ReactContextValidator-test.js +++ b/packages/react/src/__tests__/ReactContextValidator-test.js @@ -119,43 +119,6 @@ describe('ReactContextValidator', () => { expect(actualComponentWillUpdate).toEqual({foo: 'def'}); }); - it('should not pass previous context to lifecycles', () => { - let actualComponentDidUpdate; - - class Parent extends React.Component { - getChildContext() { - return { - foo: this.props.foo, - }; - } - - render() { - return ; - } - } - Parent.childContextTypes = { - foo: PropTypes.string.isRequired, - }; - - class Component extends React.Component { - componentDidUpdate(...args) { - actualComponentDidUpdate = args; - } - - render() { - return
; - } - } - Component.contextTypes = { - foo: PropTypes.string, - }; - - const container = document.createElement('div'); - ReactDOM.render(, container); - ReactDOM.render(, container); - expect(actualComponentDidUpdate).toHaveLength(2); - }); - it('should check context types', () => { class Component extends React.Component { render() { diff --git a/packages/shared/ReactTypeOfSideEffect.js b/packages/shared/ReactTypeOfSideEffect.js index 9c59502f596a6..750b392f1b5ee 100644 --- a/packages/shared/ReactTypeOfSideEffect.js +++ b/packages/shared/ReactTypeOfSideEffect.js @@ -10,22 +10,23 @@ export type TypeOfSideEffect = number; // Don't change these two values. They're used by React Dev Tools. -export const NoEffect = /* */ 0b00000000000; -export const PerformedWork = /* */ 0b00000000001; +export const NoEffect = /* */ 0b000000000000; +export const PerformedWork = /* */ 0b000000000001; // You can change the rest (and add more). -export const Placement = /* */ 0b00000000010; -export const Update = /* */ 0b00000000100; -export const PlacementAndUpdate = /* */ 0b00000000110; -export const Deletion = /* */ 0b00000001000; -export const ContentReset = /* */ 0b00000010000; -export const Callback = /* */ 0b00000100000; -export const DidCapture = /* */ 0b00001000000; -export const Ref = /* */ 0b00010000000; -export const ErrLog = /* */ 0b00100000000; +export const Placement = /* */ 0b000000000010; +export const Update = /* */ 0b000000000100; +export const PlacementAndUpdate = /* */ 0b000000000110; +export const Deletion = /* */ 0b000000001000; +export const ContentReset = /* */ 0b000000010000; +export const Callback = /* */ 0b000000100000; +export const DidCapture = /* */ 0b000001000000; +export const Ref = /* */ 0b000010000000; +export const ErrLog = /* */ 0b000100000000; +export const Snapshot = /* */ 0b100000000000; // Union of all host effects -export const HostEffectMask = /* */ 0b00111111111; +export const HostEffectMask = /* */ 0b000111111111; -export const Incomplete = /* */ 0b01000000000; -export const ShouldCapture = /* */ 0b10000000000; +export const Incomplete = /* */ 0b001000000000; +export const ShouldCapture = /* */ 0b010000000000; From 0f984ac4c64003975de1e0931c97a4263556583a Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 20 Mar 2018 10:59:19 -0700 Subject: [PATCH 02/11] Moved snapshot value from Fiber to instance (__reactInternalSnapshotBeforeUpdate) --- packages/react-reconciler/src/ReactFiber.js | 5 ----- packages/react-reconciler/src/ReactFiberCommitWork.js | 10 +++++++--- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiber.js b/packages/react-reconciler/src/ReactFiber.js index 5b6524972e569..6c88c0aa18b02 100644 --- a/packages/react-reconciler/src/ReactFiber.js +++ b/packages/react-reconciler/src/ReactFiber.js @@ -150,9 +150,6 @@ export type Fiber = {| // memory if we need to. alternate: Fiber | null, - // Stores getSnapshotBeforeUpdate return value to be passed to componentDidUpdate - snapshot: any | null, - // Conceptual aliases // workInProgress : Fiber -> alternate The alternate used for reuse happens // to be the same as work in progress. @@ -207,8 +204,6 @@ function FiberNode( this.alternate = null; - this.snapshot = null; - if (__DEV__) { this._debugID = debugCounter++; this._debugSource = null; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 106867a60b482..f261f52e34750 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -164,12 +164,12 @@ export default function( ): void { switch (finishedWork.tag) { case ClassComponent: { - const instance = finishedWork.stateNode; if (finishedWork.effectTag & Snapshot) { if (current !== null) { const prevProps = current.memoizedProps; const prevState = current.memoizedState; startPhaseTimer(finishedWork, 'getSnapshotBeforeUpdate'); + const instance = finishedWork.stateNode; instance.props = finishedWork.memoizedProps; instance.state = finishedWork.memoizedState; const snapshot = instance.getSnapshotBeforeUpdate( @@ -177,7 +177,7 @@ export default function( prevState, ); // TODO Warn about undefined return value - current.snapshot = snapshot != null ? snapshot : null; + instance.__reactInternalSnapshotBeforeUpdate = snapshot; stopPhaseTimer(); } } @@ -222,7 +222,11 @@ export default function( startPhaseTimer(finishedWork, 'componentDidUpdate'); instance.props = finishedWork.memoizedProps; instance.state = finishedWork.memoizedState; - instance.componentDidUpdate(prevProps, prevState, current.snapshot); + instance.componentDidUpdate( + prevProps, + prevState, + instance.__reactInternalSnapshotBeforeUpdate, + ); stopPhaseTimer(); } } From db84b9a783bc819cf77d24892fd4832f5248afa7 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 20 Mar 2018 11:05:19 -0700 Subject: [PATCH 03/11] Use commitAllHostEffects() traversal for getSnapshotBeforeUpdate() --- .../src/ReactFiberScheduler.js | 57 ++----------------- 1 file changed, 6 insertions(+), 51 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index e1750dd2d5064..ec4c554182eab 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -321,6 +321,12 @@ export default function( recordEffect(); const effectTag = nextEffect.effectTag; + + if (effectTag & Snapshot) { + const current = nextEffect.alternate; + commitBeforeMutationLifeCycles(current, nextEffect); + } + if (effectTag & ContentReset) { commitResetTextContent(nextEffect); } @@ -378,22 +384,6 @@ export default function( } } - function commitBeforeMutationLifecycles() { - while (nextEffect !== null) { - const effectTag = nextEffect.effectTag; - - if (effectTag & Snapshot) { - recordEffect(); - const current = nextEffect.alternate; - commitBeforeMutationLifeCycles(current, nextEffect); - } - - // Don't cleanup effects yet; - // This will be done by commitAllLifeCycles() - nextEffect = nextEffect.nextEffect; - } - } - function commitAllLifeCycles( finishedRoot: FiberRoot, currentTime: ExpirationTime, @@ -501,41 +491,6 @@ export default function( prepareForCommit(root.containerInfo); - // Invoke instances of getSnapshotBeforeUpdate before mutation. - nextEffect = firstEffect; - // TODO Start new commit phase timer from ReactDebugFiberPerf - while (nextEffect !== null) { - let didError = false; - let error; - if (__DEV__) { - invokeGuardedCallback(null, commitBeforeMutationLifecycles, null); - if (hasCaughtError()) { - didError = true; - error = clearCaughtError(); - } - } else { - try { - commitBeforeMutationLifecycles(); - } catch (e) { - didError = true; - error = e; - } - } - if (didError) { - invariant( - nextEffect !== null, - 'Should have next effect. This error is likely caused by a bug ' + - 'in React. Please file an issue.', - ); - onCommitPhaseError(nextEffect, error); - // Clean-up - if (nextEffect !== null) { - nextEffect = nextEffect.nextEffect; - } - } - } - // TODO Stop new commit phase timer from ReactDebugFiberPerf - // Commit all the side-effects within a tree. We'll do this in two passes. // The first pass performs all the host insertions, updates, deletions and // ref unmounts. From 70225680787a20ed8da7c1ed59fea53bc291bcc4 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 20 Mar 2018 11:31:12 -0700 Subject: [PATCH 04/11] Added DEV warnings and tests for new lifecycle --- .../__tests__/ReactComponentLifeCycle-test.js | 40 +++++++++++++++++++ .../src/ReactFiberClassComponent.js | 16 ++++++++ .../src/ReactFiberCommitWork.js | 23 ++++++++++- 3 files changed, 78 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js index f36bc88e3fa2f..ab71128e530b4 100644 --- a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js +++ b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js @@ -955,4 +955,44 @@ describe('ReactComponentLifeCycle', () => { // De-duped ReactDOM.render(, div); }); + + it('should warn if getSnapshotBeforeUpdate returns undefined', () => { + class MyComponent extends React.Component { + getSnapshotBeforeUpdate() {} + componentDidUpdate() {} + render() { + return null; + } + } + + const div = document.createElement('div'); + ReactDOM.render(, div); + expect(() => ReactDOM.render(, div)).toWarnDev( + 'MyComponent.getSnapshotBeforeUpdate(): A snapshot value (or null) must ' + + 'be returned. You have returned undefined.', + ); + + // De-duped + ReactDOM.render(, div); + }); + + it('should warn if getSnapshotBeforeUpdate is defined with no componentDidUpdate', () => { + class MyComponent extends React.Component { + getSnapshotBeforeUpdate() { + return null; + } + render() { + return null; + } + } + + const div = document.createElement('div'); + expect(() => ReactDOM.render(, div)).toWarnDev( + 'MyComponent: getSnapshotBeforeUpdate() should be used with componentDidUpdate(). ' + + 'This component defines getSnapshotBeforeUpdate() only.', + ); + + // De-duped + ReactDOM.render(, div); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 4e83705fba7f5..2663dcc2f25c3 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -42,6 +42,7 @@ let didWarnAboutStateAssignmentForComponent; let didWarnAboutUndefinedDerivedState; let didWarnAboutUninitializedState; let didWarnAboutWillReceivePropsAndDerivedState; +let didWarnAboutGetSnapshotBeforeUpdateWithoutDidUpdate; let warnOnInvalidCallback; if (__DEV__) { @@ -49,6 +50,7 @@ if (__DEV__) { didWarnAboutUndefinedDerivedState = {}; didWarnAboutUninitializedState = {}; didWarnAboutWillReceivePropsAndDerivedState = {}; + didWarnAboutGetSnapshotBeforeUpdateWithoutDidUpdate = new Set(); const didWarnOnInvalidCallback = {}; @@ -364,6 +366,20 @@ export default function( name, name, ); + + if ( + typeof instance.getSnapshotBeforeUpdate === 'function' && + typeof instance.componentDidUpdate !== 'function' && + !didWarnAboutGetSnapshotBeforeUpdateWithoutDidUpdate.has(type) + ) { + didWarnAboutGetSnapshotBeforeUpdateWithoutDidUpdate.add(type); + warning( + false, + '%s: getSnapshotBeforeUpdate() should be used with componentDidUpdate(). ' + + 'This component defines getSnapshotBeforeUpdate() only.', + getComponentName(workInProgress), + ); + } } const state = instance.state; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index f261f52e34750..101f751f34d38 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -49,6 +49,11 @@ const { clearCaughtError, } = ReactErrorUtils; +let didWarnAboutUndefinedSnapshotBeforeUpdate: Set | null = null; +if (__DEV__) { + didWarnAboutUndefinedSnapshotBeforeUpdate = new Set(); +} + function logError(boundary: Fiber, errorInfo: CapturedValue) { const source = errorInfo.source; let stack = errorInfo.stack; @@ -176,7 +181,23 @@ export default function( prevProps, prevState, ); - // TODO Warn about undefined return value + if (__DEV__) { + const didWarnSet = ((didWarnAboutUndefinedSnapshotBeforeUpdate: any): Set< + mixed, + >); + if ( + snapshot === undefined && + !didWarnSet.has(finishedWork.type) + ) { + didWarnSet.add(finishedWork.type); + warning( + false, + '%s.getSnapshotBeforeUpdate(): A snapshot value (or null) ' + + 'must be returned. You have returned undefined.', + getComponentName(finishedWork), + ); + } + } instance.__reactInternalSnapshotBeforeUpdate = snapshot; stopPhaseTimer(); } From 55e75da77d0d69df493188cceeb3345b33622db7 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 22 Mar 2018 13:32:19 -0700 Subject: [PATCH 05/11] Don't invoke legacy lifecycles if getSnapshotBeforeUpdate() is defined. DEV warn about this. --- .../__tests__/ReactComponentLifeCycle-test.js | 128 ++++++++++++++++-- .../src/ReactFiberClassComponent.js | 84 +++++++----- .../src/ReactShallowRenderer.js | 68 +++++----- .../__tests__/ReactShallowRenderer-test.js | 42 ++++++ .../createReactClassIntegration-test.js | 36 ++++- 5 files changed, 276 insertions(+), 82 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js index 31846ab454988..c8264410b64ea 100644 --- a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js +++ b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js @@ -673,10 +673,38 @@ describe('ReactComponentLifeCycle', () => { const container = document.createElement('div'); expect(() => ReactDOM.render(, container)).toWarnDev( - 'Unsafe legacy lifecycles will not be called for components using the new getDerivedStateFromProps() API.', + 'Unsafe legacy lifecycles will not be called for components using new component APIs.', ); }); + it('should not invoke deprecated lifecycles (cWM/cWRP/cWU) if new getSnapshotBeforeUpdate is present', () => { + class Component extends React.Component { + state = {}; + getSnapshotBeforeUpdate() { + return null; + } + componentWillMount() { + throw Error('unexpected'); + } + componentWillReceiveProps() { + throw Error('unexpected'); + } + componentWillUpdate() { + throw Error('unexpected'); + } + componentDidUpdate() {} + render() { + return null; + } + } + + const container = document.createElement('div'); + expect(() => ReactDOM.render(, container)).toWarnDev( + 'Unsafe legacy lifecycles will not be called for components using new component APIs.', + ); + ReactDOM.render(, container); + }); + it('should not invoke new unsafe lifecycles (cWM/cWRP/cWU) if static gDSFP is present', () => { class Component extends React.Component { state = {}; @@ -698,9 +726,10 @@ describe('ReactComponentLifeCycle', () => { } const container = document.createElement('div'); - expect(() => ReactDOM.render(, container)).toWarnDev( - 'Unsafe legacy lifecycles will not be called for components using the new getDerivedStateFromProps() API.', + expect(() => ReactDOM.render(, container)).toWarnDev( + 'Unsafe legacy lifecycles will not be called for components using new component APIs.', ); + ReactDOM.render(, container); }); it('should warn about deprecated lifecycles (cWM/cWRP/cWU) if new static gDSFP is present', () => { @@ -720,7 +749,7 @@ describe('ReactComponentLifeCycle', () => { } expect(() => ReactDOM.render(, container)).toWarnDev( - 'Unsafe legacy lifecycles will not be called for components using the new getDerivedStateFromProps() API.\n\n' + + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + 'AllLegacyLifecycles uses getDerivedStateFromProps() but also contains the following legacy lifecycles:\n' + ' componentWillMount\n' + ' UNSAFE_componentWillReceiveProps\n' + @@ -741,7 +770,7 @@ describe('ReactComponentLifeCycle', () => { } expect(() => ReactDOM.render(, container)).toWarnDev( - 'Unsafe legacy lifecycles will not be called for components using the new getDerivedStateFromProps() API.\n\n' + + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + 'WillMount uses getDerivedStateFromProps() but also contains the following legacy lifecycles:\n' + ' UNSAFE_componentWillMount\n\n' + 'The above lifecycles should be removed. Learn more about this warning here:\n' + @@ -761,7 +790,7 @@ describe('ReactComponentLifeCycle', () => { } expect(() => ReactDOM.render(, container)).toWarnDev( - 'Unsafe legacy lifecycles will not be called for components using the new getDerivedStateFromProps() API.\n\n' + + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + 'WillMountAndUpdate uses getDerivedStateFromProps() but also contains the following legacy lifecycles:\n' + ' componentWillMount\n' + ' UNSAFE_componentWillUpdate\n\n' + @@ -781,7 +810,7 @@ describe('ReactComponentLifeCycle', () => { } expect(() => ReactDOM.render(, container)).toWarnDev( - 'Unsafe legacy lifecycles will not be called for components using the new getDerivedStateFromProps() API.\n\n' + + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + 'WillReceiveProps uses getDerivedStateFromProps() but also contains the following legacy lifecycles:\n' + ' componentWillReceiveProps\n\n' + 'The above lifecycles should be removed. Learn more about this warning here:\n' + @@ -789,6 +818,88 @@ describe('ReactComponentLifeCycle', () => { ); }); + it('should warn about deprecated lifecycles (cWM/cWRP/cWU) if new getSnapshotBeforeUpdate is present', () => { + const container = document.createElement('div'); + + class AllLegacyLifecycles extends React.Component { + state = {}; + getSnapshotBeforeUpdate() {} + componentWillMount() {} + UNSAFE_componentWillReceiveProps() {} + componentWillUpdate() {} + componentDidUpdate() {} + render() { + return null; + } + } + + expect(() => ReactDOM.render(, container)).toWarnDev( + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + + 'AllLegacyLifecycles uses getSnapshotBeforeUpdate() but also contains the following legacy lifecycles:\n' + + ' componentWillMount\n' + + ' UNSAFE_componentWillReceiveProps\n' + + ' componentWillUpdate\n\n' + + 'The above lifecycles should be removed. Learn more about this warning here:\n' + + 'https://fb.me/react-async-component-lifecycle-hooks', + ); + + class WillMount extends React.Component { + state = {}; + getSnapshotBeforeUpdate() {} + UNSAFE_componentWillMount() {} + componentDidUpdate() {} + render() { + return null; + } + } + + expect(() => ReactDOM.render(, container)).toWarnDev( + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + + 'WillMount uses getSnapshotBeforeUpdate() but also contains the following legacy lifecycles:\n' + + ' UNSAFE_componentWillMount\n\n' + + 'The above lifecycles should be removed. Learn more about this warning here:\n' + + 'https://fb.me/react-async-component-lifecycle-hooks', + ); + + class WillMountAndUpdate extends React.Component { + state = {}; + getSnapshotBeforeUpdate() {} + componentWillMount() {} + UNSAFE_componentWillUpdate() {} + componentDidUpdate() {} + render() { + return null; + } + } + + expect(() => ReactDOM.render(, container)).toWarnDev( + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + + 'WillMountAndUpdate uses getSnapshotBeforeUpdate() but also contains the following legacy lifecycles:\n' + + ' componentWillMount\n' + + ' UNSAFE_componentWillUpdate\n\n' + + 'The above lifecycles should be removed. Learn more about this warning here:\n' + + 'https://fb.me/react-async-component-lifecycle-hooks', + ); + + class WillReceiveProps extends React.Component { + state = {}; + getSnapshotBeforeUpdate() {} + componentWillReceiveProps() {} + componentDidUpdate() {} + render() { + return null; + } + } + + expect(() => ReactDOM.render(, container)).toWarnDev( + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + + 'WillReceiveProps uses getSnapshotBeforeUpdate() but also contains the following legacy lifecycles:\n' + + ' componentWillReceiveProps\n\n' + + 'The above lifecycles should be removed. Learn more about this warning here:\n' + + 'https://fb.me/react-async-component-lifecycle-hooks', + ); + }); + it('calls effects on module-pattern component', function() { const log = []; @@ -1037,9 +1148,6 @@ describe('ReactComponentLifeCycle', () => { ReactDOM.render(
, div); expect(log).toEqual([]); - - // De-duped - ReactDOM.render(, div); }); it('should warn if getSnapshotBeforeUpdate returns undefined', () => { diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 58f48faca9e74..271a415412af1 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -370,6 +370,7 @@ export default function( if ( typeof instance.getSnapshotBeforeUpdate === 'function' && typeof instance.componentDidUpdate !== 'function' && + typeof instance.UNSAFE_componentDidUpdate !== 'function' && !didWarnAboutGetSnapshotBeforeUpdateWithoutDidUpdate.has(type) ) { didWarnAboutGetSnapshotBeforeUpdateWithoutDidUpdate.add(type); @@ -452,24 +453,30 @@ export default function( adoptClassInstance(workInProgress, instance); if (__DEV__) { - if (typeof ctor.getDerivedStateFromProps === 'function') { - if (state === null) { - const componentName = getComponentName(workInProgress) || 'Component'; - if (!didWarnAboutUninitializedState[componentName]) { - warning( - false, - '%s: Did not properly initialize state during construction. ' + - 'Expected state to be an object, but it was %s.', - componentName, - instance.state === null ? 'null' : 'undefined', - ); - didWarnAboutUninitializedState[componentName] = true; - } + if ( + typeof ctor.getDerivedStateFromProps === 'function' && + state === null + ) { + const componentName = getComponentName(workInProgress) || 'Component'; + if (!didWarnAboutUninitializedState[componentName]) { + warning( + false, + '%s: Did not properly initialize state during construction. ' + + 'Expected state to be an object, but it was %s.', + componentName, + instance.state === null ? 'null' : 'undefined', + ); + didWarnAboutUninitializedState[componentName] = true; } + } - // If getDerivedStateFromProps() is defined, "unsafe" lifecycles won't be called. - // Warn about these lifecycles if they are present. - // Don't warn about react-lifecycles-compat polyfilled methods though. + // If new component APIs are defined, "unsafe" lifecycles won't be called. + // Warn about these lifecycles if they are present. + // Don't warn about react-lifecycles-compat polyfilled methods though. + if ( + typeof ctor.getDerivedStateFromProps === 'function' || + typeof instance.getSnapshotBeforeUpdate === 'function' + ) { let foundWillMountName = null; let foundWillReceivePropsName = null; let foundWillUpdateName = null; @@ -503,16 +510,18 @@ export default function( foundWillUpdateName !== null ) { const componentName = getComponentName(workInProgress) || 'Component'; + const newApiName = ctor.getDerivedStateFromProps + ? 'getDerivedStateFromProps()' + : 'getSnapshotBeforeUpdate()'; if (!didWarnAboutLegacyLifecyclesAndDerivedState[componentName]) { warning( false, - 'Unsafe legacy lifecycles will not be called for components using ' + - 'the new getDerivedStateFromProps() API.\n\n' + - '%s uses getDerivedStateFromProps() but also contains the following legacy lifecycles:' + - '%s%s%s\n\n' + + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + + '%s uses %s but also contains the following legacy lifecycles:%s%s%s\n\n' + 'The above lifecycles should be removed. Learn more about this warning here:\n' + 'https://fb.me/react-async-component-lifecycle-hooks', componentName, + newApiName, foundWillMountName !== null ? `\n ${foundWillMountName}` : '', foundWillReceivePropsName !== null ? `\n ${foundWillReceivePropsName}` @@ -696,11 +705,12 @@ export default function( } // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( + typeof ctor.getDerivedStateFromProps !== 'function' && + typeof instance.getSnapshotBeforeUpdate !== 'function' && (typeof instance.UNSAFE_componentWillMount === 'function' || - typeof instance.componentWillMount === 'function') && - typeof ctor.getDerivedStateFromProps !== 'function' + typeof instance.componentWillMount === 'function') ) { callComponentWillMount(workInProgress, instance); // If we had additional state updates during this life-cycle, let's @@ -741,11 +751,12 @@ export default function( // during componentDidUpdate we pass the "current" props. // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( + typeof ctor.getDerivedStateFromProps !== 'function' && + typeof instance.getSnapshotBeforeUpdate !== 'function' && (typeof instance.UNSAFE_componentWillReceiveProps === 'function' || - typeof instance.componentWillReceiveProps === 'function') && - typeof ctor.getDerivedStateFromProps !== 'function' + typeof instance.componentWillReceiveProps === 'function') ) { if (oldProps !== newProps || oldContext !== newContext) { callComponentWillReceiveProps( @@ -852,11 +863,12 @@ export default function( if (shouldUpdate) { // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( + typeof ctor.getDerivedStateFromProps !== 'function' && + typeof instance.getSnapshotBeforeUpdate !== 'function' && (typeof instance.UNSAFE_componentWillMount === 'function' || - typeof instance.componentWillMount === 'function') && - typeof ctor.getDerivedStateFromProps !== 'function' + typeof instance.componentWillMount === 'function') ) { startPhaseTimer(workInProgress, 'componentWillMount'); if (typeof instance.componentWillMount === 'function') { @@ -913,11 +925,12 @@ export default function( // during componentDidUpdate we pass the "current" props. // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( + typeof ctor.getDerivedStateFromProps !== 'function' && + typeof instance.getSnapshotBeforeUpdate !== 'function' && (typeof instance.UNSAFE_componentWillReceiveProps === 'function' || - typeof instance.componentWillReceiveProps === 'function') && - typeof ctor.getDerivedStateFromProps !== 'function' + typeof instance.componentWillReceiveProps === 'function') ) { if (oldProps !== newProps || oldContext !== newContext) { callComponentWillReceiveProps( @@ -1038,11 +1051,12 @@ export default function( if (shouldUpdate) { // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( + typeof ctor.getDerivedStateFromProps !== 'function' && + typeof instance.getSnapshotBeforeUpdate !== 'function' && (typeof instance.UNSAFE_componentWillUpdate === 'function' || - typeof instance.componentWillUpdate === 'function') && - typeof ctor.getDerivedStateFromProps !== 'function' + typeof instance.componentWillUpdate === 'function') ) { startPhaseTimer(workInProgress, 'componentWillUpdate'); if (typeof instance.componentWillUpdate === 'function') { diff --git a/packages/react-test-renderer/src/ReactShallowRenderer.js b/packages/react-test-renderer/src/ReactShallowRenderer.js index 8d80221cbbd1e..131db2d417511 100644 --- a/packages/react-test-renderer/src/ReactShallowRenderer.js +++ b/packages/react-test-renderer/src/ReactShallowRenderer.js @@ -139,20 +139,18 @@ class ReactShallowRenderer { ) { const beforeState = this._newState; - if (typeof this._instance.componentWillMount === 'function') { - // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. - if (typeof element.type.getDerivedStateFromProps !== 'function') { - this._instance.componentWillMount(); - } - } + // In order to support react-lifecycles-compat polyfilled components, + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( - typeof this._instance.UNSAFE_componentWillMount === 'function' && - typeof element.type.getDerivedStateFromProps !== 'function' + typeof element.type.getDerivedStateFromProps !== 'function' && + typeof this._instance.getSnapshotBeforeUpdate !== 'function' ) { - // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. - this._instance.UNSAFE_componentWillMount(); + if (typeof this._instance.componentWillMount === 'function') { + this._instance.componentWillMount(); + } + if (typeof this._instance.UNSAFE_componentWillMount === 'function') { + this._instance.UNSAFE_componentWillMount(); + } } // setState may have been called during cWM @@ -173,20 +171,20 @@ class ReactShallowRenderer { const oldProps = this._instance.props; if (oldProps !== props) { - if (typeof this._instance.componentWillReceiveProps === 'function') { - // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. - if (typeof element.type.getDerivedStateFromProps !== 'function') { - this._instance.componentWillReceiveProps(props, context); - } - } + // In order to support react-lifecycles-compat polyfilled components, + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( - typeof this._instance.UNSAFE_componentWillReceiveProps === 'function' && - typeof element.type.getDerivedStateFromProps !== 'function' + typeof element.type.getDerivedStateFromProps !== 'function' && + typeof this._instance.getSnapshotBeforeUpdate !== 'function' ) { - // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. - this._instance.UNSAFE_componentWillReceiveProps(props, context); + if (typeof this._instance.componentWillReceiveProps === 'function') { + this._instance.componentWillReceiveProps(props, context); + } + if ( + typeof this._instance.UNSAFE_componentWillReceiveProps === 'function' + ) { + this._instance.UNSAFE_componentWillReceiveProps(props, context); + } } this._updateStateFromStaticLifecycle(props); @@ -211,20 +209,18 @@ class ReactShallowRenderer { } if (shouldUpdate) { - if (typeof this._instance.componentWillUpdate === 'function') { - // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. - if (typeof type.getDerivedStateFromProps !== 'function') { - this._instance.componentWillUpdate(props, state, context); - } - } + // In order to support react-lifecycles-compat polyfilled components, + // Unsafe lifecycles should not be invoked for components using the new APIs. if ( - typeof this._instance.UNSAFE_componentWillUpdate === 'function' && - typeof type.getDerivedStateFromProps !== 'function' + typeof element.type.getDerivedStateFromProps !== 'function' && + typeof this._instance.getSnapshotBeforeUpdate !== 'function' ) { - // In order to support react-lifecycles-compat polyfilled components, - // Unsafe lifecycles should not be invoked for any component with the new gDSFP. - this._instance.UNSAFE_componentWillUpdate(props, state, context); + if (typeof this._instance.componentWillUpdate === 'function') { + this._instance.componentWillUpdate(props, state, context); + } + if (typeof this._instance.UNSAFE_componentWillUpdate === 'function') { + this._instance.UNSAFE_componentWillUpdate(props, state, context); + } } } diff --git a/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js b/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js index 2c6618d28b729..7e60bde00367f 100644 --- a/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js +++ b/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js @@ -128,6 +128,48 @@ describe('ReactShallowRenderer', () => { shallowRenderer.render(); }); + it('should not invoke deprecated lifecycles (cWM/cWRP/cWU) if new getSnapshotBeforeUpdate is present', () => { + class Component extends React.Component { + getSnapshotBeforeUpdate() { + return null; + } + componentWillMount() { + throw Error('unexpected'); + } + componentWillReceiveProps() { + throw Error('unexpected'); + } + componentWillUpdate() { + throw Error('unexpected'); + } + render() { + return null; + } + } + + const shallowRenderer = createRenderer(); + shallowRenderer.render(); + shallowRenderer.render(); + }); + + it('should not call getSnapshotBeforeUpdate or componentDidUpdate when updating since refs wont exist', () => { + class Component extends React.Component { + getSnapshotBeforeUpdate() { + throw Error('unexpected'); + } + componentDidUpdate() { + throw Error('unexpected'); + } + render() { + return null; + } + } + + const shallowRenderer = createRenderer(); + shallowRenderer.render(); + shallowRenderer.render(); + }); + it('should only render 1 level deep', () => { function Parent() { return ( diff --git a/packages/react/src/__tests__/createReactClassIntegration-test.js b/packages/react/src/__tests__/createReactClassIntegration-test.js index eedc2df740438..a1dadaef4b0a2 100644 --- a/packages/react/src/__tests__/createReactClassIntegration-test.js +++ b/packages/react/src/__tests__/createReactClassIntegration-test.js @@ -510,7 +510,7 @@ describe('create-react-class-integration', () => { expect(() => { ReactDOM.render(, document.createElement('div')); }).toWarnDev( - 'Unsafe legacy lifecycles will not be called for components using the new getDerivedStateFromProps() API.\n\n' + + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + 'Component uses getDerivedStateFromProps() but also contains the following legacy lifecycles:\n' + ' componentWillMount\n' + ' componentWillReceiveProps\n' + @@ -521,6 +521,40 @@ describe('create-react-class-integration', () => { ReactDOM.render(, document.createElement('div')); }); + it('should not invoke deprecated lifecycles (cWM/cWRP/cWU) if new getSnapshotBeforeUpdate is present', () => { + const Component = createReactClass({ + getSnapshotBeforeUpdate: function() { + return null; + }, + componentWillMount: function() { + throw Error('unexpected'); + }, + componentWillReceiveProps: function() { + throw Error('unexpected'); + }, + componentWillUpdate: function() { + throw Error('unexpected'); + }, + componentDidUpdate: function() {}, + render: function() { + return null; + }, + }); + + expect(() => { + ReactDOM.render(, document.createElement('div')); + }).toWarnDev( + 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + + 'Component uses getSnapshotBeforeUpdate() but also contains the following legacy lifecycles:\n' + + ' componentWillMount\n' + + ' componentWillReceiveProps\n' + + ' componentWillUpdate\n\n' + + 'The above lifecycles should be removed. Learn more about this warning here:\n' + + 'https://fb.me/react-async-component-lifecycle-hooks', + ); + ReactDOM.render(, document.createElement('div')); + }); + it('should invoke both deprecated and new lifecycles if both are present', () => { const log = []; From d4ac1062bb5b26d3d966c206323678698f346fbf Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 22 Mar 2018 13:41:15 -0700 Subject: [PATCH 06/11] Fixed a type-check --- packages/react-reconciler/src/ReactFiberClassComponent.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 271a415412af1..1318f4088e20b 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -510,9 +510,10 @@ export default function( foundWillUpdateName !== null ) { const componentName = getComponentName(workInProgress) || 'Component'; - const newApiName = ctor.getDerivedStateFromProps - ? 'getDerivedStateFromProps()' - : 'getSnapshotBeforeUpdate()'; + const newApiName = + typeof ctor.getDerivedStateFromProps === 'function' + ? 'getDerivedStateFromProps()' + : 'getSnapshotBeforeUpdate()'; if (!didWarnAboutLegacyLifecyclesAndDerivedState[componentName]) { warning( false, From ca09ef845e751e69287ecc33b543c3c2fe8bf658 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 26 Mar 2018 09:03:50 -0700 Subject: [PATCH 07/11] Converted did-warn objects to Sets in ReactFiberClassComponent --- .../src/ReactFiberClassComponent.js | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 1318f4088e20b..ed1db32e5e65c 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -46,20 +46,21 @@ let didWarnAboutLegacyLifecyclesAndDerivedState; let warnOnInvalidCallback; if (__DEV__) { - didWarnAboutStateAssignmentForComponent = {}; - didWarnAboutUndefinedDerivedState = {}; - didWarnAboutUninitializedState = {}; + didWarnAboutStateAssignmentForComponent = new Set(); + didWarnAboutUndefinedDerivedState = new Set(); + didWarnAboutUninitializedState = new Set(); didWarnAboutGetSnapshotBeforeUpdateWithoutDidUpdate = new Set(); - didWarnAboutLegacyLifecyclesAndDerivedState = {}; + didWarnAboutLegacyLifecyclesAndDerivedState = new Set(); - const didWarnOnInvalidCallback = {}; + const didWarnOnInvalidCallback = new Set(); warnOnInvalidCallback = function(callback: mixed, callerName: string) { if (callback === null || typeof callback === 'function') { return; } const key = `${callerName}_${(callback: any)}`; - if (!didWarnOnInvalidCallback[key]) { + if (!didWarnOnInvalidCallback.has(key)) { + didWarnOnInvalidCallback.add(key); warning( false, '%s(...): Expected the last optional `callback` argument to be a ' + @@ -67,7 +68,6 @@ if (__DEV__) { callerName, callback, ); - didWarnOnInvalidCallback[key] = true; } }; @@ -458,7 +458,8 @@ export default function( state === null ) { const componentName = getComponentName(workInProgress) || 'Component'; - if (!didWarnAboutUninitializedState[componentName]) { + if (!didWarnAboutUninitializedState.has(componentName)) { + didWarnAboutUninitializedState.add(componentName); warning( false, '%s: Did not properly initialize state during construction. ' + @@ -466,7 +467,6 @@ export default function( componentName, instance.state === null ? 'null' : 'undefined', ); - didWarnAboutUninitializedState[componentName] = true; } } @@ -514,7 +514,8 @@ export default function( typeof ctor.getDerivedStateFromProps === 'function' ? 'getDerivedStateFromProps()' : 'getSnapshotBeforeUpdate()'; - if (!didWarnAboutLegacyLifecyclesAndDerivedState[componentName]) { + if (!didWarnAboutLegacyLifecyclesAndDerivedState.has(componentName)) { + didWarnAboutLegacyLifecyclesAndDerivedState.add(componentName); warning( false, 'Unsafe legacy lifecycles will not be called for components using new component APIs.\n\n' + @@ -529,7 +530,6 @@ export default function( : '', foundWillUpdateName !== null ? `\n ${foundWillUpdateName}` : '', ); - didWarnAboutLegacyLifecyclesAndDerivedState[componentName] = true; } } } @@ -610,7 +610,8 @@ export default function( if (instance.state !== oldState) { if (__DEV__) { const componentName = getComponentName(workInProgress) || 'Component'; - if (!didWarnAboutStateAssignmentForComponent[componentName]) { + if (!didWarnAboutStateAssignmentForComponent.has(componentName)) { + didWarnAboutStateAssignmentForComponent.add(componentName); warning( false, '%s.componentWillReceiveProps(): Assigning directly to ' + @@ -618,7 +619,6 @@ export default function( 'constructor). Use setState instead.', componentName, ); - didWarnAboutStateAssignmentForComponent[componentName] = true; } } updater.enqueueReplaceState(instance, instance.state, null); @@ -652,14 +652,14 @@ export default function( if (__DEV__) { if (partialState === undefined) { const componentName = getComponentName(workInProgress) || 'Component'; - if (!didWarnAboutUndefinedDerivedState[componentName]) { + if (!didWarnAboutUndefinedDerivedState.has(componentName)) { + didWarnAboutUndefinedDerivedState.add(componentName); warning( false, '%s.getDerivedStateFromProps(): A valid state object (or null) must be returned. ' + 'You have returned undefined.', componentName, ); - didWarnAboutUndefinedDerivedState[componentName] = componentName; } } } From 88d4cac4cfde3374ee107fea4f967105829d6195 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 26 Mar 2018 09:08:43 -0700 Subject: [PATCH 08/11] Replaced redundant new lifecycle checks in a few methods --- .../src/ReactFiberClassComponent.js | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index ed1db32e5e65c..f18574dccf935 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -747,6 +747,10 @@ export default function( const newUnmaskedContext = getUnmaskedContext(workInProgress); const newContext = getMaskedContext(workInProgress, newUnmaskedContext); + const hasNewLifecycles = + typeof ctor.getDerivedStateFromProps === 'function' || + typeof instance.getSnapshotBeforeUpdate === 'function'; + // Note: During these life-cycles, instance.props/instance.state are what // ever the previously attempted to render - not the "current". However, // during componentDidUpdate we pass the "current" props. @@ -754,8 +758,7 @@ export default function( // In order to support react-lifecycles-compat polyfilled components, // Unsafe lifecycles should not be invoked for components using the new APIs. if ( - typeof ctor.getDerivedStateFromProps !== 'function' && - typeof instance.getSnapshotBeforeUpdate !== 'function' && + !hasNewLifecycles && (typeof instance.UNSAFE_componentWillReceiveProps === 'function' || typeof instance.componentWillReceiveProps === 'function') ) { @@ -866,8 +869,7 @@ export default function( // In order to support react-lifecycles-compat polyfilled components, // Unsafe lifecycles should not be invoked for components using the new APIs. if ( - typeof ctor.getDerivedStateFromProps !== 'function' && - typeof instance.getSnapshotBeforeUpdate !== 'function' && + !hasNewLifecycles && (typeof instance.UNSAFE_componentWillMount === 'function' || typeof instance.componentWillMount === 'function') ) { @@ -921,6 +923,10 @@ export default function( const newUnmaskedContext = getUnmaskedContext(workInProgress); const newContext = getMaskedContext(workInProgress, newUnmaskedContext); + const hasNewLifecycles = + typeof ctor.getDerivedStateFromProps === 'function' || + typeof instance.getSnapshotBeforeUpdate === 'function'; + // Note: During these life-cycles, instance.props/instance.state are what // ever the previously attempted to render - not the "current". However, // during componentDidUpdate we pass the "current" props. @@ -928,8 +934,7 @@ export default function( // In order to support react-lifecycles-compat polyfilled components, // Unsafe lifecycles should not be invoked for components using the new APIs. if ( - typeof ctor.getDerivedStateFromProps !== 'function' && - typeof instance.getSnapshotBeforeUpdate !== 'function' && + !hasNewLifecycles && (typeof instance.UNSAFE_componentWillReceiveProps === 'function' || typeof instance.componentWillReceiveProps === 'function') ) { @@ -1054,8 +1059,7 @@ export default function( // In order to support react-lifecycles-compat polyfilled components, // Unsafe lifecycles should not be invoked for components using the new APIs. if ( - typeof ctor.getDerivedStateFromProps !== 'function' && - typeof instance.getSnapshotBeforeUpdate !== 'function' && + !hasNewLifecycles && (typeof instance.UNSAFE_componentWillUpdate === 'function' || typeof instance.componentWillUpdate === 'function') ) { From 24286048487f40c09d76d63957fd131f89857698 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 26 Mar 2018 09:17:03 -0700 Subject: [PATCH 09/11] Check for polyfill suppress flag on cWU as well before warning --- packages/react-reconciler/src/ReactFiberClassComponent.js | 5 ++++- packages/react-reconciler/src/ReactStrictModeWarnings.js | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index f18574dccf935..4014e41e8145f 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -499,7 +499,10 @@ export default function( ) { foundWillReceivePropsName = 'UNSAFE_componentWillReceiveProps'; } - if (typeof instance.componentWillUpdate === 'function') { + if ( + typeof instance.componentWillUpdate === 'function' && + instance.componentWillUpdate.__suppressDeprecationWarning !== true + ) { foundWillUpdateName = 'componentWillUpdate'; } else if (typeof instance.UNSAFE_componentWillUpdate === 'function') { foundWillUpdateName = 'UNSAFE_componentWillUpdate'; diff --git a/packages/react-reconciler/src/ReactStrictModeWarnings.js b/packages/react-reconciler/src/ReactStrictModeWarnings.js index 0afaa74d520b4..de37ab620336c 100644 --- a/packages/react-reconciler/src/ReactStrictModeWarnings.js +++ b/packages/react-reconciler/src/ReactStrictModeWarnings.js @@ -213,7 +213,10 @@ if (__DEV__) { ) { pendingComponentWillReceivePropsWarnings.push(fiber); } - if (typeof instance.componentWillUpdate === 'function') { + if ( + typeof instance.componentWillUpdate === 'function' && + instance.componentWillUpdate.__suppressDeprecationWarning !== true + ) { pendingComponentWillUpdateWarnings.push(fiber); } }; From ebb695183e4954d0e48a34539a02f75ce95bd070 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 26 Mar 2018 09:39:51 -0700 Subject: [PATCH 10/11] Fixed UNSAFE_componentDidUpdate -> componentDidUpdate mistake --- packages/react-reconciler/src/ReactFiberClassComponent.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 4014e41e8145f..bf0baf0d865f3 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -370,7 +370,7 @@ export default function( if ( typeof instance.getSnapshotBeforeUpdate === 'function' && typeof instance.componentDidUpdate !== 'function' && - typeof instance.UNSAFE_componentDidUpdate !== 'function' && + typeof instance.componentDidUpdate !== 'function' && !didWarnAboutGetSnapshotBeforeUpdateWithoutDidUpdate.has(type) ) { didWarnAboutGetSnapshotBeforeUpdateWithoutDidUpdate.add(type); From a13ad5223e030869930b80004d8f058642bc18c3 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 26 Mar 2018 13:04:03 -0700 Subject: [PATCH 11/11] Added Snapshot bit to HostEffectMask --- packages/shared/ReactTypeOfSideEffect.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/shared/ReactTypeOfSideEffect.js b/packages/shared/ReactTypeOfSideEffect.js index 750b392f1b5ee..82e8c3342fc70 100644 --- a/packages/shared/ReactTypeOfSideEffect.js +++ b/packages/shared/ReactTypeOfSideEffect.js @@ -26,7 +26,7 @@ export const ErrLog = /* */ 0b000100000000; export const Snapshot = /* */ 0b100000000000; // Union of all host effects -export const HostEffectMask = /* */ 0b000111111111; +export const HostEffectMask = /* */ 0b100111111111; export const Incomplete = /* */ 0b001000000000; export const ShouldCapture = /* */ 0b010000000000;