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

Keep or Drop replaceState? #2843

Closed
sebmarkbage opened this issue Jan 12, 2015 · 7 comments
Closed

Keep or Drop replaceState? #2843

sebmarkbage opened this issue Jan 12, 2015 · 7 comments

Comments

@sebmarkbage
Copy link
Collaborator

This is continuing the conversation started in #2805

We're considering dropping replaceState because ideally state should always keep a consistent signature, defined by getInitialState.

@bowd
Copy link

bowd commented Jan 12, 2015

On the immutability note, I've started using fluxy which uses mori internally for store states and I've started using object subtrees of the store as state in components (like a cursor) and the component listens to changes in the store under it's cursor's key array scope. Then we get O(1) diffs in shouldComponentUpdate; when the store changes and we just replace the state with the new "cursor".

It's working pretty well and I'd hate to lose this :). But at the same time if you're planning on supporting something like Immutable.Record it might be cool (fluxy is working on abstracting the immutable data structure implementation to support immutable.js). But to be honest tying React into a specific implementation of immutable data kinda defeats the purpose when it comes to modularity and so on. Maybe if there are some hooks that allow you to plugin what you use wether it is immutable.js or mori.

@chenglou
Copy link
Contributor

A bit out of loop but, if state is {a: 1, b: 2}, is setting it (merging it) {b: 3} still gonna work? I can see it being convenient for a long time to come, but if we go immutable, I'd rather return the whole state object: setState(state.updateIn(path, val)). Avoids a merge and uses structural sharing as much as possible.

I'm against whatever custom merge mechanism mentioned in the other issue though. Don't think it's needed with the above solution.

Edit: if you wanna type check setState well, you do want it to accept the full obj/record, right?

@brigand
Copy link
Contributor

brigand commented Jan 13, 2015

The main place I use/see use of replaceState is when you've already done the merge, and while setState would work the same, it's also inefficient.

// getInitialState: () => ({a: {b: 0}}})
incr = (o) => updates(o, {a: {b: {$apply: (x) => x + 1}}});
var state = incr(this.state);
state = incr(state);
this.replaceState(state, (state) => state.a.b === 2);

This pattern tends to arise from react's inability to queue immutable updates.

this.setState({a: incr(this.state.a)});

// setState is async, so this would throw out the above changes
this.setState({a: incr(this.state.a)})

// when doing async things, the final state depends on batching strategy, and unless
// batching strategy is nextTick, race conditions
asyncThing(() => {
    this.setState({a: incr(this.state.a)}, (state) => state.a.b === 1 || state.a.b === 2)
});

// vs a hypothetical safe variant
this.enqueState({a: incr});
this.enqueState({a: incr});
asyncThing(() => {
    this.enqueState({a: incr}, (state) => state.a.b === 3);
});

@syranide
Copy link
Contributor

We're considering dropping replaceState because ideally state should always keep a consistent signature, defined by getInitialState.

I've always loved that idea, but I don't think it negates the value of replaceState. However, requiring calls to replaceState to provide the full state would surely not make it very practical... so it's not entirely obvious how you would keep useful without ruining a part of its appeal...

@brigand I feel quite confident that "inefficient" in this case is not really measurable in any real life scenario. But I could be wrong.

@sebmarkbage
Copy link
Collaborator Author

Presumably an engine with hidden classes can do an optimization for Object.assign(a, b, c);

if a is fresh empty object
  and hiddenClassOf(b) === hiddenClassOf(c)
    then memcpy(a, c)

this.replaceState with a mutable object may be dangerous so I think we'd ideally clone and freeze any such objects anyway. We already do this with props in createElement.

@sophiebits
Copy link
Collaborator

Closing in favor of #2843, I guess.

@sophiebits
Copy link
Collaborator

Er, #3236.

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

No branches or pull requests

6 participants