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

RFC 6: Deprecate unsafe lifecycles #12028

Merged
merged 28 commits into from
Jan 19, 2018
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
e709e30
Added unsafe_* lifecycles and deprecation warnings
bvaughn Jan 16, 2018
404944e
Ran lifecycle hook codemod over project
bvaughn Jan 16, 2018
bd300b3
Manually migrated CoffeeScript and TypeScript tests
bvaughn Jan 16, 2018
2868176
Added inline note to createReactClassIntegration-test
bvaughn Jan 16, 2018
8679926
Udated NativeMethodsMixin with new lifecycle hooks
bvaughn Jan 16, 2018
64f27d7
Added static getDerivedStateFromProps to ReactPartialRenderer
bvaughn Jan 17, 2018
1047182
Added getDerivedStateFromProps to shallow renderer
bvaughn Jan 17, 2018
035c220
Dedupe and DEV-only deprecation warning in server renderer
bvaughn Jan 17, 2018
8d0e001
Renamed unsafe_* prefix to UNSAFE_* to be more noticeable
bvaughn Jan 17, 2018
b71ca93
Added getDerivedStateFromProps to ReactFiberClassComponent
bvaughn Jan 17, 2018
09c39d0
Warn about UNSAFE_componentWillRecieveProps misspelling
bvaughn Jan 17, 2018
286df77
Added tests to createReactClassIntegration for new lifecycles
bvaughn Jan 17, 2018
b699543
Added warning for stateless functional components with gDSFP
bvaughn Jan 17, 2018
68f2fe7
Added createReactClass test for static gDSFP
bvaughn Jan 18, 2018
2d9f75d
Moved lifecycle deprecation warnings behind (disabled) feature flag
bvaughn Jan 18, 2018
8f125b7
Tidying up
bvaughn Jan 18, 2018
1d3e3d5
Merge branch 'master' into rfc-6
bvaughn Jan 18, 2018
d95ec49
Tweaked warning message wording slightly
bvaughn Jan 18, 2018
b940938
Replaced truthy partialState checks with != null
bvaughn Jan 18, 2018
6cd0a8e
Call getDerivedStateFromProps via .call(null) to prevent type access
bvaughn Jan 18, 2018
7572667
Move shallow-renderer didWarn* maps off the instance
bvaughn Jan 18, 2018
361a2cf
Only call getDerivedStateFromProps if props instance has changed
bvaughn Jan 18, 2018
8178d52
Avoid creating new state object if not necessary
bvaughn Jan 18, 2018
8d67e27
Inject state as a param to callGetDerivedStateFromProps
bvaughn Jan 18, 2018
53770c3
Explicitly warn about uninitialized state before calling getDerivedSt…
bvaughn Jan 18, 2018
3772ee2
Improved wording for deprecation lifecycle warnings
bvaughn Jan 18, 2018
4dfa6e1
Merge branch 'master' into rfc-6
bvaughn Jan 18, 2018
5609031
Fix state-regression for module-pattern components
bvaughn Jan 19, 2018
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 @@ -275,7 +275,7 @@ describe('ReactCallReturn', () => {
return ReactCallReturn.unstable_createReturn(this.props.children);
}

componentWillMount() {
UNSAFE_componentWillMount() {
ops.push(`Mount Return ${this.props.value}`);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/**
* 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';

let React;
let ReactDOM;
let ReactFeatureFlags;

describe('ReactComponentLifeCycle', () => {
beforeEach(() => {
jest.resetModules();

ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.warnAboutDeprecatedLifecycles = true;

React = require('react');
ReactDOM = require('react-dom');
});

// TODO (RFC #6) Merge this back into ReactComponentLifeCycles-test once
// the 'warnAboutDeprecatedLifecycles' feature flag has been removed.
it('warns about deprecated unsafe lifecycles', function() {
class MyComponent extends React.Component {
componentWillMount() {}
componentWillReceiveProps() {}
componentWillUpdate() {}
render() {
return null;
}
}

const container = document.createElement('div');
expect(() => ReactDOM.render(<MyComponent x={1} />, container)).toWarnDev([
'Warning: MyComponent: componentWillMount() is deprecated and will be ' +
'removed in the next major version. ' +
'Please use UNSAFE_componentWillMount() instead.',
]);
Copy link
Collaborator

@acdlite acdlite Jan 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like this?

Warning: MyComponent: componentWillMount() is deprecated and will be removed in the next major version. Read about the motivations behind this change: (link)

As a temporary workaround, you can rename to UNSAFE_componentWillMount instead.

(link) can point to some fburl that we write later.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now the link can be your RFC :)


expect(() => ReactDOM.render(<MyComponent x={2} />, container)).toWarnDev([
'Warning: MyComponent: componentWillReceiveProps() is deprecated and ' +
'will be removed in the next major version. ' +
'Please use UNSAFE_componentWillReceiveProps() instead.',
Copy link
Collaborator

@acdlite acdlite Jan 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this one, we should call out getDerivedStateFromProps as the preferred solution.

Warning: MyComponent: componentWillReceiveProps() is deprecated and will be removed in the next major version. Use getDerivedStateFromProps() instead. Read about the motivations behind this change: (link)

As a temporary workaround, you can rename to UNSAFE_componentWillReceiveProps instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for the improved wording suggestions!

I've made the wording improvements in 3772ee2.

'Warning: MyComponent: componentWillUpdate() is deprecated and will be ' +
'removed in the next major version. ' +
'Please use UNSAFE_componentWillUpdate() instead.',
]);

// Dedupe check (instantiate and update)
ReactDOM.render(<MyComponent key="new" x={1} />, container);
ReactDOM.render(<MyComponent key="new" x={2} />, container);
});
});
70 changes: 56 additions & 14 deletions packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ describe('ReactComponentLifeCycle', () => {
// had provided a getInitialState method.
it('throws when accessing state in componentWillMount', () => {
class StatefulComponent extends React.Component {
componentWillMount() {
UNSAFE_componentWillMount() {
void this.state.yada;
}

Expand All @@ -182,7 +182,7 @@ describe('ReactComponentLifeCycle', () => {

it('should allow update state inside of componentWillMount', () => {
class StatefulComponent extends React.Component {
componentWillMount() {
UNSAFE_componentWillMount() {
this.setState({stateField: 'something'});
}

Expand Down Expand Up @@ -231,7 +231,7 @@ describe('ReactComponentLifeCycle', () => {
// reaching into the updater.
return this.updater.isMounted(this);
}
componentWillMount() {
UNSAFE_componentWillMount() {
expect(this._isMounted()).toBeFalsy();
}
componentDidMount() {
Expand All @@ -258,7 +258,7 @@ describe('ReactComponentLifeCycle', () => {
// reaching into the updater.
return this.updater.isMounted(this);
}
componentWillMount() {
UNSAFE_componentWillMount() {
expect(this._isMounted()).toBeFalsy();
}
componentDidMount() {
Expand Down Expand Up @@ -334,7 +334,7 @@ describe('ReactComponentLifeCycle', () => {
this.state = initState;
}

componentWillMount() {
UNSAFE_componentWillMount() {
this._testJournal.stateAtStartOfWillMount = clone(this.state);
this._testJournal.lifeCycleAtStartOfWillMount = getLifeCycleState(this);
this.state.hasWillMountCompleted = true;
Expand Down Expand Up @@ -509,11 +509,17 @@ describe('ReactComponentLifeCycle', () => {
};
};
class Outer extends React.Component {
componentWillMount = logger('outer componentWillMount');
static getDerivedStateFromProps(props, prevState) {
log.push('outer getDerivedStateFromProps');
return null;
}
UNSAFE_componentWillMount = logger('outer componentWillMount');
componentDidMount = logger('outer componentDidMount');
componentWillReceiveProps = logger('outer componentWillReceiveProps');
UNSAFE_componentWillReceiveProps = logger(
'outer componentWillReceiveProps',
);
shouldComponentUpdate = logger('outer shouldComponentUpdate');
componentWillUpdate = logger('outer componentWillUpdate');
UNSAFE_componentWillUpdate = logger('outer componentWillUpdate');
componentDidUpdate = logger('outer componentDidUpdate');
componentWillUnmount = logger('outer componentWillUnmount');
render() {
Expand All @@ -526,11 +532,17 @@ describe('ReactComponentLifeCycle', () => {
}

class Inner extends React.Component {
componentWillMount = logger('inner componentWillMount');
static getDerivedStateFromProps(props, prevState) {
log.push('inner getDerivedStateFromProps');
return null;
}
UNSAFE_componentWillMount = logger('inner componentWillMount');
componentDidMount = logger('inner componentDidMount');
componentWillReceiveProps = logger('inner componentWillReceiveProps');
UNSAFE_componentWillReceiveProps = logger(
'inner componentWillReceiveProps',
);
shouldComponentUpdate = logger('inner shouldComponentUpdate');
componentWillUpdate = logger('inner componentWillUpdate');
UNSAFE_componentWillUpdate = logger('inner componentWillUpdate');
componentDidUpdate = logger('inner componentDidUpdate');
componentWillUnmount = logger('inner componentWillUnmount');
render() {
Expand All @@ -540,21 +552,33 @@ describe('ReactComponentLifeCycle', () => {

const container = document.createElement('div');
log = [];
ReactDOM.render(<Outer x={17} />, container);
expect(() => ReactDOM.render(<Outer x={1} />, container)).toWarnDev([
'Warning: Outer: Defines both componentWillReceiveProps() and static ' +
'getDerivedStateFromProps() methods. ' +
'We recommend using only getDerivedStateFromProps().',
'Warning: Inner: Defines both componentWillReceiveProps() and static ' +
'getDerivedStateFromProps() methods. ' +
'We recommend using only getDerivedStateFromProps().',
]);
expect(log).toEqual([
'outer getDerivedStateFromProps',
'outer componentWillMount',
'inner getDerivedStateFromProps',
'inner componentWillMount',
'inner componentDidMount',
'outer componentDidMount',
]);

// Dedup warnings
log = [];
ReactDOM.render(<Outer x={42} />, container);
ReactDOM.render(<Outer x={2} />, container);
expect(log).toEqual([
'outer componentWillReceiveProps',
'outer getDerivedStateFromProps',
'outer shouldComponentUpdate',
'outer componentWillUpdate',
'inner componentWillReceiveProps',
'inner getDerivedStateFromProps',
'inner shouldComponentUpdate',
'inner componentWillUpdate',
'inner componentDidUpdate',
Expand All @@ -579,7 +603,7 @@ describe('ReactComponentLifeCycle', () => {
log.push('render');
return <Child />;
},
componentWillMount() {
UNSAFE_componentWillMount() {
log.push('will mount');
},
componentDidMount() {
Expand Down Expand Up @@ -619,4 +643,22 @@ describe('ReactComponentLifeCycle', () => {
'ref',
]);
});

it('should warn if getDerivedStateFromProps returns undefined', () => {
class MyComponent extends React.Component {
static getDerivedStateFromProps() {}
render() {
return null;
}
}

const div = document.createElement('div');
expect(() => ReactDOM.render(<MyComponent />, div)).toWarnDev(
'MyComponent.getDerivedStateFromProps(): A valid state object (or null) must ' +
'be returned. You may have returned undefined.',
);

// De-duped
ReactDOM.render(<MyComponent />, div);
});
});
20 changes: 10 additions & 10 deletions packages/react-dom/src/__tests__/ReactCompositeComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -850,7 +850,7 @@ describe('ReactCompositeComponent', () => {
foo: PropTypes.string.isRequired,
};

componentWillReceiveProps(nextProps, nextContext) {
UNSAFE_componentWillReceiveProps(nextProps, nextContext) {
expect('foo' in nextContext).toBe(true);
}

Expand All @@ -865,7 +865,7 @@ describe('ReactCompositeComponent', () => {
}

class Intermediary extends React.Component {
componentWillReceiveProps(nextProps, nextContext) {
UNSAFE_componentWillReceiveProps(nextProps, nextContext) {
expect('foo' in nextContext).toBe(false);
}

Expand Down Expand Up @@ -916,7 +916,7 @@ describe('ReactCompositeComponent', () => {
foo: PropTypes.string.isRequired,
};

componentWillReceiveProps(nextProps, nextContext) {
UNSAFE_componentWillReceiveProps(nextProps, nextContext) {
expect('foo' in nextContext).toBe(true);

if (nextProps !== this.props) {
Expand All @@ -938,7 +938,7 @@ describe('ReactCompositeComponent', () => {
foo: PropTypes.string.isRequired,
};

componentWillReceiveProps(nextProps, nextContext) {
UNSAFE_componentWillReceiveProps(nextProps, nextContext) {
expect('foo' in nextContext).toBe(true);

if (nextProps !== this.props) {
Expand All @@ -956,7 +956,7 @@ describe('ReactCompositeComponent', () => {
}

class ChildWithoutContext extends React.Component {
componentWillReceiveProps(nextProps, nextContext) {
UNSAFE_componentWillReceiveProps(nextProps, nextContext) {
expect('foo' in nextContext).toBe(false);

if (nextProps !== this.props) {
Expand Down Expand Up @@ -1047,7 +1047,7 @@ describe('ReactCompositeComponent', () => {
class Component extends React.Component {
state = {updated: false};

componentWillReceiveProps(props) {
UNSAFE_componentWillReceiveProps(props) {
expect(props.update).toBe(1);
expect(renders).toBe(1);
this.setState({updated: true});
Expand Down Expand Up @@ -1075,7 +1075,7 @@ describe('ReactCompositeComponent', () => {
class Component extends React.Component {
state = {updated: false};

componentWillReceiveProps(props) {
UNSAFE_componentWillReceiveProps(props) {
expect(props.update).toBe(1);
expect(renders).toBe(1);
this.setState({updated: true});
Expand Down Expand Up @@ -1377,7 +1377,7 @@ describe('ReactCompositeComponent', () => {
const log = [];

class Spy extends React.Component {
componentWillMount() {
UNSAFE_componentWillMount() {
log.push(this.props.name + ' componentWillMount');
}
render() {
Expand Down Expand Up @@ -1556,7 +1556,7 @@ describe('ReactCompositeComponent', () => {
};
}

componentWillMount() {
UNSAFE_componentWillMount() {
this.setState(
{hasUpdatedState: true},
() => (stateSuccessfullyUpdated = this.state.hasUpdatedState),
Expand Down Expand Up @@ -1586,7 +1586,7 @@ describe('ReactCompositeComponent', () => {
};
}

componentWillMount() {
UNSAFE_componentWillMount() {
instance = this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ describe('ReactCompositeComponent-state', () => {
return <div>{this.state.color}</div>;
}

componentWillMount() {
UNSAFE_componentWillMount() {
this.peekAtState('componentWillMount-start');
this.setState(function(state) {
this.peekAtState('before-setState-sunrise', state);
Expand Down Expand Up @@ -78,7 +78,7 @@ describe('ReactCompositeComponent-state', () => {
this.peekAtState('componentDidMount-end');
}

componentWillReceiveProps(newProps) {
UNSAFE_componentWillReceiveProps(newProps) {
this.peekAtState('componentWillReceiveProps-start');
if (newProps.nextColor) {
this.setState(function(state) {
Expand All @@ -105,7 +105,7 @@ describe('ReactCompositeComponent-state', () => {
return true;
}

componentWillUpdate(nextProps, nextState) {
UNSAFE_componentWillUpdate(nextProps, nextState) {
this.peekAtState('componentWillUpdate-currentState');
this.peekAtState('componentWillUpdate-nextState', nextState);
}
Expand Down Expand Up @@ -323,7 +323,7 @@ describe('ReactCompositeComponent-state', () => {
}
let updated = false;
class Child extends React.Component {
componentWillReceiveProps() {
UNSAFE_componentWillReceiveProps() {
if (updated) {
return;
}
Expand Down Expand Up @@ -383,7 +383,7 @@ describe('ReactCompositeComponent-state', () => {
let ops = [];
class Test extends React.Component {
state = {step: 1, extra: true};
componentWillReceiveProps() {
UNSAFE_componentWillReceiveProps() {
this.setState({step: 2}, () => {
// Tests that earlier setState callbacks are not dropped
ops.push(
Expand Down Expand Up @@ -426,7 +426,7 @@ describe('ReactCompositeComponent-state', () => {
let ops = [];
class Test extends React.Component {
state = {step: 1, extra: true};
componentWillMount() {
UNSAFE_componentWillMount() {
this.setState({step: 2}, () => {
// Tests that earlier setState callbacks are not dropped
ops.push(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ describe('ReactDOMServerIntegration', () => {
getChildContext() {
return {text: this.state.text};
}
componentWillMount() {
UNSAFE_componentWillMount() {
this.setState({text: 'foo'});
}
render() {
Expand Down
Loading