Skip to content

Removal of React 16.3 unsafe lifecycles #6256

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

Closed
wants to merge 4 commits into from
Closed

Removal of React 16.3 unsafe lifecycles #6256

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
16044d7
refac: removing unsafe lifecycles
gustavo-depaula Jul 17, 2018
a572d94
refac: extracting state from constructor
gustavo-depaula Jul 18, 2018
98195cb
refac: requested changes
gustavo-depaula Aug 5, 2018
3d149d3
No need to return true.
timdorr Aug 5, 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
16 changes: 9 additions & 7 deletions packages/react-router/modules/Router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,6 @@ class Router extends React.Component {
};
}

state = {
match: this.computeMatch(this.props.history.location.pathname)
};

computeMatch(pathname) {
return {
path: "/",
Expand All @@ -46,8 +42,9 @@ class Router extends React.Component {
};
}

componentWillMount() {
Copy link
Member

Choose a reason for hiding this comment

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

Because we're mainly doing a subscription here, we should be following the React async guidelines and move the listener to a cDM: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#adding-event-listeners-or-subscriptions

I'm not sure where the best place for the invariant check would be. Possibly just the render method.

Copy link

@renatoagds renatoagds Jul 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@timdorr I think that invariant could stay in constructor and just move the subscription for cDM, since, IMO the invariant needs to be the first thing to do here.

Copy link

Choose a reason for hiding this comment

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

Technically, you could change the children of <Router /> when component is already rendered, causing it to re-render. If it's in constructor, you won't get the message then. Possibly static getDerivedStateFromProps again? As it's called every time component receives props.

Copy link

Choose a reason for hiding this comment

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

@grzegorzjudas That's a good point. I think we could follow with getDerivedStateFromProps for invariant, since it's called in Mounting and Updating phases.

const { children, history } = this.props;
constructor(props) {
Copy link
Member

Choose a reason for hiding this comment

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

The subscription can't go in the constructor. If the component is removed from the tree before rendering (which may happen in async rendering), we'll leak a subscription to the history instance. The subscription needs to be put in a componentDidMount.

super(props);
const { children, history } = props;

invariant(
children == null || React.Children.count(children) === 1,
Expand All @@ -62,13 +59,18 @@ class Router extends React.Component {
match: this.computeMatch(history.location.pathname)
});
});

this.state = {
match: this.computeMatch(this.props.history.location.pathname)
};
Copy link
Member

Choose a reason for hiding this comment

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

This change isn't needed. The class field above transpiles/desugars into this exact same form.

Copy link
Author

Choose a reason for hiding this comment

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

makes sense!

}

componentWillReceiveProps(nextProps) {
shouldComponentUpdate(nextProps) {
warning(
this.props.history === nextProps.history,
"You cannot change <Router history>"
);
return true;
Copy link
Member

Choose a reason for hiding this comment

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

I think this will have an unintended negative perf side effect. While not the worst thing in the world (we don't do a whole lot in the render), technically this will cause this component to render on every update, even if it's unneeded.

While the error message will need to change (we're not preventing the change, only warning that it will have no effect), I believe this actually belongs in cDU: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#side-effects-on-props-change

Copy link

Choose a reason for hiding this comment

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

@timdorr maybe we could use getDerivedStateFromProps here? It could guarantee that we're warning before the render.

Copy link

Choose a reason for hiding this comment

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

I think that's exactly what is suggested (to use getDerivedStateFromProps I mean) - article.

static getDerivedStateFromProps(nextProps) {
    warning(
        this.props.history === nextProps.history,
        "You cannot change <Router history>"
    );

    return null;
}

Copy link
Member

Choose a reason for hiding this comment

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

You can't use this in a static function.

Copy link

Choose a reason for hiding this comment

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

@timdorr @grzegorzjudas following what is proposed here (https://github.com/reactjs/rfcs/blob/master/text/0006-static-lifecycle-methods.md#state-derived-from-propsstate), I think we should mirror history in state and compare it in getDerivedStateFromProps, using the second parameter prevState.

Copy link
Member

Choose a reason for hiding this comment

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

gDSFP is intended to be a pure function, so I don't think that would work. cDU might be a better option.

Copy link

Choose a reason for hiding this comment

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

@timdorr Ah, right, my bad.

}

componentWillUnmount() {
Expand Down