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

Set state takes a function #2991

Merged
merged 1 commit into from Feb 4, 2015

Conversation

Projects
None yet
7 participants
@leebyron
Contributor

leebyron commented Jan 31, 2015

This diff enables setState to accept a function in addition to a state partial. If you provide a function, it will be called with the up-to-date state, props, context as arguments.

This enables some nicer syntax for complex setState patterns:

If setState is doing an increment and wants to guarantee atomicy, you need a function:

this.setState(state => ({ number: state.number + 1 }));

This atomicy is particularly important if setState is called multiple times in a single frame of execution as the result of complex user actions. It's a tricky bug to chase down and difficult to determine how to fix when you find it. The current pattern of reaching into _pendingState relies on an implementation detail.

In this example: props.doAction() may result in your ancestor re-rendering and providing you with new props or context. If setState is called directly with an object literal referencing this.props or this.context, it will use the old version of those values, not the new value. Using a function solves for this case:

this.props.doAction();
this.setState((state, props) => ({ number: state.number * props.multiplier }));
@leebyron

This comment has been minimized.

Show comment
Hide comment
@leebyron

leebyron Jan 31, 2015

Contributor

Note: ReactCompositeComponentState-test will fail now because it relies on _pendingState to make assertions.

I'm not entirely certain how to fix because I'm not entirely certain what that test is actually testing.

Contributor

leebyron commented Jan 31, 2015

Note: ReactCompositeComponentState-test will fail now because it relies on _pendingState to make assertions.

I'm not entirely certain how to fix because I'm not entirely certain what that test is actually testing.

@brigand

This comment has been minimized.

Show comment
Hide comment
@brigand

brigand Jan 31, 2015

Contributor

Also,

var update = React.addons.update;

