Deprecate replaceState
sebmarkbage opened this issue ยท 30 comments
The use cases for replaceState seems to fit one of these patterns:
-
Let state temporarily be null until some data is fetched. Then reset it to null using replaceState at some later point.
-
Use an immutable-js Record to store your state. That way you can use it with cursors and convenience methods like "update" and batching.
-
Using immutable-js Map to store your data.
-
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.
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.
๐๐
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.
By mutating state we also have the timing issues that comes with mutation.
Oh right, yeah.
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 :).
- Use an immutable-js Record to store your state. That way you can use it with cursors and convenience methods like "update" and batching.
- 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.
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.
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
@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.
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.
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({})
?
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({})?
@sebmarkbage nope :) my only concern was the state sealing, we have easy enough workarounds for the few places we've used replaceState, and won't shed a tear over its removal
๐
@sebmarkbage quick question on what to do if migrating to ES6 classes and I'm already using Immutable maps...
Whether I use Map
or Record
, setState
breaks the immutable object. What's the alternative if we want to keep on using immutable states in our views?
Thanks :)
class View extends React.Component {
constructor(props) {
this.update = this.update.bind(this)
this.state = Immutable.Map()
}
update() {
this.setState(this.state.set('foo', 'bar'))
}
render() {
return (
<div>
<button onClick={this.update}>update!</button>
<div>this.state.get { !!this.state.get ? 'exists' : 'doesn\'t exist' }</div>
</div>
)
}
}
class View extends React.Component {
constructor(props) {
super(props);
this.update = this.update.bind(this)
this.state = new ViewState({
foo: 'bar'
})
}
update() {
this.setState(this.state.set('foo', 'baz'))
}
render() {
return (
<div>
<button onClick={this.update}>update!</button>
<div>this.state.get { !!this.state.get ? 'exists' : 'doesn\'t exist' }</div>
</div>
)
}
}
On the question above, I guess a more relevant one is: if my stores are holding an immutable state, do I really need to care about components that deal with that data to have their states being immutable too or is it ok to leave React to do what it does best and trust that because actions are triggering the renders through stores changing our data will always come fresh from a non-mutated source?
I have a component that controls a flow as the user is navigated between different screens. The surrounding component uses state to model a state machine transitioning between different flows. For this purpose replaceState is a perfect fit.
You can simulate replaceState in your component by using only a single state variable. Ie. always do: setState({mystate: {...}}); Then you effectively have replaceState again, except that now you're component can be updated in the future without a huge refactor (as per Sebsatian's original post).
In fact, you can even define a replaceState function on your component, and if you always use your replaceState function (as oppose to ever using setState), it behaves almost exactly the same as the original function did.
function replaceState(newState)
{
setState({myState: newState});
}
Our philosophy is to maintain a minimal API surface. This is especially important for public facing open source projects (like React), since we need to keep the learning curve low. Having the documentation littered with functions that we "continue to support, but recommend you don't use" is very suboptimal and confusing to people who don't eat-sleep-and-breath React. We move fast, try new things, and deprecate anything that doesn't stand the test of time.
A quick workaround i did was do a forceUpdate to trigger re-render:
onChange(state) {
this.forceUpdate();
}
We then work with the latest state by simply grabbing it from the store:
<Component {...Store.getState()}/>
Not having replaceState in ES6 class object has been very stressful. Can someone provide a quick example that demonstrates simply how you'd replace state with .replaceState
vs without it
?
Edit: Decided to refactor my state a bit to nest the stuff wated to replace as suggested by @jimfb
I can only think of something ugly...
class Parent extends React.Component {
render() {
return (
<Child
render = {this.render}
{...Store.getState()}
/>
)
}
}
class Child extends React.Component {
componentDidMount() {
Store.listen(this.onChange);
}
componentWillUnmount() {
Store.unlisten(this.onChange);
}
onChange = () => {
this.props.render();
}
render() {
return (
<div />
)
}
}
I am using Immutable.js
and would like to replace my state... I don't want to listen to changes in the parent component. Not sure how to go about it?
@clouddra I am not sure your solution works... doesn't it simply take the latest props
and state
?
@Yakka My solution is the same as yours except but I listen to store changes in the parent component.
This is probably two years too late, and despite using replaceState
during that time I've just noticed replaceState
wasn't in the docs and stumbled across this issue to deprecate it, so I'm adding my two cents to defend it's continued existence.
I use none of the four patterns describe by OP, yet I still use replaceState
. I've adopted immutable data structures as a practice. I don't use Immutable.js, instead I either use lenses from Ramda or React.addons.update
. These tools yield new objects with deep modifications without touching the original object. I have no need of setState
or its shallow merge capability, and happy was I that I could choose not to use it. I define all my state properties in getInitialState
, and I never add or drop properties, only modify values.
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.
I don't understand this claim. I've never experienced any issue when adding a new property to my state object that forced me into some "painful" refactoring exercise. Maybe it's because I'm not using Immutable.js and I work with plain old javascript objects (POJOs). Can someone provide a concrete example where adding another state value induces painful refactoring and how setState
magically solves this?
- Why is
replaceState
so substantial that it litters the API? - Does it really kill the API and docs to keep
replaceState
for those of us who find it more valuable/relevant to our needs thansetState
?
Reading through this issue disappointed me. The arguments against seem superficial or based on anecdotes that weren't elaborated upon. A minimal API is a worthy pursuit, but I don't subscribe to the notion that "there can be only one!" way. Two similar, yet different, functions can still be considered a minimal API with some give & take.
If there are technical arguments (e.g. replaceState
impacts optimizations in React), I'd like to read them in detail, please.
Ok, here's a technical argument against replaceState
.
replaceState
has one effective signature of replaceState : Object -> Void
, which means you give it an object, and that object becomes the new state of the component. While this works well with synchronous code, it fails miserably with asynchronous code. The end result is an asynchronous call to replaceState
may rollback some properties to an earlier state. This has bit me in the past.
replaceState
could handle synchronous and asynchronous code fine if it's argument was a function instead of an object. i.e. replaceState : (Object -> Object) -> Void
. This is similar to setState
's alternative form, and would ensure that out of order asynchronous execution of state modifications always modifies the latest state instead of carrying around stale state.
Changing the signature from the error-prone form to callback form is an API-breaking change, so unless a PR is welcome (which I doubt), replaceState
may as well die.
I have a use case for ReplaceState.
On my company's app, we are creating "custom forms" - that is, the component takes a order definition from a JSON request and creates various forms of different types. All of them create a value in the state to store the keys on changes.
So far so good.
The problem is that we're using ReactRouter with /customOrder/:orderNumber -- and the component is tied into CustomOrder. Moving from one order number to the next is a different page, but the state remains from the previous custom order.
As a workaround, I'm thinking about creating this function:
const initialState = {{some intitial state}}
this.reset = () => {
this.state = initialState;
this.forceUpdate();
}
But I'm p
@brianboyko Sounds like you should store the form state as an object in state instead. I.e. this.setState({formState: {fields}});
.
Moving from one order number to the next is a different page, but the state remains from the previous custom order.
This doesn't seem like preferable behavior. You should probably use the orderNumber to key the element.
This has been successfully deprecated for a long time now.
I'm not sure is it fit my case.
I initialize state such as
this.state = {
a: 1,
b: 2,
}
After some operation, I want my state
change to be
{
a: 4,
c: 5,
}
I delete the b
property. I have no idea to implement this by using setState
. Maybe this is a case for replaceState
.
Maybe this is a case for
replaceState
.
@tangshuang I think it'll be a weak case. You can achieve your goal with:
this.state = {
myState: {
a: 1,
b: 2,
},
}
After some operation, setState({ myState: { a: 4, c: 5 } })
gives...
this.state = {
myState: {
a: 4,
c: 5,
},
}
It's not perfect, but then your request for replaceState
is a want, not a necessity.
Maybe this is a case for
replaceState
.@tangshuang I think it'll be a weak case. You can achieve your goal with:
this.state = { myState: { a: 1, b: 2, }, }
After some operation,
setState({ myState: { a: 4, c: 5 } })
gives...this.state = { myState: { a: 4, c: 5, }, }
It's not perfect, but then your request for
replaceState
is a want, not a necessity.
Why should I give a deep single property? Not intuitive.
Why should I give a deep single property?
I'm not a developer on the React team, so I ultimately decide nothing here, and I'm not sure if this remark is meant to be petulant or not.
The short answer is, "It's a compromise". The long answer requires knowing the reasons the React team decided to drop replaceState
, of which this was asked for from the React team, and never provided by the React team. I provided one technical reason above long ago why replaceState
was problematic and could corrupt state.
Not intuitive [to you]
But it was intuitive to me.
You want to replace your state object with a new object that omits properties. React no longer lets you replace the core state object. React owns this object, not you. React allows you to replace values of the properties in the core state object. So my solution to you leverages that ability to replace properties with new values to achieve your desired goal.
If you don't like it, then just assign undefined
to property b
and let the new state be:
{ a: 4,
b: undefined,
c: 5,
}
Also, I'm fairly certain javascript engines optimize faster functions when the arguments that are objects retain a consistent set of properties, so you're probably better off not deleting properties from objects.
@TanaseHagi @boxofrox AFAIK, the reason can be summarized as; state should have a predefined structure and not be any random object/properties. So if your component can have a+b+c as properties in state, then these should always be present (whether they are undefined/null is irrelevant as long as they are present).
I would expect the reason for this to be two-fold; best practice and performance.