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

Deprecate componentWillMount Maybe? #7671

Closed
sebmarkbage opened this Issue Sep 7, 2016 · 81 comments

Comments

Projects
None yet
@sebmarkbage
Member

sebmarkbage commented Sep 7, 2016

Let's use this thread to discuss use cases for componentWillMount and alternative solutions to those problems. Generally the solution is simply to use componentDidMount and two pass rendering if necessary.

There are several problems with doing global side-effects in the "componentWill" phase. That includes starting network requests or subscribing to Flux stores etc.

  1. It is confusing when used with error boundaries because currently componentWillUnmount can be called without componentDidMount ever being called. componentWill* is a false promise until all the children have successfully completed. Currently, this only applies when error boundaries are used but we'll probably want to revert this decision and simply not call componentWillUnmount here.

  2. The Fiber experiment doesn't really have a good way to call componentWillUnmount when a new render gets aborted because a higher priority update interrupted it. Similarly, our sister project ComponentKit does reconciliation in threads where it is not safe to perform side-effects yet.

  3. Callbacks from componentWillMount that update parent components with a setState is completely unsupported and lead to strange and order dependent race conditions. We already know that we want to deprecate that pattern.

  4. The reconciliation order of children can easily be dependent upon if you perform global side-effects in componentWillMount. They're already not fully guaranteed because updates can cause unexpected reconciliation orders. Relying on order also limits future use cases such as async or streaming rendering and parallelized rendering.

The only legit use case for componentWillMount is to call this.setState on yourself. Even then you never really need it since you can just initialize your initial state to whatever you had. We only really kept it around for a very specific use case:

