Allow setState in onSelect (fixes #51) (#110)

* Add a failing test for #51.
This shows that currently calling `setState` in `onChange`
prevents changing tabs.

* 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.

* Add a `setState` call in the focus example.

Author: joepvl

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({
             <input
               type="text"
               onChange={this.handleInputChange}
+              value={this.state.inputValue}
             />
           </TabPanel>
         </Tabs>

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;
       }

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(<Wrap />);
+
+    wrapper.childAt(0).childAt(1).simulate('click');
+    assertTabSelected(wrapper, 1);
+
+    wrapper.childAt(0).childAt(2).simulate('click');
+    assertTabSelected(wrapper, 2);
+  });
 });