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

Allow setState in onSelect (fixes #51) #110

Merged
merged 3 commits into from
Jun 23, 2016

Conversation

joepvl
Copy link
Collaborator

@joepvl joepvl commented Jun 22, 2016

Post my comment in #51, I investigated this issue some more and concluded that the problem was in making a decision based on reading from outdated local state. I'll repeat what I wrote in my commit message here:

What?

This changes the copyPropsToState method to take a second argument (state), enabling using it when feeding setState a callback so we can do a transactional update in componentWillReceiveProps.

Why?

This is needed because previously we were hitting a race condition whenever the parent component was triggering a rerender when using setState in its onSelect handler: componentWillReceiveProps would be triggered, calling copyPropsToState, which in turn read from this.state synchronously before the state changes triggered in Tabs's own setSelected method had been committed to the state.

This PR seems to fix the problem, but there are some things I'm unsure about and would love some input on (cc @danez):

  • the test I added works, but I'm not sure it's the cleanest, most idiomatic-enzyme-way of doing things. I've just created a full component inline but I think there may be a better way to mock one.
  • I also added a setState call to the focus example to illustrate that it works, but I didn't document that and tbh am not sure how/where I'd do that. I also suspect the this.forceUpdate() call that's in there was to make sure an update was triggered when something was typed into the input, but since I've got the setState call in there now an update will be triggered anyway, so we can probably remove the forceUpdate.
  • the inline documentation I added in Tabs#componentWillReceiveProps may not be clear enough

This shows that currently calling `setState` in `onChange`
prevents changing tabs.
# What?
This changes the `copyPropsToState` method to take a second argument (`state`), enabling using it when feeding `setState` a callback so we can do a transactional update in `componentWillReceiveProps`.

# Why?
This is needed because previously we were hitting a race condition whenever the parent component was triggering a rerender when using `setState` in its `onSelect` handler: `componentWillReceiveProps` would be triggered, calling `copyPropsToState`, which in turn read from `this.state` synchronously before the state changes triggered in `Tabs`'s own `setSelected` method had been committed to the state.
@danez
Copy link
Collaborator

danez commented Jun 23, 2016

Thanks. That looks good 👍

@danez danez merged commit 213e243 into reactjs:master Jun 23, 2016
@danez
Copy link
Collaborator

danez commented Jun 23, 2016

The only thing I changed is making the mock class more compact. We don't have a dedicated folder for mocks (yet), so it should be fine inline.

@joepvl
Copy link
Collaborator Author

joepvl commented Jun 23, 2016

Ah yes, I knew I was missing something obvious in that mock :). Great! 👍

@joepvl joepvl deleted the allow-setState-in-onSelect branch June 23, 2016 13:27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants