Skip to content

warn against dispatching redux actions during componentWillUpdate at runtime or in docs? #9693

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

Closed
jedwards1211 opened this issue May 15, 2017 · 13 comments

Comments

@jedwards1211
Copy link
Contributor

Do you want to request a feature or report a bug?
Feature

What is the current behavior?
I had a component that was dispatching a redux action in its componentWillUpdate, which causing things to blow up in confusing ways. For instance, it was causing a different component's componentWillReceiveProps to get called for the second time before its componentDidUpdate got called, which led to various NPEs during the reconciliation process.

What is the expected behavior?
It would be nice if anything that would trigger an update to during a call to a component's componentWillUpdate would immediately error out in dev mode, even if it's causing a different component to update (in my case, triggering an ancestor react-redux container to update). Would that be possible?

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
I'm seeing it with React 0.14.9 and Chrome on macOS. I can't say for sure if my code caused the same bugs with previous versions of React, but the blame rests on my code anyway.

@igreulich
Copy link

I understand your headache, I have run into it as well, but the Lifecycle is well documented.

And since Redux operates on state, well.., I guess I am not sure why more NEEDS to be done.

@jedwards1211
Copy link
Contributor Author

The lifecycle is well documented from the perspective of one component, but what happens when a lifecycle method triggers and ancestor update is a big question in my mind. I've seen it work in most cases only to blow up in a few situations, and I don't really know why.

@igreulich
Copy link

igreulich commented May 30, 2017

I am not really sure we agree on what the issue is.

I would argue that state changes in the lifecycle methods is always bad. I would also think that any state changes would run through the redux store, so the change has already happened before the lifecycle methods are invoked, or caused them to invoke.

Ultimately it seems to me, granted with little information, that you should have an additional reducer listening for the action you dispatched that triggered the lifecycle methods in the first place, rather than rely on React to tell you when you made an error in Redux. (At least that seems like what you are asking, or suggesting, in the original issue.)

@mr47
Copy link

mr47 commented Jun 14, 2017

@igreulich i agree with you, if you making something on componentWillUpdate (like dispatching actions) you missing the point.

@jedwards1211
Copy link
Contributor Author

@igreulich I know it seems like my question is Redux-specific but there are any number of ways you can trigger ancestor updates from componentWillUpdate that don't involve Redux. For example:

  • pushing a new location with react-router
  • publishing a state change to some state framework besides Redux

I would argue that state changes in the lifecycle methods is always bad
Are you saying even in componentDidMount, componentDidUpdate, etc., it would be better to wrap state change calls in setTimeout? I'm not sure there's consensus on this in the React community, because I think I've even seen Dan Abramov encouraging people to dispatch actions in componentWillMount.

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Jun 14, 2017

@igreulich

Ultimately it seems to me, granted with little information, that you should have an additional reducer listening for the action you dispatched that triggered the lifecycle methods in the first place, rather than rely on React to tell you when you made an error in Redux.

I understand that sounds ideal, for instance have a reducer or middleware that responds to the location change from react-router-redux that causes the component to mount or update. But this is simply impossible for libraries like redux-form to do because they have to work independently of whatever routing framework you use or whatever causes a form component to mount. They have to dispatch actions from lifecycle methods, even if they wrap the dispatch in a setTimeout.

@mr47
Copy link

mr47 commented Jun 14, 2017

@jedwards1211 it's hard to believe but did you try to use internal component state with setState like a function, a heard that in some causes it can help. But if you want use something other than redux try react-jsonschema-form it's a not so fashion but works great with redux.

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Jun 14, 2017

Guys, everything I'm saying about redux, redux-form, react-router, etc. Is just an example. Here's what I'm trying to say:
Any code running in componentWillUpdate that causes an ancestor to update will probably cause errors later on. So if React can immediately error out when the ancestor update is triggered, rather than blowing up later with confusing error messages, then it will help everyone, including consumers if libraries that mistakenly do this.
In addition, the docs should explicitly state that you shouldn't update local or external state inside componentWillUpdate.

@mr47
Copy link

mr47 commented Jun 15, 2017

@jedwards1211 ok, i get it, but in react docs there is direct message that says

Note that you cannot call this.setState() here. If you need to update state in response to a prop change, use componentWillReceiveProps() instead.

If you are redux guy you should understand that update state (internal, external) is bad idea. Trying to fix that thing

blowing up later with confusing error messages

I think transactions in react and later on new reconciler fiber can't handle so simple that situation.

So i think deal with it 🚀

@gaearon
Copy link
Collaborator

gaearon commented Jul 11, 2017

I agree we should better document that you're not expected to cause side effects that synchronously update state in WillUpdate or WillReceiveProps.

@jedwards1211
Copy link
Contributor Author

@gaearon here is my PR for the docs about this: https://github.com/facebook/react/pull/9694/files

@bvaughn
Copy link
Contributor

bvaughn commented Oct 8, 2017

I agree we should better document that you're not expected to cause side effects that synchronously update state in WillUpdate or WillReceiveProps.

Hmm! @gaearon: Isn't it okay to call setState in componentWillReceiveProps? (Has this changed since I last checked?)

@bvaughn
Copy link
Contributor

bvaughn commented Oct 8, 2017

Thank you for filing this issue and raising these concerns.

The documentation and source code for reactjs.org now lives in a different repository: reactjs/reactjs.org. (For more info on why we made this move, see issue #11075.)

I closed the associated PR and added a comment explaining this.

I've also moved your issue to the new repo: reactjs/react.dev#74

Let's continue the discussion there! Sorry for the inconvenience.

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

5 participants