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

Hot reloading via react-transform/babel6 is not working when rendering a connected component from a non-connected component #224

Closed
bunkat opened this issue Dec 21, 2015 · 4 comments · Fixed by #225
Labels

Comments

@bunkat
Copy link

bunkat commented Dec 21, 2015

I think this is an issue with how react-redux handles the hot reloading of connected components. If you have a non-connected (dumb) component render a connected component as a wrapper component, any content within that wrapper will no longer be re-rendered on change.

_Connected component:_

class Form extends Component {
  constructor(props) {
    super(props);
  }

  render() {
    return (
      <form>
        {this.props.children}
      </form>
    );
  }
}

export default connect(...)(Form);

_Dumb component rendering connected component:_

class LoginForm extends Component {
  constructor(props, context) {
    super(props, context);
  }

  render() {
    return (
      <Form id="loginForm">
        this is a test
      </Form>
    );
  }
}

Changing the text this is a test will not cause the component to re-render. Replace <Form> with <div> and everything works as expected. Replace connect()(Form) with just Form and everything works as expected.

Repository demonstrating issue is available at https://github.com/bunkat/counter. It is the react-redux counter example with babel6 hot loading and a new connected wrapper component.

To reproduce the issue:

  1. git clone https://github.com/bunkat/counter.git
  2. cd counter
  3. npm install
  4. npm start
  5. Open browser to http://localhost:3000
  6. Edit components/Counter.js
  7. Modify the render method, change Clicked to Not Clicked

The module will be updated via HMR, but the Counter component will not be rendered with the new text.

@gaearon
Copy link
Contributor

gaearon commented Dec 21, 2015

So this is a bug. In particular it is caused by us recomputing child props inside shouldComponentUpdate() which is not at all guaranteed to run (for example, it won't run if the connect wrapper is forceUpdate()d which is exactly what react-transform-hmr does). This was introduced in #99.

@epeli Can you look at this? Ideally we shouldn't care if shouldComponentUpdate() is called or not. We definitely shouldn't rely on React doing it before every render. Maybe we should move the calculations back into componentWillReceiveProps and handleChange and solve #99 in some other way.

@gaearon
Copy link
Contributor

gaearon commented Dec 21, 2015

Note: this shouldn't affect anything other than hot reloading because normally forceUpdate() can only be called by component itself, and this is where the bug is. However we can't rely on React always calling shouldComponentUpdate() in the future in all cases so we better fix this before this breaks any real cases.

@gaearon
Copy link
Contributor

gaearon commented Dec 22, 2015

Fixed in 4.0.3, thank you again very much for reporting.
It involved an important rewrite and the lib just got better. ;-)

@bunkat
Copy link
Author

bunkat commented Dec 22, 2015

Confirmed this works on my project as well. Thanks for fixing this so quickly!

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

Successfully merging a pull request may close this issue.

2 participants