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

Calling setState in componentDidUpdate #1875

Open
dlwalsh opened this issue Jul 30, 2018 · 10 comments
Open

Calling setState in componentDidUpdate #1875

dlwalsh opened this issue Jul 30, 2018 · 10 comments

Comments

@dlwalsh
Copy link
Contributor

@dlwalsh dlwalsh commented Jul 30, 2018

It might be worth revisiting react/no-did-update-set-state.

Now that componentWillReceiveProps() is being deprecated, componentDidUpdate() is the only "safe" way to detect changes in props and set state accordingly.

The React docs say calling setState() in componentDidUpdate() is permitted, albeit with caveats.

You may call setState() immediately in componentDidUpdate() but note that it must be wrapped in a condition like in the example above, or you’ll cause an infinite loop. It would also cause an extra re-rendering which, while not visible to the user, can affect the component performance.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Jul 30, 2018

getDerivedStateFromProps?

@dlwalsh
Copy link
Contributor Author

@dlwalsh dlwalsh commented Jul 30, 2018

The new lifecycle method getDerivedStateFromProps is not a perfect replacement for componentWillReceiveProps as it does not provide access to prevProps.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Jul 30, 2018

It would be reasonable, in React 16 and later, to modify the rule's behavior so that it does not warn on "safe" usages of setState.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Jul 30, 2018

(this would need to be done in eslint-plugin-react, however, not here)

@AlexanderSoliar
Copy link

@AlexanderSoliar AlexanderSoliar commented Jul 31, 2018

The new lifecycle method getDerivedStateFromProps is not a perfect replacement for componentWillReceiveProps as it does not provide access to prevProps.

@dlwalsh, you may want to check this technique from the official React blog but I would suggest to read the entire article -- maybe you can completely avoid usage of getDerivedStateFromProps :)

@dlwalsh
Copy link
Contributor Author

@dlwalsh dlwalsh commented Aug 1, 2018

Polluting state with previous props/state really sucks.

@luishdez
Copy link

@luishdez luishdez commented Nov 9, 2018

@dlwalsh yep it sucks, but it's inevitable in some cases. How do you change the state of a component reacting from a change from react-router that pass match through props?

@nickjuntilla
Copy link

@nickjuntilla nickjuntilla commented Jun 21, 2019

componentDidUpdate is the appropriate place to put asynchronous server updates so this warning is patently false, misleading and confusing.

@jdolearydl
Copy link

@jdolearydl jdolearydl commented Aug 21, 2019

This rule should be removed or modified to check for a condition. The React docs themselves explain how to safely update the state within componentDidUpdate:

You may call setState() immediately in componentDidUpdate() but note that it must be wrapped in a condition
React Docs

@benmosher
Copy link

@benmosher benmosher commented Sep 4, 2019

FWIW, I like to outsource lifecycle method work to (other) named methods in order to ensure they remain segmented by concern. This rule only looks for this.setState in the body of the lifecycle method, so you can do this and get no warning (regardless of whether there should be):

 componentDidUpdate(lastProps: Props) {
    this.updateCursorSuggestionIfNeeded(lastProps.data)
  }

  updateCursorSuggestionIfNeeded(lastData: WhateverData) {
    const { data } = this.props
    if (needsUpdate(data, lastData)) {
      // no way for linter to warn here, with or without the above condition
      this.setState(updatedStateForData(data))
    }
  }

Keeps things neat and negates the (in this case, unnecessary) lint warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.