// ...
  updateFooBar(value){
      this.setState(state => update(state, {foo: {bar: {$set: 5}}});
  }

And swap that with your favorite immutable update method.

Contributor

brigand commented Jan 31, 2015

Also,

var update = React.addons.update;

// ...
  updateFooBar(value){
      this.setState(state => update(state, {foo: {bar: {$set: 5}}});
  }

And swap that with your favorite immutable update method.

@leebyron

This comment has been minimized.

Show comment
Hide comment
@leebyron

leebyron Jan 31, 2015

Contributor

@brigand not quite in this case. I'm proposing to keep the same semantics of setState(). I can enable that effect in a separate pull built on this same idea using replaceState().

The semantics of setState() are to return a partial state patch to merge in.

Contributor

leebyron commented Jan 31, 2015

@brigand not quite in this case. I'm proposing to keep the same semantics of setState(). I can enable that effect in a separate pull built on this same idea using replaceState().

The semantics of setState() are to return a partial state patch to merge in.

@leebyron

This comment has been minimized.

Show comment
Hide comment
@leebyron

leebyron Jan 31, 2015

Contributor

@brigand - however:

updateFooBar(value){
  this.setState(state => ({ fooBar: update(state.fooBar, {foo: {bar: {$set: 5}}}) });
}
Contributor

leebyron commented Jan 31, 2015

@brigand - however:

updateFooBar(value){
  this.setState(state => ({ fooBar: update(state.fooBar, {foo: {bar: {$set: 5}}}) });
}
@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jan 31, 2015

Member

lgtm. Just needs to fix the existing test and add tests of the new functionality.

Member

sebmarkbage commented Jan 31, 2015

lgtm. Just needs to fix the existing test and add tests of the new functionality.

@leebyron

This comment has been minimized.

Show comment
Hide comment
@leebyron

leebyron Jan 31, 2015

Contributor

Suggestion on how to approach the broken test? What conditions do we need to test there?


Sent from Mailbox

On Fri, Jan 30, 2015 at 8:30 PM, Sebastian Markbåge
notifications@github.com wrote:

lgtm. Just needs to fix the existing test and add tests of the new functionality.

Reply to this email directly or view it on GitHub:
#2991 (comment)

Contributor

leebyron commented Jan 31, 2015

Suggestion on how to approach the broken test? What conditions do we need to test there?


Sent from Mailbox

On Fri, Jan 30, 2015 at 8:30 PM, Sebastian Markbåge
notifications@github.com wrote:

lgtm. Just needs to fix the existing test and add tests of the new functionality.

Reply to this email directly or view it on GitHub:
#2991 (comment)

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jan 31, 2015

Member

I think that it is probably better to remove the piece from that test that introspects pending state. Then add another exhaustive test of this new setState-with-a-function. Then assert the sequencing of calling it in the various life cycles. As well as the sequencing of when the callbacks gets invoked. Including what this.state is at various life-cycles.

Member

sebmarkbage commented Jan 31, 2015

I think that it is probably better to remove the piece from that test that introspects pending state. Then add another exhaustive test of this new setState-with-a-function. Then assert the sequencing of calling it in the various life cycles. As well as the sequencing of when the callbacks gets invoked. Including what this.state is at various life-cycles.

@leebyron

This comment has been minimized.

Show comment
Hide comment
@leebyron

leebyron Jan 31, 2015

Contributor

Just updated this pull to fix up ReactCompositeComponentState-test. It no longer inspects internal information and in order to test the same lifecycle of pending state, I added more setState() calls and a replaceState() call to ensure they're called in the correct order.

I think this should also cover the usages of providing a function.

Contributor

leebyron commented Jan 31, 2015

Just updated this pull to fix up ReactCompositeComponentState-test. It no longer inspects internal information and in order to test the same lifecycle of pending state, I added more setState() calls and a replaceState() call to ensure they're called in the correct order.

I think this should also cover the usages of providing a function.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 2, 2015

Member

👍 Feel free to merge when you're comfortable.

Member

sebmarkbage commented Feb 2, 2015

👍 Feel free to merge when you're comfortable.

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Feb 2, 2015

Contributor

I'm curious, in which cases can props/context differ from the ones in this? Or are they just there for completeness (i.e. avoid accessing this, using non-method callbacks, etc).

Contributor

syranide commented Feb 2, 2015

I'm curious, in which cases can props/context differ from the ones in this? Or are they just there for completeness (i.e. avoid accessing this, using non-method callbacks, etc).

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Feb 2, 2015

Contributor

Nice, glad my API suggestion was picked up :)

Contributor

vjeux commented Feb 2, 2015

Nice, glad my API suggestion was picked up :)

@vsiao

This comment has been minimized.

Show comment
Hide comment
@vsiao

vsiao Feb 2, 2015

Contributor

Fixes #122, yeah?

Contributor

vsiao commented Feb 2, 2015

Fixes #122, yeah?

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 2, 2015

Member

@syranide If you receive props in the same batch as where you have a state update, then this gets executed before shouldComponentUpdate passing the new props and state, before the instance is update.

@leebyron Your unit test doesn't cover this scenario. ^ Might be worth adding.

Member

sebmarkbage commented Feb 2, 2015

@syranide If you receive props in the same batch as where you have a state update, then this gets executed before shouldComponentUpdate passing the new props and state, before the instance is update.

@leebyron Your unit test doesn't cover this scenario. ^ Might be worth adding.

@leebyron

This comment has been minimized.

Show comment
Hide comment
@leebyron

leebyron Feb 2, 2015

Contributor

Yes, let me add a unit test to illustrate that case before I merge

Contributor

leebyron commented Feb 2, 2015

Yes, let me add a unit test to illustrate that case before I merge

@andreypopp

This comment has been minimized.

Show comment
Hide comment
@andreypopp

andreypopp Feb 3, 2015

Contributor

Why not making this a separate method (for example updateState) rather than overloading setState?

Contributor

andreypopp commented Feb 3, 2015

Why not making this a separate method (for example updateState) rather than overloading setState?

@leebyron

This comment has been minimized.

Show comment
Hide comment
@leebyron

leebyron Feb 3, 2015

Contributor

I don't have a strong opinion. There are pros and cons to each. Smaller API surface area vs explicit type definition per method. Both are unambiguous. @sebmarkbage?

Contributor

leebyron commented Feb 3, 2015

I don't have a strong opinion. There are pros and cons to each. Smaller API surface area vs explicit type definition per method. Both are unambiguous. @sebmarkbage?

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Feb 3, 2015

Member

We originally explored something like enqueueTransaction which would allow the setState within the transaction to keep the pseudo-OO looking API. However, when we realized that we couldn't update this.state and this.props at the point in the life-cycle where this operation must occur, we decided to go with something more monadic (which is what we really wanted all along anyway).

We evaluated a bunch of options such as transact, enqueueStateUpdate and updateState. I thought updateState was too confusing. There is nothing in those names to indicate the difference between the two signatures, updateState and setState. The only way to tell them apart is by some memorized knowledge. I would definitely screw that up and put the wrong argument with the wrong name.

@vjeux suggested that we'd just overload setState and that solved the problem.

We're also planning on overloading the API to accept a Promise anyway so this seems like a good start.

That said, I never really liked setState since it is deceptive. It is really scheduling or enqueuing the next state merge. I think that whatever name it has, the overloaded API makes sense.

Member

sebmarkbage commented Feb 3, 2015

We originally explored something like enqueueTransaction which would allow the setState within the transaction to keep the pseudo-OO looking API. However, when we realized that we couldn't update this.state and this.props at the point in the life-cycle where this operation must occur, we decided to go with something more monadic (which is what we really wanted all along anyway).

We evaluated a bunch of options such as transact, enqueueStateUpdate and updateState. I thought updateState was too confusing. There is nothing in those names to indicate the difference between the two signatures, updateState and setState. The only way to tell them apart is by some memorized knowledge. I would definitely screw that up and put the wrong argument with the wrong name.

@vjeux suggested that we'd just overload setState and that solved the problem.

We're also planning on overloading the API to accept a Promise anyway so this seems like a good start.

That said, I never really liked setState since it is deceptive. It is really scheduling or enqueuing the next state merge. I think that whatever name it has, the overloaded API makes sense.

Set state takes a function
This diff enables setState to accept a function in addition to a state partial. If you provide a function, it will be called with the up-to-date `state, props, context` as arguments.

This enables some nicer syntax for complex setState patterns:

If setState is doing an increment and wants to guarantee atomicy, you need a function:

```
this.setState(state => ({ number: state.number + 1 }));
```

This atomicy is particularly important if setState is called multiple times in a single frame of execution as the result of complex user actions. It's a tricky bug to chase down and difficult to determine how to fix when you find it. The current pattern of reaching into _pendingState relies on an implementation detail.

In this example: props.doAction() may result in your ancestor re-rendering and providing you with new props. If setState is called directly with an object literal referencing `this.props`, it will use the *old* version of props, not the new value. Using a function solves for this case:

```
this.props.doAction();
this.setState((state, props) => ({ number: state.number * props.multiplier }));
```
@leebyron

This comment has been minimized.

Show comment
Hide comment
@leebyron

leebyron Feb 4, 2015

Contributor

I just added a new test which illustrates the values of props and state when dealing with the parent child relationship that led us to this, @sebmarkbage.

Contributor

leebyron commented Feb 4, 2015

I just added a new test which illustrates the values of props and state when dealing with the parent child relationship that led us to this, @sebmarkbage.

leebyron added a commit that referenced this pull request Feb 4, 2015

@leebyron leebyron merged commit 9174501 into facebook:master Feb 4, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@leebyron leebyron deleted the leebyron:state-queue branch Feb 4, 2015

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