class Foo {
  state = { data: null };
  // ANTI-PATTERN
  componentWillMount() {
    this._subscription = GlobalStore.getFromCacheOrFetch(data => this.setState({ data: data });
  }
  componentWillUnmount() {
    if (this._subscription) {
      GlobalStore.cancel(this._subscription);
    }
  }
  ...
}

When the same callback can be used both synchronously and asynchronously it is convenient to avoid an extra rerender if data is already available.

The solution is to split this API out into a synchronous version and an asynchronous version.

class Foo {
  state = { data: GlobalStore.getFromCacheOrNull() };
  componentDidMount() {
    if (!this.state.data) {
      this._subscription = GlobalStore.fetch(data => this.setState({ data: data });
    }
  }
  componentWillUnmount() {
    if (this._subscription) {
      GlobalStore.cancel(this._subscription);
    }
  }
  ...
}

This guarantees that the side-effect only happens if the component successfully mounts. If the async side-effect is needed, then a two-pass rendering is needed regardless.

I'd argue that it is not too much boilerplate since you need a componentWillUnmount anyway. This can all be hidden inside a Higher-Order Component.

Global side-effects in componentWillReceiveProps and componentWillUpdate are also bad since they're not guaranteed to complete. Due to aborts or errors. You should prefer componentDidUpdate when possible. However, they will likely remain in some form even if their use case is constrained. They're also not nearly as bad since they will still get their componentWillUnmount invoked for cleanup.

@sebmarkbage sebmarkbage changed the title from Deprecate componentWillMount to Deprecate componentWillMount Maybe? Sep 7, 2016

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Sep 7, 2016

Contributor

I mostly use cWM to perform setState based on initial props, since cWRP doesn't fire initially. I could do it in constructor, but I try and avoid extending that method when possible because I have an allergy to super calls :/

Contributor

probablyup commented Sep 7, 2016

I mostly use cWM to perform setState based on initial props, since cWRP doesn't fire initially. I could do it in constructor, but I try and avoid extending that method when possible because I have an allergy to super calls :/

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Sep 7, 2016

Member

@yaycmyk If you could use field initializers would you use that instead?

class Foo {
  state = { data: this.props.initialData };
  ...
}

or for something more complicated:

class Foo {
  state = this.computeInitialState();
  computeInitialState() {
    var state = { data: null};
    if (this.props.something) {
      state.data = this.props.somethingElse;
    }
    return state;
  }
  ...
}
Member

sebmarkbage commented Sep 7, 2016

@yaycmyk If you could use field initializers would you use that instead?

class Foo {
  state = { data: this.props.initialData };
  ...
}

or for something more complicated:

class Foo {
  state = this.computeInitialState();
  computeInitialState() {
    var state = { data: null};
    if (this.props.something) {
      state.data = this.props.somethingElse;
    }
    return state;
  }
  ...
}
@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Sep 7, 2016

Collaborator

for createClass cWM is the only place to do constructor stuff, since you can't actually define a constructor. I need to do a grep through some of our code bases I think there are a couple other use cases/optimizations that use it I'm not remembering

Collaborator

jquense commented Sep 7, 2016

for createClass cWM is the only place to do constructor stuff, since you can't actually define a constructor. I need to do a grep through some of our code bases I think there are a couple other use cases/optimizations that use it I'm not remembering

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Sep 7, 2016

Contributor

@sebmarkbage I do use property intializers, though they become clumsy if you need a non-trivial amount of Boolean logic. To be fair, they do cover 95% of all my use cases. cWM is a rare sighting in our products

Contributor

probablyup commented Sep 7, 2016

@sebmarkbage I do use property intializers, though they become clumsy if you need a non-trivial amount of Boolean logic. To be fair, they do cover 95% of all my use cases. cWM is a rare sighting in our products

@mars

This comment has been minimized.

Show comment
Hide comment
@mars

mars Sep 7, 2016

For server rendering, componentWillMount fires but not componentDidMount.

What would replace the browser-only nature of componentDidMount? Adding if (typeof window === 'undefined') expressions to detect render mode in componentDidMount seems dirty compared to the current lifecycle function approach.

mars commented Sep 7, 2016

For server rendering, componentWillMount fires but not componentDidMount.

What would replace the browser-only nature of componentDidMount? Adding if (typeof window === 'undefined') expressions to detect render mode in componentDidMount seems dirty compared to the current lifecycle function approach.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Sep 7, 2016

Member

@mars componentDidMount would still only fire on the client. Do you have a use case in mind for componentWillMount on the server?

Member

sebmarkbage commented Sep 7, 2016

@mars componentDidMount would still only fire on the client. Do you have a use case in mind for componentWillMount on the server?

@petehunt

This comment has been minimized.

Show comment
Hide comment
@petehunt

petehunt Sep 7, 2016

Contributor

I only use it to set up data structures on this for things like memoization, but with ES6 class constructors there's no reason for it anymore.

Contributor

petehunt commented Sep 7, 2016

I only use it to set up data structures on this for things like memoization, but with ES6 class constructors there's no reason for it anymore.

@mars

This comment has been minimized.

Show comment
Hide comment
@mars

mars Sep 7, 2016

After looking back at my last app that used server-rendering, the only componentWillMount use case is an initial setState. So, following @sebmarkbage's suggestion to conditionally setState in componentDidMount would be a fine way to allow deprecation of componentWillMount.

mars commented Sep 7, 2016

After looking back at my last app that used server-rendering, the only componentWillMount use case is an initial setState. So, following @sebmarkbage's suggestion to conditionally setState in componentDidMount would be a fine way to allow deprecation of componentWillMount.

@ryanflorence

This comment has been minimized.

Show comment
Hide comment
@ryanflorence

ryanflorence Sep 7, 2016

Contributor

This issue is probably because of some of my recent hackery. Today, given that when componentWillMount is called you can actually trust that it will be mounted, you can do stuff like register descendants with an ancestor and use that information in render in another ancestor.

http://codepen.io/anon/pen/gwbEJy?editors=0010

As a sort of DOM parallel, when you have a form that has two inputs where one of them is a button, then "keyDown" of a text input causes the form to submit. In other words, the presence or non-presence of another ancestor changes the behavior of another ancestor element. Also, radio groups.

Not saying "keep componentWillMount" but it's a great way to create relationships between ancestors of a context. It would be nice to either 1) have a componentMountCancelled (sounds unlikely, or 2) call cDM on the server (???) because that's the only reason I do this in cWM or 3) have some first class API for building this kind of relationship between ancestors in the same context.

I'd like (3), but have no idea what that looks like.

For the use-case here, it's in the React Router v4 API:

<div>
  <Match pattern="/users" component={Users}/>
  <Match pattern="/about" component={About}/>
  <Miss component={NoMatch}/>
</div>

If no <Match/> matches the location, then the descendant <Miss/> components in context will be rendered (and it's beautiful, isn't it?!)

Only reason this matters is server rendering. Thanks to some talks with @sebmarkbage we've got a plan B, but I am a little 😢 about this. (I think we might get a flicker when we move to cDM ... which is not exciting.)

I'm pretty sure @kenwheeler's new <Song> employs this technique also, but could probably move to cDM.

Contributor

ryanflorence commented Sep 7, 2016

This issue is probably because of some of my recent hackery. Today, given that when componentWillMount is called you can actually trust that it will be mounted, you can do stuff like register descendants with an ancestor and use that information in render in another ancestor.

http://codepen.io/anon/pen/gwbEJy?editors=0010

As a sort of DOM parallel, when you have a form that has two inputs where one of them is a button, then "keyDown" of a text input causes the form to submit. In other words, the presence or non-presence of another ancestor changes the behavior of another ancestor element. Also, radio groups.

Not saying "keep componentWillMount" but it's a great way to create relationships between ancestors of a context. It would be nice to either 1) have a componentMountCancelled (sounds unlikely, or 2) call cDM on the server (???) because that's the only reason I do this in cWM or 3) have some first class API for building this kind of relationship between ancestors in the same context.

