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

componentWillReceiveProps changes 1 #11233

Conversation

chrisnojima
Copy link
Contributor

Updating componentWillReceiveProps: More complicated than the didMount change.

See patterns here: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html

@chrisnojima chrisnojima changed the title WIP: componentWillReceiveProps changes componentWillReceiveProps changes 1 Apr 5, 2018
@chrisnojima
Copy link
Contributor Author

@keybase/react-hackers this is part of of updating componentWillReceiveProps. There are a couple of patterns that are based on the blog post above. Sometimes the intention is to just make some state off of props. sometimes we want to make a call, etc

@chrisnojima
Copy link
Contributor Author

note: i'm marking the folders stuff as unsafe as we're going to deprecate that whole section before any of this matters

@chrisnojima
Copy link
Contributor Author

@keybase/react-hackers bump

1 similar comment
@chrisnojima
Copy link
Contributor Author

@keybase/react-hackers bump

Copy link
Contributor

@buoyad buoyad left a comment

Choose a reason for hiding this comment

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

Looks good! A couple comments. Also, do you know what this is about? (thrown on startup)

Warning: Unsafe legacy lifecycles will not be called for components using new component APIs.

ErrorBoundary uses getDerivedStateFromProps() but also contains the following legacy lifecycles:
  componentWillReceiveProps

ErrorBoundary doesn't use componentWillReceiveProps as far as I can tell. Does this mean that the deprecated lifecycles won't be called for any children?

componentWillReceiveProps(nextProps: Props) {
if (nextProps.error !== this.props.error) {
componentDidUpdate(prevProps: Props) {
if (prevProps.error !== this.props.error) {
this.props.setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you store the timeoutIDs rather than _mounted and clearTimeout in componentWillUnmount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

?

}
}

componentDidUpdate(prevProps: Props, prevState: State) {
// We just got new settings for this team, reset any user selections
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment belongs in getDerivedStateFromProps now

@chrisnojima
Copy link
Contributor Author

@chrisnojima
Copy link
Contributor Author

they have a pr for hot loader: gaearon/react-hot-loader#927

@buoyad
Copy link
Contributor

buoyad commented Apr 10, 2018

@chrisnojima ok cool!

@chrisnojima
Copy link
Contributor Author

@buoyad ptal

@chrisnojima chrisnojima merged commit 83d265c into nojima/DESKTOP-6374-react-lifecycles Apr 10, 2018
@chrisnojima chrisnojima deleted the nojima/DESKTOP-6374-react-lifecycles-2 branch April 10, 2018 17:01
chrisnojima added a commit that referenced this pull request Apr 10, 2018
… to desktop (#11232)

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* fix rn storybook

* WIP

* WIP

* WIP

* WIP

* remove react infection in usertimings

* Upgrade Electron to 1.8.4 (#11204)

* bring back class properties

* WIP

* WIP

* use componentDidMount vs willMount. Add StrictMode to desktop

* componentWillReceiveProps changes 1 (#11233)

* WIP

* wip

* WIP

* WIP

* remove strictmode. remove _mounted (already a hoc so its protected) . add return null from getDerived
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants