Skip to content
This repository has been archived by the owner on Dec 21, 2024. It is now read-only.

Avoid react-transform-hmr when compiling server side code. #83

Closed
coatlicue89 opened this issue Mar 15, 2016 · 10 comments
Closed

Avoid react-transform-hmr when compiling server side code. #83

coatlicue89 opened this issue Mar 15, 2016 · 10 comments
Assignees

Comments

@coatlicue89
Copy link

I think you should avoid the react-transform-hmr step, when compiling server side code as this gaearon comment suggests. Particularly when you run the functional tests and e2e tests.
One simple solution might be using BABEL_ENV=production or something similar to disable the babel transform step. Another one is described further in the issue #5 where they suggest to put the .babelrc config inline in the webpack.config file.

Also I noticed that when running the dev console (npm start), of course hmr should be enabled but the devServer is doing server side rendering as well. This is an inconsistency. Is server side rendering necessary for development?

@ericelliott
Copy link
Contributor

I think you should avoid the react-transform-hmr step

Absolutely right. Would you like to propose a PR?

Is server side rendering necessary for development?

Is this causing problems?

@coatlicue89
Copy link
Author

Ok, I'll propose a PR.
It is known that hmr does not work with pure render components yet. When i tried to make it work with React.createClass method I found the error of the issue #5, in the functional tests and in the dev console. But I don't know why if you use pure render components the error doesn't come out.

@nkbt
Copy link
Contributor

nkbt commented Mar 16, 2016

Hot reloading in general now is in the middle of transition to something
better. Hmr will be retired soon. Existing solutions are temporary and do
not work for all cases, unfortunately. Internally we decided to go with
PORC (plain old react components 😏) which work perfectly with hmr and then
migrate to stateless components when hot reloading works with them.

See
https://medium.com/@dan_abramov/hot-reloading-in-react-1140438583bf#.lrehixng9
for more details on reasons, problems and potential solutions

On Wed, 16 Mar 2016 at 11:39 AM, gacosta89 notifications@github.com wrote:

Ok, I'll propose a PR.
It is known that hmr does not work with pure render components yet. When i
tried to make it work with React.createClass method I found the error of
the issue #5 gaearon/react-transform-hmr#5,
in the functional tests and in the dev console. But I don't know why if you
use pure render components the error doesn't come out.


You are receiving this because you are subscribed to this thread.

Reply to this email directly or view it on GitHub
#83 (comment)

@ericelliott
Copy link
Contributor

I'm not willing to trade pure components for HMR. Pure components are simpler & faster. HMR is nice to have -- and with the live dev console running unit tests, meh.

See Browsersync for an alternative to HMR. Admittedly much less cool, but good enough for the time being... the promise of HMR has not fully materialized in any of my real projects, yet. It works until it doesn't, and then it's just extra complexity for nothing.

@nkbt
Copy link
Contributor

nkbt commented Mar 17, 2016

Yep, it is all about priorities. Since hmr is not working with stateless
components it is worth removing it from the repo, shall we?
On Thu, 17 Mar 2016 at 11:15 AM, Eric Elliott notifications@github.com
wrote:

I'm not willing to trade pure components for HMR. Pure components are
simpler & faster. HMR is nice to have -- and with the live dev console
running unit tests, meh.

See Browsersync https://www.browsersync.io/ for an alternative to HMR.
Admittedly much less cool, but good enough for the time being... the
promise of HMR has not fully materialized in any of my real projects, yet.
It works until it doesn't, and then it's just extra complexity for nothing.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#83 (comment)

@ericelliott
Copy link
Contributor

Yes. If you want to contribute a PR, that would be wonderful. =)

@nkbt
Copy link
Contributor

nkbt commented Mar 30, 2016

Back from holidays, ready to rock again \m/. Sorry for the delay

@ericelliott
Copy link
Contributor

🔥 🎸 🎶

@sergeylukin
Copy link

If only I found this topic before... Spent literally hours on trying to figure out why my components don't get HMRed, was sure it was my fault somewhere; only to find out from this thread that pure components are not supported. Indeed there is an open discussion about that at gaearon/babel-plugin-react-transform#57

@nkbt
Copy link
Contributor

nkbt commented Apr 3, 2016

Yep. Adding a wrapper for client side app helps, though it does not work with non-pure components in a way that their own state is lost. Pretty much everything is explained in details by Dan (see link above).

I am not sure really what is better, having it working partially or not having at all. Leaning towards adding pure hmr and waiting until React has support for hmr natively to keep component's state.

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

No branches or pull requests

4 participants