React: Ability to call setState in the constructor

[jimfb]

A design question raised in another bug: https://github.com/facebook/react/issues/5313#issuecomment-152668532

What is the ideal behavior? Why don't we support setState() in the constructor? It seems better than having people manually add this.state = {...} in the constructor, since the current method encourages the imperative pattern that can't be used elsewhere in the component. cc @sebmarkbage for an answer.

[jimfb]

Just talked with Sebastian, and we agreed that this is probably a pattern that we will never want to support. In most cases, calling setState in the constructor is an anti-pattern. The intended "story" for the future is to use property initializers once they land in ES7-ish, and then the this.state = {...} will go away.

I'll leave this issue open for another day or two in case people want to continue the discussion.

[sebmarkbage]

componentWillMount exists as an escape hatch.

[acdlite]

I agree that

class MyComponent extends React.Component {
  constructor() {
    this.setState(something); // wrong
  }
}

shouldn't be supported.

But https://github.com/facebook/react/issues/5313 is about a constructor which triggers setState() on a _different_ component besides the one being constructed. I believe there are valid use cases where this might happen, such as firing an action creator.

componentWillMount could provide a workaround, but it seems like the only reason this currently prints a warning is because the check interprets the existence of ReactCurrentOwner to mean that a render method is currently executing, which isn't strictly true.

The test I wrote for https://github.com/facebook/react/pull/5343 should illustrate what I mean: https://github.com/acdlite/react/blob/prevent-setState-warning-triggered-by-constructor/src/renderers/shared/reconciler/__tests__/ReactCompositeComponent-test.js#L404-L434

[jimfb]

Dan did file https://github.com/facebook/react/issues/5313, but his concern was more about the error message than about the legitimacy of calling setState() in the constructor. I wonder if @gaearon would consider it an antipattern (the code where he first discovered that the warning-message was wrong).

In general, it seems like a huge code smell if creating/rendering a component is having side effects on other components. I'm a little skeptical of the use case where you would want to fire an action creator within a constructor; do you have a specific use case or example in mind?

[acdlite]

Now that I think about it more, I can't think of any use case where the better solution isn't to move it to componentWillMount(). It does seem a little weird to me—if an external setState can be triggered inside componentWillMount(), why not from the constructor?—but I can see how conceptually those are two separate phases of the component lifecycle.

I'll go ahead and update https://github.com/facebook/react/pull/5343 to print a more helpful warning message. Thanks!

[sebmarkbage]

Even componentWillMount is an anti-pattern. Another scenario is callbacks to a parent in componentWillMount. Where it may reach into a ref before the ref is uninitialized. It causes all kinds of strange artifacts.

Better to just move it to didMount.

Side-effects that change state due to a component being rendered is even more of an anti-pattern and may break scheduling etc.

[acdlite]

I think there are valid edge cases where you want to react / fire a callback every time a component receives props. Since componentWillReceiveProps() does not get called on the initial render, to accomplish this you also need to fire the callback inside of componentWillMount(). (componentDidMount() won't do because it isn't called on the server.)

For example, I have a project called relay-sink that allows you to create a Relay container that fires a callback whenever it receives a new fragment, as a (temporary, in my team's case) way to pass Relay data into a Redux store: https://github.com/acdlite/relay-sink/blob/master/src/index.js#L16-L18

Another example is a project I have called react-rx-component which creates an Observable sequence of props updates: https://github.com/acdlite/react-rx-component/blob/master/src/createRxComponent.js#L30-L36 (Implementation could be improved, but it essentially works.)

I realize these are edge cases but they seem perfectly valid to me.

I can see how this causes problems when working with refs, but if you're working with refs, componentDidMount() is available to you because refs aren't relevant on the server, anyway.

[acdlite]

Though I could be wrong... maybe componentDidMount() is sufficient for both those cases.

[probablyup]

I've gotten around this by extending React.Component and implementing my own getInitialState() method. Perhaps the same thing could be done for the core ES6 implementation of React.Component?

[sebmarkbage]

@yaycmyk You can use componentWillMount. What else is different?

[probablyup]

@sebmarkbage From an ergonomics point of view, the getInitialState way seems better and retains continuity with how stuff is done on the ES5 side of things.

[sebmarkbage]

I'm confused. Are you calling setState from your getInitialState? That's not possible on the ES5 side of things.

[probablyup]

@sebmarkbage I implemented a getInitialState function that I call within the constructor of my extended React.Component to do this.state = this.getInitialState(). Basically emulating what happens in ES5 createClass

[basarat]

I implemented a getInitialState function that I call within the constructor of my extended React.Component to do this.state = this.getInitialState()

@yaycmyk seems like a pointless overhead over this.state = { /* the actual initial state */ } :rose:

[probablyup]

You're entitled to your opinion. I maintain a UI library used inside my company and people prefer the abstraction over having to do a constructor + super for such a simple need.

[basarat]

@yaycmyk I meant no offence :rose:

[jimfb]

Ok, this was mostly a discussion thread, since Sebastian and I are in agreement that setState within the constructor is probably not something we want to support. I'm going to close out the issue (as per https://github.com/facebook/react/issues/5342#issuecomment-152675459), but feel free to continue the discussion here and we can re-open if our thinking on this matter changes substantially.

[davidworkman9]

Just going to throw my use-case out there:

I'm using Meteor with React, and am trying to tie a Meteor data source to the state of a component. Meteor has a tracking system for it's data sources, you pass it a function and when any data sources you use in that function change, it re-runs the function. You get back an object that you can call stop on to tell it to stop listening. So in the componentWillUnmount I make a call to the stop method, and in the constructor I setup the function to run when changes are detected. It calls setState at the end of the function, which triggers this warning because Meteor will run the function the first time it's called. If I move this to the componentDidMount the error obviously goes away, as stated above.

Integrations into other third party state managers might have similar effects, now and in the future.

[binki]

Isn’t setState()’s automatic property merging much nicer than needing to do Object.assign()? If you have a multi-level class hierarchy where your base class is doing this.state = {x:true} and your sub class is doing this.state = {y:false} after calling super(), that’ll break the functionality of the base class, right? Why is the recommended pattern this.state = {} rather than this.state = Object.assign(this.state || {}, {}) ?