Deprecate replaceState

[sebmarkbage]

The use cases for replaceState seems to fit one of these patterns:

1. Let state temporarily be null until some data is fetched. Then reset it to null using replaceState at some later point.

2. Use an immutable-js Record to store your state. That way you can use it with cursors and convenience methods like "update" and batching.

3. Using immutable-js Map to store your data.

4. Use state as a map. I.e. adding new keys dynamically. setState({ [someID]: value })

All these use cases break down when you need to add another state value which makes refactoring a pain when you thought that you wouldn't need to.

It also means that mixins can't add state. (I'm not particularly found of this pattern but it is a common one.)

We could potentially add native support for (2) but it would expand the record type to one that contains all the fields required by mixins.

We would like to add a warning if you expand the state with fields that are not in the original getInitialState. The reason is that VMs optimize around records that keep their type signature. By changing the type signature dynamically you introduce performance penalties. We can't add that warning if patterns 3-4 is used.

[syranide]

It's fairly safe to say that (1) is equivalent to if (this.state) { this.state = null; this.forceUpdate(); } though, it's not very neat.

Idea; deprecate setState() as well in-favor of this.enqueueUpdate(void), i.e. forceUpdate without force. You're now responsible for updating this.state, we make no assumptions, you can easily re-add setState to your base class.

Tangent; technically we could even move to something like React.forceUpdate/enqueueUpdate(instance) if we want to avoid publicly exposing methods on the base component class. Only isMounted would be left then right? It even seems imaginable to drop the base component class entirely if you wanted to?

It also means that mixins can't add state. (I'm not particularly found of this pattern but it is a common one.)

* shoulder shrug *, your class, your choice. If you don't use an object you also accept that it won't be compatible with "standard mixins" out there.

We would like to add a warning if you expand the state with fields that are not in the original getInitialState.

👍👍

[sebmarkbage]

I would like to move the authoritative state object to the internals. Ideally it should also be frozen. At render we would resurface whatever the internals thinks is the current state object. That way the internals could be implemented in terms of immutable trees/cursors. The component structure could also be quickly hydrated and restored if we need to reclaim some memory (e.g. infinite scrolling lists). Ideally this.state = null and even this.state.foo = null wouldn't work. Nested objects would still work.

By mutating state we also have the timing issues that comes with mutation.

[syranide]

By mutating state we also have the timing issues that comes with mutation.

Oh right, yeah.

[jimfb]

This effectively means that all stateful components must define getInitialState. My knee-jerk reaction was that this is a bad thing, but now I'm starting to like it :).

[Zenwolf]

2. Use an immutable-js Record to store your state. That way you can use it with cursors and convenience methods like "update" and batching.
3. Using immutable-js Map to store your data.

I don't maintain much state inside components(I mostly maintain global state in Immutable.Record objects inside data stores), but when I do have component state it is always an Immutable.Map or Immutable.Record, and I replace the entire state after changes using replaceState(). I never allow mixins to change state and I am not currently using cursors. Hopefully there will be a support for this same kind of immutable state going forward.

[andersekdahl]

I wouldn't mind removing replaceState, but it would be great if it was possible to override the state merging. That would allow you to use immutable objects as state without having to keep it in a property in a regular object.

It would also allow you to detect when a mixin tries to use a regular object as state, and merge that with your immutable object.

[jquense]

Warning on changing the state signature feels like an overreach to me, if I want to suffer tiny performance hits for the convenience of not having to initialize false boolean values, I feel like I should be able to do that with being chastised by the compiler :P or have to suffer noise in my console for it

[jimfb]

@jquense There are other advantages to requiring a complete getInitialState(). Readability is the most obvious advantage, because you can always see (in one place) what state variables are being used. Another minor advantage is that it forces component developers to explicitly think through their component state (and consider if state is really necessary, since ideally most components should be stateless and this nudges developers in that direction). Much more importantly, forcing getInitialState() means we can identify stateful components, which allows for performance optimizations across your entire React app. It's not just about the minor cost of a V8 optimization surrounding statically-keyed objects; it's potentially about a framework-level optimization. So saving a little convenience for one person could mean a lot of performance loss for a lot of people.

[jquense]

I was reading here that the existence of getInitialState() is enough for optimization, and I can be on board with that no problem. The issue for me is sealing my state object. We frequently employ this sort of pattern:

getInitialState(){
  return {}
}

render(){
  return { this.state.open ? <div/> : <span/> }
}

I think that React requiring that I explicitly initialize open: false borders on stylistic prescription, since isn't invalid javascript. I agree that in some cases there is improved readability with explicit declaration of state, but I also think that often yet another multiline object literal in the middle of my constructor, or component definition reduces readability. The Readability argument, is ultimately a fruitless one given how notoriously subjective "readibility" is in CS. I'd much rather my team, and not React, make those decisions, if at all possible. This is all of course baring some excellent compiler optimization that can't be done otherwise.

[sebmarkbage]

I agree with @jquense. React should not be prescribing style guides or subjective readability/documentation rules. That's one of the nice features of the new class support in React. We don't really care how you create your classes as long as they correspond to some interface.

The only time we would introduce something prescriptive is if the mere existence of a convenience means that is blocks features for everyone that doesn't use it.

I think that Flow already do and will continue to require you to define the full state signature at construction type. For static typing that makes sense that you want stable type signatures. That also unlocks optimizations in VMs.

I think that is enough for us. We probably don't need to add a runtime warning.

I don't think we have any React-specific optimizations that would require state to be consistent, however, I could see that becoming the case in the future. So I'd still strongly recommend using a consistent state.

Regardless, I think it safe to remove replaceState. @jquense are you arguing that your pattern also requires you to reset state by doing this.replaceState({})?