search

cloverfield-tools / universal-react-boilerplate

A simple boilerplate Node app.
MIT License
904 stars 97 forks source link

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

Closed gacosta89 closed 8 years ago

gacosta89 commented 8 years ago

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 commented 8 years ago

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?

gacosta89 commented 8 years ago

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 commented 8 years ago

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 https://github.com/gaearon/react-transform-hmr/issues/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 https://github.com/cloverfield-tools/universal-react-boilerplate/issues/83#issuecomment-197086489

ericelliott commented 8 years ago

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 commented 8 years ago

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 https://github.com/cloverfield-tools/universal-react-boilerplate/issues/83#issuecomment-197619481

ericelliott commented 8 years ago

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

nkbt commented 8 years ago

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

ericelliott commented 8 years ago

:fire: :guitar: :notes:

sergeylukin commented 8 years ago

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 https://github.com/gaearon/babel-plugin-react-transform/issues/57

nkbt commented 8 years ago

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.