Skip to content
This repository has been archived by the owner on Sep 11, 2018. It is now read-only.

Hot Module Reloading not working with functional components #277

Closed
ntgn81 opened this issue Dec 13, 2015 · 6 comments · Fixed by #292
Closed

Hot Module Reloading not working with functional components #277

ntgn81 opened this issue Dec 13, 2015 · 6 comments · Fixed by #292

Comments

@ntgn81
Copy link
Contributor

ntgn81 commented Dec 13, 2015

Hot Module Reloading is not yet working with functional components, according to gaearon/babel-plugin-react-transform#57

Took me some time to get to that link. Just putting this out here in case someone else has the same issue with AboutView not hot reloading.

Looks like until it's fixed, it might be better to not use functional components. HMR gives me so much more than functional components at this point.

Also, this is a great starter kit. Exactly what I wanted without all the universal fuzz, making it so much harder to learn. Thank you for the great work! 🌟

@mtsr
Copy link
Contributor

mtsr commented Dec 13, 2015

Thanks for sharing, good to know why my About Page wasn’t reloading.

On 13 Dec 2015, at 04:46, Nam Nguyen notifications@github.com wrote:

Hot Module Reloading is not yet working with functional components, according to gaearon/babel-plugin-react-transform#57

Took me some time to get to that link. Just putting this out here in case someone else has the same issue with AboutView not hot reloading.

Looks like until it's fixed, it might be better to not use functional components. HMR gives me so much more than functional components at this point.

Also, this is a great starter kit. Exactly what I wanted without all the universal fuzz, making it so much harder to learn. Thank you for the great work!


Reply to this email directly or view it on GitHub.

@justingreenberg
Copy link
Contributor

@mtsr @ntgn81 see discussion on #279 for explanation and discussion

TL;DR: react-transform does not support functional components... this is known, documented and deliberate due to limitations with react itself (this issue can be closed)

@davezuko i kinda assumed HMR for functional components wouldn't be an expectation since react-transform-hmr never supported it. might be be a good idea to add a note to docs about this?

@dvdzkwsk
Copy link
Owner

@justingreenberg My vote is that we just make everything extend React.Component for now until there's some movement on this from react-transform-hmr. I'd like to avoid people submitting bugs or thinking that the starter kit doesn't work because they're messing with pre-packaged stateless components. A note would still be nice to add so people who incorporate their own know why HMR doesn't work for those.

@patrickheeney
Copy link
Contributor

@davezuko Sounds good. Are there any optimizations for production in place yet? For example the one mentioned with #279.

@justingreenberg
Copy link
Contributor

they are still considered pure components as long as they only have a render method. @patrickheeney i have a working branch using babel-plugin-react-pure-components (written by babel core contributor) that detects pure components and transforms them into function components for optimized production builds.

i haven't opened a pr yet because i'm using a local npm linked version of the plugin... the published npm package contained deps that were out of date and must be updated prior to merge, since it's not working "out of the box" yet

i dug a little deeper and it looks like referentially transparent components in between root and edge nodes are no longer an issue since the rewrite of react-transform... transforms are not applied to function components but the plugin continue to traverse it's children. this means that the Root component must be component class, however something structural such as the CoreLayout component could be a function component (since it doesn't "need" HMR) and transforms still work on HomeView.

This is essentially what I did in #292, with a explanation

@mtsr
Copy link
Contributor

mtsr commented Dec 14, 2015

@justingreenberg Awesome work. I'd love to get that at some point, even if it might not make it into the starter kit because of the complexity.

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

Successfully merging a pull request may close this issue.

5 participants