reactjs / react.dev

The React documentation website
https://react.dev/
Creative Commons Attribution 4.0 International
11.03k stars 7.53k forks source link

Explain why componentDidMount is a better place to make an ajax request #302

Open gacosta89 opened 6 years ago

gacosta89 commented 6 years ago

In the docs, in componentWillMount section section it says:

Avoid introducing any side-effects or subscriptions in this method

And in the componentDidMount section:

If you need to load data from a remote endpoint, this is a good place to instantiate the network request

Which for me is confusing because I won't like to wait for the component to be mounted to dispatch an ajax call to fulfill the component data dependencies. I would like to do it as soon as possible, like in the constructor, not even in componentWillMount.

Clearly you may have a reason why you say this in the docs, I might be not seeing the bigger picture. So it will be nice if you can explain a bit more in detail or point me in the direction of where I can find the reason.

clemmy commented 6 years ago

I believe that the component to render’s state is last sent to the update queue in componentWillMount(). So, if the ajax request finishes before the render(), but after componentWillMount(), then the modified state won’t be included as a part of the render, and the changes in the view won’t be shown until another render happens.

You may be able to understand more from the source: https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactFiberClassComponent.js

gaearon commented 6 years ago

I won't like to wait for the component to be mounted to dispatch an ajax call to fulfill the component data dependencies. I would like to do it as soon as possible, like in the constructor, not even in componentWillMount.

If it's an async request, it won't be fulfilled by the time the component mounts anyway, regardless of where you fire it. This is because JS is single threaded, and the network request can't "come back" and be handled while we are still rendering. So the difference between firing it earlier and later is often negligible.

You're right that it matters in some rare cases though and for those cases it might make sense to break the recommendation. But you should be extra cautious as state can update before mounting, and if your data depends on state, you might have to refetch in that case. In other words: when in doubt, do it in componentDidMount.

The specific recommendation to avoid side effects in the constructor and Will* lifecycles is related to the changes we are making to allow rendering to be asynchronous and interruptible (in part to support use cases like this better). We are still figuring out the exact semantics of how it should work, so at the moment our recommendations are more conservative. As we use async rendering more in production we will provide a more specific guidance as to where to fire the requests without sacrificing either efficiency or correctness. But for now providing a clear migration path to async rendering (and thus being more conservative in our recommendations) is more important.

bvaughn commented 6 years ago

I'll mark this issue as a "good first issue" in case anyone wants to take Dan's note about async and interruptions and add it to the docs.

Put another way, in the future (once the async API is stable and you're using it), it will be possible for lifecycle hooks like componentWillMount to be invoked more than once before a render- or to be invoked once and then never rendered- depending on whether higher priority work interrupts.

That's why it's important for side-effects (eg your network requests) to live in methods like componentDidMount, because they are guaranteed to only invoke once and only if the component has actually mounted. So you avoid firing network requests off needlessly, or perhaps logging events multiple times and corrupting your metrics, etc. Following these patterns now will make your eventual upgrade to async- (something we hope you'll want to do for performance reasons)- much easier.

gaearon commented 6 years ago

It's important to not overload the docs with speculations about future behavior though. If explanation raises more questions than it answers I'd argue it would only make the docs more complex for the majority of readers.

bvaughn commented 6 years ago

I agree. But I also agree with OP that it's confusing and limiting for us to say "don't do this" without offering some hint at our rationale.

My elaboration above wasn't meant as a suggestion for what we should add to the docs, but rather to help whoever may work on adding the note to better understand why.

gacosta89 commented 6 years ago

Thanks for the quick replies!

I agree with you of course. I only felt it was counter intuitive, and wanted to know the whys of the recommendation, that's all.

Perhaps in the future, when the API is stable, we can come up with a concise explanation for this recommendation.

vishalvrv9 commented 6 years ago

Hello I'm a beginner to open source contributions. Have gone through the guidelines and was wondering if I should take up this issue?

bvaughn commented 6 years ago

This issue is all yours, @vishalvrv9! 😄

I've added an "in-progress" label so that others will know not to start work on the issue. If you change your mind about the issue, no worries! Just let me know so that I can remove the label and free it up for someone else to claim.

Cheers!

michaelgichia commented 6 years ago

@gacosta89 @gaearon maybe I have misunderstood componentWillMount. Please correct me if I'm wrong.

The sole purpose of componentWillMount is for prefetching such as Ajax calls as long as the state is updated on componentWillReceiveProps. This is because React will queue state changes. On the other hand, setState should not be called on componentWillMount for the reasons mentioned above.

bvaughn commented 6 years ago

The sole purpose of componentWillMount is for prefetching such as Ajax calls

This is correct, @michaelgichia. Early prefetching is the canonical use for componentWillMount. It lets your app make a GET request a little bit earlier than waiting for mount.

It's best to stick with read-only (eg GET) requests though, for the reasons mentioned above regarding future async behavior. I like Dan's summary above: "When in doubt, do it in componentDidMount"

On the other hand, setState should not be called on componentWillMount

It is actually okay to set state from within componentWillMount. You initialize state in the constructor and this method is kind of an alternate constructor for components that were created via createClass. (See here for an explicit list of where it's okay to set state.)

That being said, it's unlikely that a GET request sent from componentWillMount will complete (thus triggering a setState call) before render happens anyway.

michaelgichia commented 6 years ago

@bvaughn thank you for taking your time to write a comprehensive explanation. I hope others will find this thread too for a better understanding of React Lifecycle and Setstate.

kevinkiklee commented 6 years ago

@vishalvrv9 hi vishal! are you still working on this issue?

i don't see a in-progress label on this issue at the moment.
is this issue open again? i would love to work on this!

vishalvrv9 commented 6 years ago

Firstly, sorry for not keeping anyone posted.

@kevinkiklee Noticed there was no in-progress tag (as you mentioned) and hence didn't move forward with the work. I could happily finish this within the next 24hrs.

bvaughn commented 6 years ago

I've marked it in-progress then. 😄

xvusrmqj commented 4 years ago

not clear I get the things from the answers are :

  1. constructor ---- compentWillMount ---- [ Your request Data return] ----- render ---componentDidMount.
    if this case your data will not be rendered.

  2. constructor ---- compentWillMount ----- render ---- [ Your request Data return] ---componentDidMount.
    if this case your data will be rendered.

  3. constructor ---- compentWillMount ----- ren[ Your request Data return]der ---componentDidMount.
    this case is impossible beacuse js is single threaded.

right? but why? if before render, render the data. if after render, rerender it. is it not ok?