-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Convert classes to stateless components #279
Conversation
@patrickheeney this breaks HMR since react-transform does not support functional components... i am tracking progress closely, i fully support this switch once support is implemented but it might be a while |
@justingreenberg It is unfortunate, but the only time I generally update stateless components is to edit the html, at which point I refresh. The modifications to the state or other parts of the app still work with HMR. How often do you edit CoreLayout or Root container? Hopefully they fix upstream soon like the babel 6 issue. Related #277. |
@patrickheeney this will break HMR for all presentational components ie. HomeView |
@justingreenberg Indeed, just the home view, but I would argue the others are rarely changed. However, this coincides with the existing AboutView so the current behavior of refreshing these two views would be similar. |
@patrickheeney yea I actually added AboutView as functional component in my PR migrating to personally, i leverage HMR to maintain component state when building, styling and testing UI. this change would break everything attached to any stateless components (styles, classes, etc), which would essentially relegate HMR to reducers also the issue isn't babel 6, it's actually react itself—currently there is no good way to track changes to these components since they are just simple functions that render html, whereas component classes can be tracked using their displayName. one of the babel authors started a discussion and outlines the issues better than i could in gaearon/babel-plugin-react-transform#57 i wish this worked now, i really would prefer to keep components stateless 😄 |
I will close this now as the starter kit is likely to change by the time this is no longer an issue. Secondly I didn't update the tests. I was trying to say the issue is similar to babel 6, in that we are essentially waiting.. A "bug" in an upstream library prevents the usage of class based components with super(), and in this case a library preventing stateless components. |
another option would be to use pure component classes (referentially transparent—only render method) during development to support HMR development workflow, then use the babel pipeline to transform them into pure functions for production using babel-plugin-react-pure-components |
@justingreenberg Interesting. Also, this worked for me as another idea. What about you?
|
Just thinking out loud. The wrapper could be useful when developing a component, when you are done with it, comment it out for the pure component. I also like the idea of using the babel plugin for production so I might explore that as well. |
@patrickheeney wrapping with a class is a great pattern for adding lifecycle methods and fixtures for testing (and yes would enable HMR) but imo wouldn't really add much practical value over pure component classes at present. it should be noted that the react team is actively working on the referential issue and and optimizations for functional components. i've been using recompose a lot lately, which is an awesome toolkit for composing pure components—when referential support is added to react core it is going to be a game-changer! |
@justingreenberg I thought of it merely as a means to enable HMR for development. Basically you would uncomment the wrapper to dev a component, re-comment when it is "production ready". For example CoreLayout for me will never change, so I would likely always leave it a pure component. If I need to edit it where a refresh wouldn't do the job, I would wrap it for HMR. (Only temporarily until the issue is fixed). I will have a look at recompose, sounds pretty cool. |
yeah i hear you. the problem is react-transform needs references to upstream components. for example, if you wanted to enable HMR for HomeView, you would have to wrap Root and CoreLayout (as well as HomeView itself). needless to say this would become very tedious as the component tree grows :( update: ok so i dug a little deeper and it looks like component references in between root and edge nodes are no longer an issue since the rewrite of react-transform |
I didn't do any extensive testing but I only wrapped home view, the other Anyway, will leave this tabled for now and see how things progress
|
You might be interested in reduxjs/redux#1455. |
React Hot Loader 3 supports functional components without destroying the state. |
It might be worthwhile to create a view to demonstrate a state-full component, maybe a button that toggles something.