I'd like (3), but have no idea what that looks like.

For the use-case here, it's in the React Router v4 API:

<div>
  <Match pattern="/users" component={Users}/>
  <Match pattern="/about" component={About}/>
  <Miss component={NoMatch}/>
</div>

If no <Match/> matches the location, then the descendant <Miss/> components in context will be rendered (and it's beautiful, isn't it?!)

Only reason this matters is server rendering. Thanks to some talks with @sebmarkbage we've got a plan B, but I am a little 😢 about this. (I think we might get a flicker when we move to cDM ... which is not exciting.)

I'm pretty sure @kenwheeler's new <Song> employs this technique also, but could probably move to cDM.

rgbkrk added a commit to rgbkrk/nteract that referenced this issue Sep 7, 2016

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Sep 7, 2016

Contributor

@ryanflorence Why not just extend constructor in that case though? cDM is really a last resort since it's way less efficient from needing a second render pass.

Contributor

probablyup commented Sep 7, 2016

@ryanflorence Why not just extend constructor in that case though? cDM is really a last resort since it's way less efficient from needing a second render pass.

@nickzuber

This comment has been minimized.

Show comment
Hide comment
@nickzuber

nickzuber Sep 7, 2016

Would there ever be a case where we'd need to compute some data or perform a certain routine before the component mounts, that we otherwise wouldn't be able to do in the component constructor?

What if we needed or wanted to fire off a Redux action before the component mounts for the first time? Or what about wanting to attach an event listener to the component before it mounts?

I'm just worried that there might be some niche use cases for componentWillMount that we aren't thinking of here.

@ajwhite I'm curious what you think about this

nickzuber commented Sep 7, 2016

Would there ever be a case where we'd need to compute some data or perform a certain routine before the component mounts, that we otherwise wouldn't be able to do in the component constructor?

What if we needed or wanted to fire off a Redux action before the component mounts for the first time? Or what about wanting to attach an event listener to the component before it mounts?

I'm just worried that there might be some niche use cases for componentWillMount that we aren't thinking of here.

@ajwhite I'm curious what you think about this

@aflanagan

This comment has been minimized.

Show comment
Hide comment
@aflanagan

aflanagan Sep 7, 2016

What if we needed or wanted to fire off a Redux action before the component mounts for the first time?

@nickzuber I was just coming to mention this. I work on a rails app that uses react-rails for server rendering a collection of smaller react/redux client apps. A pattern that is working well for us is to dispatch actions that populate our reducers with initial data in componentWillMount. That way you can easily pass data into your app from the rails view layer using react_component, and populate the state before rendering.

aflanagan commented Sep 7, 2016

What if we needed or wanted to fire off a Redux action before the component mounts for the first time?

@nickzuber I was just coming to mention this. I work on a rails app that uses react-rails for server rendering a collection of smaller react/redux client apps. A pattern that is working well for us is to dispatch actions that populate our reducers with initial data in componentWillMount. That way you can easily pass data into your app from the rails view layer using react_component, and populate the state before rendering.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Sep 7, 2016

Member

@ryanflorence This is more in response to Fiber and implementing componentWillUnmount.

For your use case, you don't really need componentWillUnmount since the use case is on the server anyway. You also don't need setState. So, technically you can still do the same thing in the constructor even though it is frowned upon. :) However, I think that for your use case a server-side-only componentDidMount might make the most sense (componentDidServerRender?). It would fire after rendering so it still requires two-passes but preserves the other capabilities.

Member

sebmarkbage commented Sep 7, 2016

@ryanflorence This is more in response to Fiber and implementing componentWillUnmount.

For your use case, you don't really need componentWillUnmount since the use case is on the server anyway. You also don't need setState. So, technically you can still do the same thing in the constructor even though it is frowned upon. :) However, I think that for your use case a server-side-only componentDidMount might make the most sense (componentDidServerRender?). It would fire after rendering so it still requires two-passes but preserves the other capabilities.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Sep 7, 2016

Member

@aflanagan Can you use the field initializer pattern (or constructor) that I used above to initialize the state from the props?

Member

sebmarkbage commented Sep 7, 2016

@aflanagan Can you use the field initializer pattern (or constructor) that I used above to initialize the state from the props?

@aflanagan

This comment has been minimized.

Show comment
Hide comment
@aflanagan

aflanagan Sep 7, 2016

@sebmarkbage ah yes, I think we could do that. We adopted the cWM strategy before we moved to es6 classes, and I hadn't considered that we could start doing that in the constructor.

aflanagan commented Sep 7, 2016

@sebmarkbage ah yes, I think we could do that. We adopted the cWM strategy before we moved to es6 classes, and I hadn't considered that we could start doing that in the constructor.

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Sep 7, 2016

Contributor

To help tackle the server use case, what about a new method like componentDidRender? For the browser lifecycle, it'd come before cDM. The idea being the JSX has been rendered/"compiled" into vdom but not yet mounted somewhere.

Contributor

probablyup commented Sep 7, 2016

To help tackle the server use case, what about a new method like componentDidRender? For the browser lifecycle, it'd come before cDM. The idea being the JSX has been rendered/"compiled" into vdom but not yet mounted somewhere.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Sep 7, 2016

Member

There is one use case, that is also the same as the only use case for componentWillUpdate that I know about. Basically, you read some global mutable state so that you can reset it later in componentDidUpdate, after the children has already rendered.

For example, if the children rendering causes the scroll position of a page to change.

class ScrollRestore {
  componentWillMount() {
    this._scroll = Global.scrollTop;
  }
  componentDidMount() {
    Global.scrollTop = this._scroll;
  }
  componentWillUpdate() {
    this._scroll = Global.scrollTop;
  }
  componentDidUpdate() {
    Global.scrollTop = this._scroll;
  }
  ...
}

Technically, you can do the same thing in the constructor but it is kind of iffy to have impure constructors.

Member

sebmarkbage commented Sep 7, 2016

There is one use case, that is also the same as the only use case for componentWillUpdate that I know about. Basically, you read some global mutable state so that you can reset it later in componentDidUpdate, after the children has already rendered.

For example, if the children rendering causes the scroll position of a page to change.

class ScrollRestore {
  componentWillMount() {
    this._scroll = Global.scrollTop;
  }
  componentDidMount() {
    Global.scrollTop = this._scroll;
  }
  componentWillUpdate() {
    this._scroll = Global.scrollTop;
  }
  componentDidUpdate() {
    Global.scrollTop = this._scroll;
  }
  ...
}

Technically, you can do the same thing in the constructor but it is kind of iffy to have impure constructors.

@mridgway

This comment has been minimized.

Show comment
Hide comment
@mridgway

mridgway Sep 7, 2016

Contributor

Just wanted to chime in that componentWillMount causes a lot of confusion in the server-rendering and side-loading use cases where it seems like a good option but doesn't really work the way you expected since it's synchronous. I think the only use case that would have to be reconsidered with this deprecation is when you're still using React.createClass and don't have a convenient way to add to the constructor.

Contributor

mridgway commented Sep 7, 2016

Just wanted to chime in that componentWillMount causes a lot of confusion in the server-rendering and side-loading use cases where it seems like a good option but doesn't really work the way you expected since it's synchronous. I think the only use case that would have to be reconsidered with this deprecation is when you're still using React.createClass and don't have a convenient way to add to the constructor.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Sep 7, 2016

Contributor

Airbnb's primary use cases for componentWillMount are to seed our global translation singleton with phrases, which come from props, are needed in the render path, and are not necessarily available at construction time; and to bootstrap Alt stores with props.

All of our translations will break in the former case, if this lifecycle method is deprecated. Since we primarily handle phrases at the top level of a tree, ordering nondeterminism is not a concern for us.

In addition, constructors should never have side effects - componentWillMount provides us with an appropriate place to put side effects, that should basically only happen once per environment, that executes both on the client and server (componentDidMount, of course, would be for client-only).

Contributor

ljharb commented Sep 7, 2016

Airbnb's primary use cases for componentWillMount are to seed our global translation singleton with phrases, which come from props, are needed in the render path, and are not necessarily available at construction time; and to bootstrap Alt stores with props.

All of our translations will break in the former case, if this lifecycle method is deprecated. Since we primarily handle phrases at the top level of a tree, ordering nondeterminism is not a concern for us.

In addition, constructors should never have side effects - componentWillMount provides us with an appropriate place to put side effects, that should basically only happen once per environment, that executes both on the client and server (componentDidMount, of course, would be for client-only).

@buildbreakdo

This comment has been minimized.

Show comment
Hide comment
@buildbreakdo

buildbreakdo Sep 7, 2016

Thought this GitHub search might be useful to see some use cases, sorted by recently indexed so it represents how people are actively using componentWillMount in the wild today.

Note: You must be logged in to search for code across all public repositories

https://github.com/search?l=javascript&o=desc&p=1&q=componentWillMount%28%29+language%3AJavaScript&ref=advsearch&s=indexed&type=Code&utf8=%E2%9C%93

buildbreakdo commented Sep 7, 2016

Thought this GitHub search might be useful to see some use cases, sorted by recently indexed so it represents how people are actively using componentWillMount in the wild today.

Note: You must be logged in to search for code across all public repositories

https://github.com/search?l=javascript&o=desc&p=1&q=componentWillMount%28%29+language%3AJavaScript&ref=advsearch&s=indexed&type=Code&utf8=%E2%9C%93

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Sep 7, 2016

Member

@ljharb So, it's basically lazy initialization of global state. We do a lot of lazy initialization in render methods, but since it is generally unobserved I don't consider that bad practice. The difference here is that it is not just lazy but dependent on props.

I guess you assume that props will never change and that you won't ever have two different subtrees with different phrases. Otherwise, people normally use context for this use case.

Given that there are already a lot of assumptions involved in this particular use case, maybe it's not that bad to just do it in the constructor as a hack?

How do you do this server-side? Do you assume that the context is only used for a single request at a time? No async rendering?

Member

sebmarkbage commented Sep 7, 2016

@ljharb So, it's basically lazy initialization of global state. We do a lot of lazy initialization in render methods, but since it is generally unobserved I don't consider that bad practice. The difference here is that it is not just lazy but dependent on props.

I guess you assume that props will never change and that you won't ever have two different subtrees with different phrases. Otherwise, people normally use context for this use case.

Given that there are already a lot of assumptions involved in this particular use case, maybe it's not that bad to just do it in the constructor as a hack?

How do you do this server-side? Do you assume that the context is only used for a single request at a time? No async rendering?

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Sep 7, 2016

Contributor

I'd very strongly prefer something besides the constructor. Hmm, I thought context was deprecated already :-p is it not?

Yes, that's right - whether it was async or not, it would be re-evaluated and reconstructed in a new vm for every request.

Contributor

ljharb commented Sep 7, 2016

I'd very strongly prefer something besides the constructor. Hmm, I thought context was deprecated already :-p is it not?

Yes, that's right - whether it was async or not, it would be re-evaluated and reconstructed in a new vm for every request.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Sep 7, 2016

Contributor

If componentWillUpdate, or componentWillReceiveProps, or something similar, could be sure to run upon every new set of props, including the first/initial one, prior to the render - that would suffice for our use cases, I think, and would probably solve them more robustly than componentWillMount. Is adding a new lifecycle method out of the question?

Contributor

ljharb commented Sep 7, 2016

If componentWillUpdate, or componentWillReceiveProps, or something similar, could be sure to run upon every new set of props, including the first/initial one, prior to the render - that would suffice for our use cases, I think, and would probably solve them more robustly than componentWillMount. Is adding a new lifecycle method out of the question?

@brigand

This comment has been minimized.

Show comment
Hide comment
@brigand

brigand Sep 7, 2016

Contributor

Adding a consistent lifecycle method to handle props would be super useful in many cases. The most common being "fetch the x for this.props.xId". The benefit of running before render is that you can show a loading indicator without requiring a second render. The problem is that for some use cases you'd want it to run on the server, like @ljharb's, but for the fetching data you'd want that on only client in most cases.

Edit: maybe this should branch out to another issue.

Contributor

brigand commented Sep 7, 2016

Adding a consistent lifecycle method to handle props would be super useful in many cases. The most common being "fetch the x for this.props.xId". The benefit of running before render is that you can show a loading indicator without requiring a second render. The problem is that for some use cases you'd want it to run on the server, like @ljharb's, but for the fetching data you'd want that on only client in most cases.

Edit: maybe this should branch out to another issue.

@leebyron

This comment has been minimized.

Show comment
Hide comment
@leebyron

leebyron Sep 7, 2016

Contributor

Is there ever a time (in the browser context) where componentWillMount is not called immediately after the constructor?

As far as I can remember, componentWillMount is effectively a constructor function. It served that purpose before ES6 classes were adopted that came with their own constructors. In fact, in much earlier versions of React, componentWillMount was always called on construction IIRC which made server rendering painful. So we changed componentWillMount to be called only in the browser render context, and not in the server render context. (Then getInitialState was the effective constructor)

All the uses I've seen for componentWillMount are just constructors which you expect to only run in the browser. If componentDidMount isn't the right place to move that code (due to double render) then moving it into the class constructor with a typeof window if check might be adequate.

I don't see the reason to be concerned about doing side effects in a React component constructor if you're not concerned about doing the same within componentWillMount which is part of component construction. Since user code never instantiates components directly (library code does), the typical reasons to avoid side effect constructors doesn't apply for React components.

Sebastian, maybe another option is to slightly change the behavior of componentWillMount rather than remove it by making it more explicitly part of construction just to avoid user code writing that browser check and preserve the majority of existing code? Would that be possible within Fiber?

Contributor

leebyron commented Sep 7, 2016

Is there ever a time (in the browser context) where componentWillMount is not called immediately after the constructor?

As far as I can remember, componentWillMount is effectively a constructor function. It served that purpose before ES6 classes were adopted that came with their own constructors. In fact, in much earlier versions of React, componentWillMount was always called on construction IIRC which made server rendering painful. So we changed componentWillMount to be called only in the browser render context, and not in the server render context. (Then getInitialState was the effective constructor)

All the uses I've seen for componentWillMount are just constructors which you expect to only run in the browser. If componentDidMount isn't the right place to move that code (due to double render) then moving it into the class constructor with a typeof window if check might be adequate.

I don't see the reason to be concerned about doing side effects in a React component constructor if you're not concerned about doing the same within componentWillMount which is part of component construction. Since user code never instantiates components directly (library code does), the typical reasons to avoid side effect constructors doesn't apply for React components.

Sebastian, maybe another option is to slightly change the behavior of componentWillMount rather than remove it by making it more explicitly part of construction just to avoid user code writing that browser check and preserve the majority of existing code? Would that be possible within Fiber?

n1k0 added a commit to Kinto/kinto-admin that referenced this issue Sep 7, 2016

@jpdesigndev

This comment has been minimized.

Show comment
Hide comment
@jpdesigndev

jpdesigndev Aug 24, 2017

My use case for componentWillMount():

class Example extends React.PureComponent {
  componentWillMount() {
    const { fetchAction, fetchStatus, dispatch } = this.props;
    if (fetchStatus !== FETCH_STATUSES.LOADING && fetchStatus !== FETCH_STATUSES.LOADED) {
      dispatch(fetchAction);
    }
  }

  render() { ... }
}

If another component renders multiple Example components, dispatch(fetchAction) only happens once as expected. If the Example component utilizes componentDidMount(), dispatch(fetchAction) is called twice. Perhaps there is a better way?

I apologize if this has already been covered. I skimmed comments that didn't seem completely relevant.

jpdesigndev commented Aug 24, 2017

My use case for componentWillMount():

class Example extends React.PureComponent {
  componentWillMount() {
    const { fetchAction, fetchStatus, dispatch } = this.props;
    if (fetchStatus !== FETCH_STATUSES.LOADING && fetchStatus !== FETCH_STATUSES.LOADED) {
      dispatch(fetchAction);
    }
  }

  render() { ... }
}

If another component renders multiple Example components, dispatch(fetchAction) only happens once as expected. If the Example component utilizes componentDidMount(), dispatch(fetchAction) is called twice. Perhaps there is a better way?

I apologize if this has already been covered. I skimmed comments that didn't seem completely relevant.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 24, 2017

Contributor

@jpdesigndev i'm not sure why it would be; componentDidMount is only ever called once just like componentWillMount?

Contributor

ljharb commented Aug 24, 2017

@jpdesigndev i'm not sure why it would be; componentDidMount is only ever called once just like componentWillMount?

@jpdesigndev

This comment has been minimized.

Show comment
Hide comment
@jpdesigndev

jpdesigndev Aug 24, 2017

Sorry, @ljharb, I wasn't clear enough.

const SomeParent = (props) =>
  <div>
    <Example />
    <Example />
  </div>

If SomeParent component renders the Example component, twice (real use case, I apologize for the generic examples), componentDidMount() will result to dispatching the actions twice, while componentWillMount() results in the action being dispatched only once (expected). I'm certain there is a perfectly reasonable explanation for this (sync/async, or some other timing reason), but this is why I prefer to do this conditional data fetching in componentWillMount() for the time being. Perhaps this is an anti-pattern? Perhaps someone will suggest that I move data fetching up to the parent (this would break a nice encapsulation)?


EDIT: for clarity (hopefully)

class Example extends React.PureComponent {
  componentWillMount() {
    const { fetchAction, fetchStatus, dispatch } = this.props;
    //
    // Notice the conditional here.
    // If this code were in a componentDidMount(), the fetchStatus would be null
    // for both <Example /> renders by <SomeParent />
    //
    // By utilizing componentWillMount()
    // The second componentWillMount() call has the new fetchStatus
    // that was updated synchronously by the dispatched action from the first <Example />
    // rendered by <SomeParent />
    if (fetchStatus !== FETCH_STATUSES.LOADING && fetchStatus !== FETCH_STATUSES.LOADED) {
      dispatch(fetchAction);
    }
  }

  render() { ... }
}

const SomeParent = (props) =>
  <div>
    <Example />
    <Example />
  </div>

jpdesigndev commented Aug 24, 2017

Sorry, @ljharb, I wasn't clear enough.

const SomeParent = (props) =>
  <div>
    <Example />
    <Example />
  </div>

If SomeParent component renders the Example component, twice (real use case, I apologize for the generic examples), componentDidMount() will result to dispatching the actions twice, while componentWillMount() results in the action being dispatched only once (expected). I'm certain there is a perfectly reasonable explanation for this (sync/async, or some other timing reason), but this is why I prefer to do this conditional data fetching in componentWillMount() for the time being. Perhaps this is an anti-pattern? Perhaps someone will suggest that I move data fetching up to the parent (this would break a nice encapsulation)?


EDIT: for clarity (hopefully)

class Example extends React.PureComponent {
  componentWillMount() {
    const { fetchAction, fetchStatus, dispatch } = this.props;
    //
    // Notice the conditional here.
    // If this code were in a componentDidMount(), the fetchStatus would be null
    // for both <Example /> renders by <SomeParent />
    //
    // By utilizing componentWillMount()
    // The second componentWillMount() call has the new fetchStatus
    // that was updated synchronously by the dispatched action from the first <Example />
    // rendered by <SomeParent />
    if (fetchStatus !== FETCH_STATUSES.LOADING && fetchStatus !== FETCH_STATUSES.LOADED) {
      dispatch(fetchAction);
    }
  }

  render() { ... }
}

const SomeParent = (props) =>
  <div>
    <Example />
    <Example />
  </div>
@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Aug 24, 2017

Contributor

That makes no sense to me - two elements should invoke two WillMounts.

Contributor

ljharb commented Aug 24, 2017

That makes no sense to me - two elements should invoke two WillMounts.

@sompylasar

This comment has been minimized.

Show comment
Hide comment
@sompylasar

sompylasar Aug 24, 2017

Contributor

@jpdesigndev I would suggest to move data fetching away from components, keeping only data fetching request dispatching in them. See redux-saga.

Contributor

sompylasar commented Aug 24, 2017

@jpdesigndev I would suggest to move data fetching away from components, keeping only data fetching request dispatching in them. See redux-saga.

@sompylasar

This comment has been minimized.

Show comment
Hide comment
@sompylasar

sompylasar Aug 24, 2017

Contributor

@jpdesigndev Sorry I meant the logic for checking if the fetch is in progress and ignoring the request.

Contributor

sompylasar commented Aug 24, 2017

@jpdesigndev Sorry I meant the logic for checking if the fetch is in progress and ignoring the request.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Aug 24, 2017

Member

@jpdesigndev In your example, the difference is that in componentWillMount, this.props points to the next props that haven't rendered yet. Whereas in componentDidMount, they point to the props that just rendered. This is because didMount fires during React's "commit phase." When you call setState inside didMount, React doesn't apply the update until the end of the current commit phase. So even though you've fired dispatch in the first didMount, that update hasn't been applied yet by the time you call the second didMount.

One solution would be read the FETCH_STATUS directly from the store, since that is mutated synchronously upon dispatch. Rather than rely on React props/state, which are updated asynchronously.

Member

acdlite commented Aug 24, 2017

@jpdesigndev In your example, the difference is that in componentWillMount, this.props points to the next props that haven't rendered yet. Whereas in componentDidMount, they point to the props that just rendered. This is because didMount fires during React's "commit phase." When you call setState inside didMount, React doesn't apply the update until the end of the current commit phase. So even though you've fired dispatch in the first didMount, that update hasn't been applied yet by the time you call the second didMount.

One solution would be read the FETCH_STATUS directly from the store, since that is mutated synchronously upon dispatch. Rather than rely on React props/state, which are updated asynchronously.

@jpdesigndev

This comment has been minimized.

Show comment
Hide comment
@jpdesigndev

jpdesigndev Aug 24, 2017

@ljharb @sompylasar @acdlite Thank you all for the thoughts/recommendations!

