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.

Sign up for GitHub

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

React lifecycles compat #12105

Merged
merged 4 commits into from
Jan 29, 2018
Merged

React lifecycles compat #12105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dc5b266
Suppress unsafe/deprecation warnings for polyfilled components
bvaughn Jan 26, 2018
7c4bbf6
Don't invoke deprecated lifecycles if static gDSFP exists
bvaughn Jan 26, 2018
a3635c9
Applied recent changes to server rendering also
bvaughn Jan 26, 2018
bd88689
Minor tweaks (inline comments, new tests) from PR feedback
bvaughn Jan 29, 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
55 changes: 55 additions & 0 deletions packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ describe('ReactComponentLifeCycle', () => {
ReactDOM = require('react-dom');
});

afterEach(() => {
jest.resetModules();
});

// TODO (RFC #6) Merge this back into ReactComponentLifeCycles-test once
// the 'warnAboutDeprecatedLifecycles' feature flag has been removed.
it('warns about deprecated unsafe lifecycles', function() {
Expand Down Expand Up @@ -55,4 +59,55 @@ describe('ReactComponentLifeCycle', () => {
ReactDOM.render(<MyComponent x={2} />, container);
ReactDOM.render(<MyComponent key="new" x={1} />, container);
});

describe('react-lifecycles-compat', () => {
// TODO Replace this with react-lifecycles-compat once it's been published
function polyfill(Component) {
Component.prototype.componentWillMount = function() {};
Component.prototype.componentWillMount.__suppressDeprecationWarning = true;
Component.prototype.componentWillReceiveProps = function() {};
Component.prototype.componentWillReceiveProps.__suppressDeprecationWarning = true;
}

it('should not warn about deprecated cWM/cWRP for polyfilled components', () => {
class PolyfilledComponent extends React.Component {
state = {};
static getDerivedStateFromProps() {
return null;
}
render() {
return null;
}
}

polyfill(PolyfilledComponent);

const container = document.createElement('div');
ReactDOM.render(<PolyfilledComponent />, container);
});

it('should not warn about unsafe lifecycles within "strict" tree for polyfilled components', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should also say "deprecated cWM/cWRP" rather than "unsafe lifecycles"?

Copy link
Contributor Author

@bvaughn bvaughn Jan 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Technically this test is verifying that the polyfill can add the old, deprecated methods (not the new ones with the UNSAFE_ prefix) without triggering a deprecation warning. This only really matters in the window between when we turn on the deprecation warnings and when 17 is released and those old methods go away entirely.

const {StrictMode} = React;

class PolyfilledComponent extends React.Component {
state = {};
static getDerivedStateFromProps() {
return null;
}
render() {
return null;
}
}

polyfill(PolyfilledComponent);

const container = document.createElement('div');
ReactDOM.render(
<StrictMode>
<PolyfilledComponent />
</StrictMode>,
container,
);
});
});
});
150 changes: 127 additions & 23 deletions packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ describe('ReactComponentLifeCycle', () => {
expect(instance.state.stateField).toBe('goodbye');
});

it('should call nested lifecycle methods in the right order', () => {
it('should call nested legacy lifecycle methods in the right order', () => {
let log;
const logger = function(msg) {
return function() {
Expand All @@ -509,11 +509,6 @@ describe('ReactComponentLifeCycle', () => {
};
};
class Outer extends React.Component {
state = {};
static getDerivedStateFromProps(props, prevState) {
log.push('outer getDerivedStateFromProps');
return null;
}
UNSAFE_componentWillMount = logger('outer componentWillMount');
componentDidMount = logger('outer componentDidMount');
UNSAFE_componentWillReceiveProps = logger(
Expand All @@ -533,11 +528,6 @@ describe('ReactComponentLifeCycle', () => {
}

class Inner extends React.Component {
state = {};
static getDerivedStateFromProps(props, prevState) {
log.push('inner getDerivedStateFromProps');
return null;
}
UNSAFE_componentWillMount = logger('inner componentWillMount');
componentDidMount = logger('inner componentDidMount');
UNSAFE_componentWillReceiveProps = logger(
Expand All @@ -554,18 +544,9 @@ describe('ReactComponentLifeCycle', () => {

const container = document.createElement('div');
log = [];
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().',
]);
ReactDOM.render(<Outer x={1} />, container);
expect(log).toEqual([
'outer getDerivedStateFromProps',
'outer componentWillMount',
'inner getDerivedStateFromProps',
'inner componentWillMount',
'inner componentDidMount',
'outer componentDidMount',
Expand All @@ -576,11 +557,9 @@ describe('ReactComponentLifeCycle', () => {
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 @@ -595,6 +574,131 @@ describe('ReactComponentLifeCycle', () => {
]);
});

it('should call nested new lifecycle methods in the right order', () => {
let log;
const logger = function(msg) {
return function() {
// return true for shouldComponentUpdate
log.push(msg);
return true;
};
};
class Outer extends React.Component {
state = {};
static getDerivedStateFromProps(props, prevState) {
log.push('outer getDerivedStateFromProps');
return null;
}
componentDidMount = logger('outer componentDidMount');
shouldComponentUpdate = logger('outer shouldComponentUpdate');
componentDidUpdate = logger('outer componentDidUpdate');
componentWillUnmount = logger('outer componentWillUnmount');
render() {
return (
<div>
<Inner x={this.props.x} />
</div>
);
}
}

class Inner extends React.Component {
state = {};
static getDerivedStateFromProps(props, prevState) {
log.push('inner getDerivedStateFromProps');
return null;
}
componentDidMount = logger('inner componentDidMount');
shouldComponentUpdate = logger('inner shouldComponentUpdate');
componentDidUpdate = logger('inner componentDidUpdate');
componentWillUnmount = logger('inner componentWillUnmount');
render() {
return <span>{this.props.x}</span>;
}
}

const container = document.createElement('div');
log = [];
ReactDOM.render(<Outer x={1} />, container);
expect(log).toEqual([
'outer getDerivedStateFromProps',
'inner getDerivedStateFromProps',
'inner componentDidMount',
'outer componentDidMount',
]);

// Dedup warnings
log = [];
ReactDOM.render(<Outer x={2} />, container);
expect(log).toEqual([
'outer getDerivedStateFromProps',
'outer shouldComponentUpdate',
'inner getDerivedStateFromProps',
'inner shouldComponentUpdate',
'inner componentDidUpdate',
'outer componentDidUpdate',
]);

log = [];
ReactDOM.unmountComponentAtNode(container);
expect(log).toEqual([
'outer componentWillUnmount',
'inner componentWillUnmount',
]);
});

it('should not invoke deprecated lifecycles (cWM/cWRP/cWU) if new static gDSFP is present', () => {
class Component extends React.Component {
state = {};
static getDerivedStateFromProps() {
return null;
}
componentWillMount() {
throw Error('unexpected');
}
componentWillReceiveProps() {
throw Error('unexpected');
}
componentWillUpdate() {
throw Error('unexpected');
}
render() {
return null;
}
}

const container = document.createElement('div');
expect(() => ReactDOM.render(<Component />, container)).toWarnDev(
'Defines both componentWillReceiveProps',
);
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

Worth adding a similar test for unsafe lifecycles maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure 👍

it('should not invoke new unsafe lifecycles (cWM/cWRP/cWU) if static gDSFP is present', () => {
class Component extends React.Component {
state = {};
static getDerivedStateFromProps() {
return null;
}
UNSAFE_componentWillMount() {
throw Error('unexpected');
}
UNSAFE_componentWillReceiveProps() {
throw Error('unexpected');
}
UNSAFE_componentWillUpdate() {
throw Error('unexpected');
}
render() {
return null;
}
}

const container = document.createElement('div');
expect(() => ReactDOM.render(<Component />, container)).toWarnDev(
'Defines both componentWillReceiveProps',
);
});

it('calls effects on module-pattern component', function() {
const log = [];

Expand Down
49 changes: 49 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMServerLifecycles-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,29 @@ describe('ReactDOMServerLifecycles', () => {
ReactDOMServer = require('react-dom/server');
});

afterEach(() => {
jest.resetModules();
});

it('should not invoke cWM if static gDSFP is present', () => {
class Component extends React.Component {
state = {};
static getDerivedStateFromProps() {
return null;
}
componentWillMount() {
throw Error('unexpected');
}
render() {
return null;
}
}

expect(() => ReactDOMServer.renderToString(<Component />)).toWarnDev(
'Component: componentWillMount() is deprecated and will be removed in the next major version.',
);
});

// TODO (RFC #6) Merge this back into ReactDOMServerLifecycles-test once
// the 'warnAboutDeprecatedLifecycles' feature flag has been removed.
it('should warn about deprecated lifecycle hooks', () => {
Expand All @@ -40,4 +63,30 @@ describe('ReactDOMServerLifecycles', () => {
// De-duped
ReactDOMServer.renderToString(<Component />);
});

describe('react-lifecycles-compat', () => {
// TODO Replace this with react-lifecycles-compat once it's been published
function polyfill(Component) {
Component.prototype.componentWillMount = function() {};
Component.prototype.componentWillMount.__suppressDeprecationWarning = true;
Component.prototype.componentWillReceiveProps = function() {};
Component.prototype.componentWillReceiveProps.__suppressDeprecationWarning = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This pollutes the react module but there's no resetModules() in afterEach. Let's add it so that any tests added later to this file aren't affected by this monkeypatching.

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 call.

}

it('should not warn about deprecated cWM/cWRP for polyfilled components', () => {
class PolyfilledComponent extends React.Component {
state = {};
static getDerivedStateFromProps() {
return null;
}
render() {
return null;
}
}

polyfill(PolyfilledComponent);

ReactDOMServer.renderToString(<PolyfilledComponent />);
});
});
});
55 changes: 54 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMServerLifecycles-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('ReactDOMServerLifecycles', () => {
resetModules();
});

it('should invoke the correct lifecycle hooks', () => {
it('should invoke the correct legacy lifecycle hooks', () => {
const log = [];

class Outer extends React.Component {
Expand Down Expand Up @@ -65,6 +65,59 @@ describe('ReactDOMServerLifecycles', () => {
]);
});

it('should invoke the correct new lifecycle hooks', () => {
const log = [];

class Outer extends React.Component {
state = {};
static getDerivedStateFromProps() {
log.push('outer getDerivedStateFromProps');
return null;
}
render() {
log.push('outer render');
return <Inner />;
}
}

class Inner extends React.Component {
state = {};
static getDerivedStateFromProps() {
log.push('inner getDerivedStateFromProps');
return null;
}
render() {
log.push('inner render');
return null;
}
}

ReactDOMServer.renderToString(<Outer />);
expect(log).toEqual([
'outer getDerivedStateFromProps',
'outer render',
'inner getDerivedStateFromProps',
'inner render',
]);
});

it('should not invoke unsafe cWM if static gDSFP is present', () => {
class Component extends React.Component {
state = {};
static getDerivedStateFromProps() {
return null;
}
UNSAFE_componentWillMount() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want to have a similar test for non-unsafe naming too?

throw Error('unexpected');
}
render() {
return null;
}
}

ReactDOMServer.renderToString(<Component />);
});

it('should update instance.state with value returned from getDerivedStateFromProps', () => {
class Grandparent extends React.Component {
state = {
Expand Down
Loading