From ad1181fde939e73c0f8995074a79fd304c6f43a3 Mon Sep 17 00:00:00 2001 From: Joep Date: Wed, 22 Jun 2016 17:03:11 +1200 Subject: [PATCH 1/3] Add a failing test for #51. This shows that currently calling `setState` in `onChange` prevents changing tabs. --- src/components/__tests__/Tabs-test.js | 30 +++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/components/__tests__/Tabs-test.js b/src/components/__tests__/Tabs-test.js index d7438e3b5c..3788e31f47 100644 --- a/src/components/__tests__/Tabs-test.js +++ b/src/components/__tests__/Tabs-test.js @@ -294,4 +294,34 @@ describe('react-tabs', () => { wrapper.childAt(0).childAt(2).simulate('click'); assertTabSelected(wrapper, 0); }); + + it('should switch tabs if setState is called within onSelect', () => { + class Wrap extends React.Component { + constructor(props, state) { + super(props, state); + + this.state = { + foo: 'foo', + }; + + this.handleSelect = this.handleSelect.bind(this); + } + + handleSelect() { + this.setState({ foo: 'bar' }); + } + + render() { + return createTabs({ onSelect: this.handleSelect }); + } + } + + const wrapper = mount(); + + wrapper.childAt(0).childAt(1).simulate('click'); + assertTabSelected(wrapper, 1); + + wrapper.childAt(0).childAt(2).simulate('click'); + assertTabSelected(wrapper, 2); + }); }); From 181a2386a8fc937467792283f7b6f79bc5b34ca9 Mon Sep 17 00:00:00 2001 From: Joep Date: Wed, 22 Jun 2016 17:25:23 +1200 Subject: [PATCH 2/3] Fix #51. # 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. --- src/components/Tabs.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/components/Tabs.js b/src/components/Tabs.js index 854247a605..7b3707aa0b 100644 --- a/src/components/Tabs.js +++ b/src/components/Tabs.js @@ -48,7 +48,7 @@ module.exports = React.createClass({ }, getInitialState() { - return this.copyPropsToState(this.props); + return this.copyPropsToState(this.props, this.state); }, getChildContext() { @@ -64,7 +64,10 @@ module.exports = React.createClass({ }, componentWillReceiveProps(newProps) { - this.setState(this.copyPropsToState(newProps)); + // Use a transactional update to prevent race conditions + // when reading the state in copyPropsToState + // See https://github.com/reactjs/react-tabs/issues/51 + this.setState(state => this.copyPropsToState(newProps, state)); }, setSelected(index, focus) { @@ -282,7 +285,7 @@ module.exports = React.createClass({ }, // This is an anti-pattern, so sue me - copyPropsToState(props) { + copyPropsToState(props, state) { let selectedIndex = props.selectedIndex; // If no selectedIndex prop was supplied, then try @@ -294,8 +297,8 @@ module.exports = React.createClass({ // Manual testing can be done using examples/focus // See 'should preserve selectedIndex when typing' in specs/Tabs.spec.js if (selectedIndex === -1) { - if (this.state && this.state.selectedIndex) { - selectedIndex = this.state.selectedIndex; + if (state && state.selectedIndex) { + selectedIndex = state.selectedIndex; } else { selectedIndex = 0; } From 6148f4af78894ceff7d0f777a385ed2601fab334 Mon Sep 17 00:00:00 2001 From: Joep Date: Wed, 22 Jun 2016 17:38:20 +1200 Subject: [PATCH 3/3] Add a `setState` call in the focus example. --- examples/focus/app.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/examples/focus/app.js b/examples/focus/app.js index 361145f14f..95085f23f9 100644 --- a/examples/focus/app.js +++ b/examples/focus/app.js @@ -3,8 +3,13 @@ import ReactDOM from 'react-dom'; import { Tab, Tabs, TabList, TabPanel } from '../../src/main'; const App = React.createClass({ - handleInputChange() { + getInitialState() { + return { inputValue: '' }; + }, + + handleInputChange(e) { this.forceUpdate(); + this.setState({ inputValue: e.target.value }); }, render() { @@ -23,6 +28,7 @@ const App = React.createClass({