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

getDerivedStateFromProps for asynchronous setState #1147

Closed
flahertyb opened this issue Aug 30, 2018 · 3 comments
Closed

getDerivedStateFromProps for asynchronous setState #1147

flahertyb opened this issue Aug 30, 2018 · 3 comments

Comments

@flahertyb
Copy link

flahertyb commented Aug 30, 2018

I am inquiring about what is hopefully an undiscussed use case related to the componentWillReceiveProps -> getDerivedStateFromProps changes. I read through the blog post and threads like #721.

The use case is a button which says either "Save", "Saving", or "Saved!". The idea is that the button component is shared across apps, which pass a prop to the button indicating whether the app is currently saving. The button component's purpose is to encapsulate the logic that temporarily shows the text "Saved!", when saving finishes.

This can be handled with componentWillReceiveProps pretty simply: https://jsfiddle.net/Luktwrdm/1137/ - if the status prop changes in a certain way, show the "Saved!" text, and then use a timeout to later change itself back to a ready state ("Save").

Without componentWillReceiveProps, this case seems to require both getDerivedStateFromProps and componentDidUpdate, and an extra key in the state: https://jsfiddle.net/Luktwrdm/1138/ - You can't do it all in didUpdate because otherwise the component would flash with the incorrect copy for a split second. And you can't do it all in getDerivedStateFromProps because it's static.

The purpose of this issue is to clarify - is this (the second fiddle) now the best way to handle such a situation? Am I missing some higher level structural way to avoid this complexity?

I can imagine other use cases, like notifications, where it might make sense for a component to react to its props changing by making a temporary state change. Is that an anti-pattern? Or something worth covering in the docs?

@gaearon
Copy link
Member

gaearon commented Aug 30, 2018

I think your first implementation is subtly broken because if status changes like:

  • STATUS_LOADING
  • STATUS_READY
  • wait small amount (e.g. 10ms)
  • STATUS_LOADING
  • STATUS_READY

then the second READY change will set done to true but the callback from the first READY change will immediately flip it back to false.

This isn't related to your question but it's to demonstrate that the logic you seem to consider straightforward in the "before" example is a bit wrong.

Regarding your specific question:

You can't do it all in didUpdate because otherwise the component would flash with the incorrect copy for a split second

What makes you think so? I think componentDidUpdate alone should work just fine:

https://jsfiddle.net/k4bwe6oy/5/

    constructor(props) {
        super(props);
        this.state = { done: false, prevStatus: this.props.status }
        this.doneTimeout = null;
    }
  
    componentDidUpdate(prevProps) {
      if (prevProps.status === STATUS_LOADING && this.props.status === STATUS_READY) {
        clearTimeout(this.doneTimeout);
        this.setState({ done: true });
        this.doneTimeout = setTimeout(() => {
          this.setState({ done: false });
        }, 1000);
      }
    }
  
    componentWillUnmount() {
        clearTimeout(this.doneTimeout);
    }

(I also fixed the first issue by keeping track of the timeout.)

Perhaps you think that setState in componentDidUpdate is too late? The documentation explicitly says:

It would also cause an extra re-rendering which, while not visible to the user, can affect the component performance.

In this case there's no performance concern (the component tree is very shallow). The user will not see the intermediate state because setState in componentDidUpdate — just like in componentDidMount — is processed immediately and before React returns control to the browser.

@gaearon gaearon closed this as completed Aug 30, 2018
@flahertyb
Copy link
Author

Both points make total sense. You are right, I thought that setState() in componentDidUpdate caused an intermediate state. Sorry, I should have tested before posting, just have had the wrong idea in my head about that for a while now.

Really appreciate the response.

@gaearon
Copy link
Member

gaearon commented Aug 30, 2018

No problem!

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

No branches or pull requests

2 participants