Skip to content

Commit

Permalink
New feature flags to help detect unexpected lifecycle side effects (#…
Browse files Browse the repository at this point in the history
…11587)

Added `debugRenderPhaseSideEffects` feature flag to help detect unexpected side effects in pre-commit lifecycle hooks and `setState` reducers.
  • Loading branch information
bvaughn authored Nov 17, 2017
1 parent b5e4b45 commit 7f68544
Show file tree
Hide file tree
Showing 8 changed files with 167 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import invariant from 'fbjs/lib/invariant';
import typeof * as FeatureFlagsType from 'shared/ReactFeatureFlags';
import typeof * as CSFeatureFlagsType from './ReactNativeCSFeatureFlags';

export const debugRenderPhaseSideEffects = false;
export const enableAsyncSubtreeAPI = true;
export const enableAsyncSchedulingByDefaultInReactDOM = false;
export const enableReactFragment = false;
Expand Down
7 changes: 7 additions & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
Ref,
} from 'shared/ReactTypeOfSideEffect';
import {ReactCurrentOwner} from 'shared/ReactGlobalSharedState';
import {debugRenderPhaseSideEffects} from 'shared/ReactFeatureFlags';
import invariant from 'fbjs/lib/invariant';
import getComponentName from 'shared/getComponentName';
import warning from 'fbjs/lib/warning';
Expand Down Expand Up @@ -269,8 +270,14 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
if (__DEV__) {
ReactDebugCurrentFiber.setCurrentPhase('render');
nextChildren = instance.render();
if (debugRenderPhaseSideEffects) {
instance.render();
}
ReactDebugCurrentFiber.setCurrentPhase(null);
} else {
if (debugRenderPhaseSideEffects) {
instance.render();
}
nextChildren = instance.render();
}
// React DevTools reads this flag.
Expand Down
26 changes: 24 additions & 2 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ import type {Fiber} from './ReactFiber';
import type {ExpirationTime} from './ReactFiberExpirationTime';

import {Update} from 'shared/ReactTypeOfSideEffect';
import {enableAsyncSubtreeAPI} from 'shared/ReactFeatureFlags';
import {
debugRenderPhaseSideEffects,
enableAsyncSubtreeAPI,
} from 'shared/ReactFeatureFlags';
import {isMounted} from 'shared/ReactFiberTreeReflection';
import * as ReactInstanceMap from 'shared/ReactInstanceMap';
import emptyObject from 'fbjs/lib/emptyObject';
Expand Down Expand Up @@ -168,6 +171,11 @@ export default function(
);
stopPhaseTimer();

// Simulate an async bailout/interruption by invoking lifecycle twice.
if (debugRenderPhaseSideEffects) {
instance.shouldComponentUpdate(newProps, newState, newContext);
}

if (__DEV__) {
warning(
shouldUpdate !== undefined,
Expand Down Expand Up @@ -374,9 +382,13 @@ export default function(
startPhaseTimer(workInProgress, 'componentWillMount');
const oldState = instance.state;
instance.componentWillMount();

stopPhaseTimer();

// Simulate an async bailout/interruption by invoking lifecycle twice.
if (debugRenderPhaseSideEffects) {
instance.componentWillMount();
}

if (oldState !== instance.state) {
if (__DEV__) {
warning(
Expand All @@ -402,6 +414,11 @@ export default function(
instance.componentWillReceiveProps(newProps, newContext);
stopPhaseTimer();

// Simulate an async bailout/interruption by invoking lifecycle twice.
if (debugRenderPhaseSideEffects) {
instance.componentWillReceiveProps(newProps, newContext);
}

if (instance.state !== oldState) {
if (__DEV__) {
const componentName = getComponentName(workInProgress) || 'Component';
Expand Down Expand Up @@ -677,6 +694,11 @@ export default function(
startPhaseTimer(workInProgress, 'componentWillUpdate');
instance.componentWillUpdate(newProps, newState, newContext);
stopPhaseTimer();

// Simulate an async bailout/interruption by invoking lifecycle twice.
if (debugRenderPhaseSideEffects) {
instance.componentWillUpdate(newProps, newState, newContext);
}
}
if (typeof instance.componentDidUpdate === 'function') {
workInProgress.effectTag |= Update;
Expand Down
1 change: 1 addition & 0 deletions packages/react-reconciler/src/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,7 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
if (__DEV__) {
ReactDebugCurrentFiber.setCurrentFiber(workInProgress);
}

let next = beginWork(current, workInProgress, nextRenderExpirationTime);
if (__DEV__) {
ReactDebugCurrentFiber.resetCurrentFiber();
Expand Down
7 changes: 7 additions & 0 deletions packages/react-reconciler/src/ReactFiberUpdateQueue.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import type {Fiber} from './ReactFiber';
import type {ExpirationTime} from './ReactFiberExpirationTime';

import {debugRenderPhaseSideEffects} from 'shared/ReactFeatureFlags';
import {Callback as CallbackEffect} from 'shared/ReactTypeOfSideEffect';
import {ClassComponent, HostRoot} from 'shared/ReactTypeOfWork';
import invariant from 'fbjs/lib/invariant';
Expand Down Expand Up @@ -181,6 +182,12 @@ function getStateFromUpdate(update, instance, prevState, props) {
const partialState = update.partialState;
if (typeof partialState === 'function') {
const updateFn = partialState;

// Invoke setState callback an extra time to help detect side-effects.
if (debugRenderPhaseSideEffects) {
updateFn.call(instance, prevState, props);
}

return updateFn.call(instance, prevState, props);
} else {
return partialState;
Expand Down
123 changes: 123 additions & 0 deletions packages/react/src/__tests__/ReactAsyncClassComponent-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
*/

'use strict';

var React;
var ReactFeatureFlags;
var ReactTestRenderer;

describe('ReactAsyncClassComponent', () => {
describe('debugRenderPhaseSideEffects', () => {
beforeEach(() => {
jest.resetModules();
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.debugRenderPhaseSideEffects = true;
React = require('react');
ReactTestRenderer = require('react-test-renderer');
});

it('should invoke precommit lifecycle methods twice', () => {
let log = [];
let shouldComponentUpdate = false;
class ClassComponent extends React.Component {
state = {};
componentDidMount() {
log.push('componentDidMount');
}
componentDidUpdate() {
log.push('componentDidUpdate');
}
componentWillMount() {
log.push('componentWillMount');
}
componentWillReceiveProps() {
log.push('componentWillReceiveProps');
}
componentWillUnmount() {
log.push('componentWillUnmount');
}
componentWillUpdate() {
log.push('componentWillUpdate');
}
shouldComponentUpdate() {
log.push('shouldComponentUpdate');
return shouldComponentUpdate;
}
render() {
log.push('render');
return null;
}
}

const component = ReactTestRenderer.create(<ClassComponent />);
expect(log).toEqual([
'componentWillMount',
'componentWillMount',
'render',
'render',
'componentDidMount',
]);

log = [];
shouldComponentUpdate = true;

component.update(<ClassComponent />);
expect(log).toEqual([
'componentWillReceiveProps',
'componentWillReceiveProps',
'shouldComponentUpdate',
'shouldComponentUpdate',
'componentWillUpdate',
'componentWillUpdate',
'render',
'render',
'componentDidUpdate',
]);

log = [];
shouldComponentUpdate = false;

component.update(<ClassComponent />);
expect(log).toEqual([
'componentWillReceiveProps',
'componentWillReceiveProps',
'shouldComponentUpdate',
'shouldComponentUpdate',
]);
});

it('should invoke setState callbacks twice', () => {
class ClassComponent extends React.Component {
state = {
count: 1,
};
render() {
return null;
}
}

let setStateCount = 0;

const rendered = ReactTestRenderer.create(<ClassComponent />);
const instance = rendered.getInstance();
instance.setState(state => {
setStateCount++;
return {
count: state.count + 1,
};
});

// Callback should be invoked twice
expect(setStateCount).toBe(2);
// But each time `state` should be the previous value
expect(instance.state.count).toBe(2);
});
});
});
3 changes: 3 additions & 0 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ export const enableNoopReconciler = false;
// Experimental persistent mode (CS):
export const enablePersistentReconciler = false;

// Helps identify side effects in begin-phase lifecycle hooks and setState reducers:
export const debugRenderPhaseSideEffects = false;

// Only used in www builds.
export function addUserTimingListener() {
invariant(false, 'Not implemented.');
Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/shims/rollup/ReactFeatureFlags-www.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export const {
} = require('ReactFeatureFlags');

// The rest of the flags are static for better dead code elimination.
export const debugRenderPhaseSideEffects = false;
export const enableAsyncSubtreeAPI = true;
export const enableReactFragment = false;
export const enableCreateRoot = true;
Expand Down

0 comments on commit 7f68544

Please sign in to comment.