gaearon / react-hot-loader

Tweak React components in real time. (Deprecated: use Fast Refresh instead.)
http://gaearon.github.io/react-hot-loader/
MIT License
12.26k stars 801 forks source link

hmr + redux @connect() class decorator issue #1120

Open rickbrenn opened 5 years ago

rickbrenn commented 5 years ago

Description

When using babel's @babel/plugin-proposal-decorators with redux's connect HOC and you make changes to the file the app updates but displays the old code.

Here's the normal way which works as expected when you change the file:

class Test extends Component {
  render() {
    return <div>Change me</div>
  }
}

export default connect()(Test)

If you use the class decorator it no longer renders new code:

@connect()
export default class Test extends Component {
  render() {
    return <div>Change me</div>
  }
}

I've tested this with other HOC like react-router's withRouter and the class decorator works fine. I'm unsure if this issue has to do with babel, hmr, or maybe I'm missing something obvious. I also transpiled the code with babel cli manually and tested the code it produces but that works fine, which is weird.

Expected behavior

The file should hot reload successfully whether you're using the standard method or class decorator.

Actual behavior

When using redux connect as a class decorator old code gets rendered as you change the file.

Environment

React Hot Loader version: 4.3.12 Node: v8.12.0 npm: 6.4.1 @babel/core: 7.2.0 @babel/plugin-proposal-decorators: 7.2.0 Operating system: macOS Mojave 10.14.1 Browser and version: Chrome 70.0.3538.110

Reproducible Demo

Here's an example repo

theKashey commented 5 years ago

Sorry, was quite busy with a new release. Could you check your problem with version 4.6.0?

rickbrenn commented 5 years ago

No problem. I updated the test repo to 4.6.0 but unfortunately the issue persists.

Wedvich commented 5 years ago

I'm seeing the same issue with using connect as a decorator on 4.3.12, and the issue is still there after upgrading to 4.6.0. So I tried adding hot to my connected component too, but it still fails:

@connect()
class Thing extends Component { /**/ }
export default hot(module)(Thing);

So this seems to suggest it's an ordering problem of some sorts: calling hot with the connect'ed component as input fails. Using a helper function that flips the order works, even as a decorator:

const hotConnect = (...connectArgs) =>
  (component) =>
    connect(...connectArgs)(hot(module)(component));

@hotConnect()
export default class Thing extends Component { /**/ }

I'm a bit confused myself as to what's the best practice - if hot should be used on every connected component, or if it's sufficient to only apply it to my root component. When using connect as a decorator only the former works - however, when not using decorators, it works fine with the latter.

theKashey commented 5 years ago

You may have one hot for your top-level component, just don't use it in the same file you are creating redux store - put it one module below.

Wedvich commented 5 years ago

Allright, good to know!

Just for the record, using the new root/hot from 4.6.0 on my root component had no effect, and I still need my hotConnect workaround.

theKashey commented 5 years ago

I'll take a look on your case later today. Please don't fix it.

theKashey commented 5 years ago

This is React-Redux v6 issue.From our side it's tracked as #1001 The problem - React-Redux creates methods via factories to render your Component. That WrappedComponent is stored in closure and RHL has no power on it.

this.selectChildElement = makeChildElementSelector()

During the class update we consider updating of the factored method as unsafe, and NOT doing it.

Result - you are rendering the old WrappedComponent.

Solutions:

Whys:

@markerikson - could you take a look at the problem? I would really appreciate if Redux and RHL would be besties 🤝

markerikson commented 5 years ago

I've got other priorities at the moment, but if someone wants to investigate this further and file a PR, we can look at it.

theKashey commented 5 years ago

This would fix the problem for now - https://github.com/reduxjs/react-redux/pull/1137