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

withStyles throws "React Hot Loader: this component is not accepted by Hot Loader." #8821

Closed
1 task done
Glavin001 opened this issue Oct 24, 2017 · 18 comments
Closed
1 task done
Labels
bug 🐛 Something doesn't work

Comments

@Glavin001
Copy link

Glavin001 commented Oct 24, 2017

Expected Behavior

  1. Make change to file
  2. Save file
  3. React hot loader renders change without erorrs and keeps state

Current Behavior

  1. Make change to file
  2. Save file
  3. React hot loader throws error "React Hot Loader: this component is not accepted by Hot Loader."
  4. Page reloads (no saved state)
  5. Renders change

image

image

Steps to Reproduce (for bugs)

Use https://github.com/Glavin001/react-hot-ts/tree/material-ui-demo to reproduce with the above steps.

Context

I loose usage of React Hot Loader when adding Material UI to my project.

Your Environment

Tech Version
Material-UI 1.0.0-beta.18
React 16.0.0
React DOM 16.0.0
Glavin001 added a commit to Glavin001/react-hot-ts that referenced this issue Oct 24, 2017
@oliviertassinari oliviertassinari added the external dependency Blocked by external dependency, we can’t do anything about it label Oct 24, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 24, 2017

The hot reloading doesn't work on the root component for some reason. I does for nested component. I don't think that we can do anything about it here. I'm closing the issue for gaearon/react-hot-loader#666

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Oct 24, 2017
@Glavin001
Copy link
Author

Glavin001 commented Oct 25, 2017

Thanks @oliviertassinari!

Confirmed workaround:

-    "react-hot-loader": "^3.1.1",
+    "react-hot-loader": "3.0.0-beta.7",

Update: Downgrading react-hot-loader stopped the warnings for withStyle, however the state still continued to be cleared upon successful reloads. I removed usage of withRoot to fix the state clearing bug.

@oliviertassinari
Copy link
Member

@Glavin001 Notice that you can disable the warnings. Looks like most people are doing so.

@Glavin001
Copy link
Author

Notice that you can disable the warnings.

Warnings are there for a reason 😉 . These warnings are justified as it does not work.

My workaround currently remove withRoot usage: https://github.com/Glavin001/react-hot-ts/blob/material-ui-demo/src/withRoot.tsx#L29-L63
I am back to using "react-hot-loader": "^3.1.1", without withRoot and now able to build and have hot reloading working. 👍

@oliviertassinari
Copy link
Member

@Glavin001 It's good to hear it. So it seems to be related to the withRoot pattern.

@rrousselGit
Copy link

I'm running in the exact same problem. Warning + state lost everytimes I rebuild when using withStyles

reload

Here's a zip of that test project

8821.zip

@rrousselGit
Copy link

After some tests, I found something interesting.
You have to export the class sent to withStyles or else your component will loose it's state on hot-reload (and will get that warning).

This explains why withRoot of react-hot-ts/material-ui-demo doesn't work.

@oliviertassinari
Copy link
Member

@rrousselGit What could explain such behavior? It sounds like an happy heuristic.

@rrousselGit
Copy link

rrousselGit commented Oct 26, 2017

@oliviertassinari Seems like it is a limitation in react-hot-loader

React Hot Loader can't replace any Component, only registered ones.
- when using webpack loader - only module exports are registered.

https://github.com/gaearon/react-hot-loader#components-not-replaced

Which also implies that you can't do

export default withStyle({ /* anonymous style */})(
   class extends React.Component {
        // anonymous class
   }
)

So far, the best "hot-reload" setup with withStyle I found is a snippet which generate something like this :

export type TestStyleType = "root"
export const style: StyleRules<TestStyleType> | StyleRulesCallback<TestStyleType> = (theme) => {
    root: {}
}
export interface TestProps {}
export class Test extends React.Component<TestProps & WithStyles<TestStyleType>> {
    render() {
        const { classes: { root } } = this.props
        return <div className={root}>
        </div>
    }
}
export default withStyles(style, { name: "Test" })(Test)

But like I told you on gitter, this doesn't work when you change the style passed to withStyles
The component is properly reloaded. Local state is preserved. New classes are generated. But the styling always stays the same until you refresh

@Glavin001
Copy link
Author

@oliviertassinari Given the fact React-Hot-Loader explicit states these components -- which are not exported, such as is with withRoot -- are not supported, I think this should be reopened and we should discuss an implementation of withRoot which does support hot reloading.

@rrousselGit
Copy link

rrousselGit commented Oct 27, 2017

For the export thing, it seems that this is an issue with RHL 3.x
react-hot-loader 3.x doesn't like decorated components gaearon/react-hot-loader#279

For the styling issue, I think I found the problem.
Inside withStyles component, there are no componentDidUpdate to update the styling when it's property is changed.

A stateful hot reload is most likely updating withStyles props instead of using a new instance.
But since there's absolutely nothing that catch this update, theme is not reloaded.

Would be a good idea to reopen this issue, to take a look at this. Should be an easy fix.

@rrousselGit
Copy link

I've tested it and I can confirm this theory.
Using react-hot-loader, changing the style passed to withStyles trigger a componentWillReceiveProps and componentDidUpdate of the Style component.

But the problem is that Style.stylesCreator is assigned only in the constructor, never during updates.
Which prevents the hot-reload of withStyles.

Adding

    componentWillReceiveProps(props: RequiredProps) {
        this.stylesCreatorSaved = stylesCreator;
        this.attach(this.theme);
    }

to Style class solves the issue. And we get a fully working stateful hot-reload.

reload

@oliviertassinari
Copy link
Member

Using react-hot-loader, changing the style passed to withStyles trigger a componentWillReceiveProps and componentDidUpdate of the Style component.

@rrousselGit Oh wow, I have never seen react-hot-loader behaving this way. It would be awesome if you could create a reproduction repository so I can write a proper fix. I want to make sure we don't introduce any memory leak nor degrading performance.

@oliviertassinari oliviertassinari removed the external dependency Blocked by external dependency, we can’t do anything about it label Oct 27, 2017
@rrousselGit
Copy link

rrousselGit commented Oct 27, 2017

Sure.
Here's a fork of react-hot-ts that I used in the above gif.
https://github.com/rrousselGit/react-hot-ts

There's the fix for withRoot (exporting everything) included

@slavab89
Copy link
Contributor

There is also a bit context on this in their Troubleshooting which is basically what @rrousselGit found. Doing their suggesting in my project did help and made the warning go away but it still appears from time to time (due to some other external components probably).

@XBeg9
Copy link

XBeg9 commented Dec 20, 2017

Hello, could someone say me what I am doing wrong or issue with hot reloader warnings still exists in latest build?

material-ui-icons@^1.0.0-beta.17:
  resolved "https://registry.yarnpkg.com/material-ui-icons/-/material-ui-icons-1.0.0-beta.17.tgz#5f19af54a2d99eeef347a55414a6853e1c850dc3"
material-ui@v1.0.0-beta.24:
  resolved "https://registry.yarnpkg.com/material-ui/-/material-ui-1.0.0-beta.24.tgz#a51452d0486f88fd7f13ebed869793f298aa800c"

rasa_ui

@sarkistlt
Copy link

any updates on this issue? using next@5 which uses "react-hot-loader": "4.0.0-beta.18", having the same issue, with next@4 all works fine

@oliviertassinari
Copy link
Member

@sarkistlt Do you have a reproduction repository?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

No branches or pull requests

6 participants