Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New feature flags to help detect unexpected lifecycle side effects #11587

Merged
merged 4 commits into from
Nov 17, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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