@sompylasar Leaving this logic at the component level can, sometimes, make sense. For example, if the component really should be in control of fetch/re-fetch. Without some force param on the action creator, it's simple enough to just conditionally call fetch in the component instead of the Action Creator / saga / observable (epic) controlling when to fetch.

@acdlite Thank you for the explanation. That really helps clarify my thoughts on why *WillMount works better here vs. *DidMount. I'm not sure I love/understand the idea of reading from the store directly in this case. In fact, this is a connected component that utilizes selectors to get at the fetchStatus from props.

I'm really just chiming in here to lay out a use case (which seems valid) for componentWillMount(). I'm still not convinced there is a better way.

Thus far the proposed better/alternative ways are:

  1. Delegate control to Action Creators or Side Effect middleware
  2. Read fetchStatus directly from the store instead of relying on props

I'm perfectly willing to change my mind on this. Perhaps, I will be forced to do so if componentWillMount() is deprecated. Do folks agree that either of these approaches are more desirable than my componentWillMount() example?

Also, have I missed the point entirely? Should I already be devising a way to migrate away from componentWillMount()?

jpdesigndev commented Aug 24, 2017

@ljharb @sompylasar @acdlite Thank you all for the thoughts/recommendations!

@sompylasar Leaving this logic at the component level can, sometimes, make sense. For example, if the component really should be in control of fetch/re-fetch. Without some force param on the action creator, it's simple enough to just conditionally call fetch in the component instead of the Action Creator / saga / observable (epic) controlling when to fetch.

@acdlite Thank you for the explanation. That really helps clarify my thoughts on why *WillMount works better here vs. *DidMount. I'm not sure I love/understand the idea of reading from the store directly in this case. In fact, this is a connected component that utilizes selectors to get at the fetchStatus from props.

I'm really just chiming in here to lay out a use case (which seems valid) for componentWillMount(). I'm still not convinced there is a better way.

Thus far the proposed better/alternative ways are:

  1. Delegate control to Action Creators or Side Effect middleware
  2. Read fetchStatus directly from the store instead of relying on props

I'm perfectly willing to change my mind on this. Perhaps, I will be forced to do so if componentWillMount() is deprecated. Do folks agree that either of these approaches are more desirable than my componentWillMount() example?

Also, have I missed the point entirely? Should I already be devising a way to migrate away from componentWillMount()?

@sompylasar

This comment has been minimized.

Show comment
Hide comment
@sompylasar

sompylasar Aug 24, 2017

Contributor

@sompylasar Leaving this logic at the component level can, sometimes, make sense. For example, if the component really should be in control of fetch/re-fetch. Without some force param on the action creator, it's simple enough to just conditionally call fetch in the component instead of the Action Creator / saga / observable (epic) controlling when to fetch.

@jpdesigndev Then you have two actions: "fetch" (that leads to auto-cache and no re-fetch) and "refetch" (that leads to resetting the cache, stopping all previous requests, and re-fetching).

Contributor

sompylasar commented Aug 24, 2017

@sompylasar Leaving this logic at the component level can, sometimes, make sense. For example, if the component really should be in control of fetch/re-fetch. Without some force param on the action creator, it's simple enough to just conditionally call fetch in the component instead of the Action Creator / saga / observable (epic) controlling when to fetch.

@jpdesigndev Then you have two actions: "fetch" (that leads to auto-cache and no re-fetch) and "refetch" (that leads to resetting the cache, stopping all previous requests, and re-fetching).

@bvaughn

This comment has been minimized.

Show comment
Hide comment
@bvaughn

bvaughn Dec 11, 2017

Contributor

componentDidServerRender now has an RFC reactjs/rfcs/pull/8 😄

Contributor

bvaughn commented Dec 11, 2017

componentDidServerRender now has an RFC reactjs/rfcs/pull/8 😄

@NE-SmallTown

This comment has been minimized.

Show comment
Hide comment
@NE-SmallTown

NE-SmallTown Feb 9, 2018

Contributor

@sebmarkbage

The purpose would be to highly discourage side-effects, because they have unforeseen consequences, even though you technically could do them in the constructor.

Could you give more details about this? Is there any edge case?

Contributor

NE-SmallTown commented Feb 9, 2018

@sebmarkbage

The purpose would be to highly discourage side-effects, because they have unforeseen consequences, even though you technically could do them in the constructor.

Could you give more details about this? Is there any edge case?

@TrySound

This comment has been minimized.

Show comment
Hide comment
@TrySound

TrySound Feb 9, 2018

Contributor

@NE-SmallTown Take a look at link above

Contributor

TrySound commented Feb 9, 2018

@NE-SmallTown Take a look at link above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment