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

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

Open
gacosta89 opened this issue Nov 18, 2017 · 15 comments
Open

Comments

@gacosta89
Copy link

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
Copy link
Contributor

clemmy commented Nov 18, 2017

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
Copy link
Member

gaearon commented Nov 18, 2017

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
Copy link
Contributor

bvaughn commented Nov 18, 2017

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
Copy link
Member

gaearon commented Nov 18, 2017

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
Copy link
Contributor

bvaughn commented Nov 18, 2017

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

bvaughn commented Nov 21, 2017

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
Copy link

@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
Copy link
Contributor

bvaughn commented Nov 27, 2017

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
Copy link

@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
Copy link

@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
Copy link
Contributor

vishalvrv9 commented Dec 7, 2017

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
Copy link
Contributor

bvaughn commented Dec 8, 2017

I've marked it in-progress then. 😄

@xvusrmqj
Copy link

xvusrmqj commented May 7, 2020

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?

BetterZxx pushed a commit to BetterZxx/react.dev that referenced this issue Mar 21, 2023
…actjs#302)

* docs(cn): translate content/docs/reference-profiler.md into Chinese

* update due to feedback

* fix due to feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants