-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Conversation
This looks super interesting. One question though, since I see you've incorporated Dan Abramov's suggestion for removing react hmr plugins: I've been following the discussion about explicitly using vanilla webpack HMR for hot reloading, but I'm not sure I completely understand the impetus or rationale -- it strikes me as caving to the "JavaScript Fatigue" atmosphere. The original reason react-hmr even became a thing was so that you could maintain local component state and develop on the fly without having to constantly reload and reproduce app state. So, while this new solution solves some other problems, it totally negates the very reason that this plugin was invented in the first place. Thoughts? Looking forward to the rest of the PR. |
@davezuko with respect to
— James Kyle, the original tweet and responses from Dan Abramov and others (Feb 16, 2016)
— Dan Abramov, RFC: remove React Transform from examples #1455 (Feb 28, 2016)
— Dan Abramov, Hot Reloading in React (or, an Ode to Accidental Complexity) (March 8, 2016) TL;DR:
The one caveat—as you alluded to—is losing HMR for local component state. I would suggest that local state in a redux app is most likely an edge case (ie Modals) or a design flaw (app/ui state should really be in the store)... I don't think there's a huge impact on DX there, imo. Vanilla HMR as implemented in this PR maintains the store, catches runtime errors in redbox (and removes them), hot reloads all the things, etc. I would bet most people won't notice a difference. Rationale from Dan's original RFC to replace
It sounds like he's working on a new implementation of HMR that maintains component state. I suspect it will be a webpack v2 plugin, since facebook relies heavily on it. Any solution will eventually have to work with both stateless as well as stateful functional components (coming in a future version of react). So yeah, imo vanilla HMR is the right move for now. With react at version 15.0.0-rc.2, we can work out any kinks and roll out with v15... Let me know what you think |
Thanks for the reply. I will get back to you on Monday to continue the conversation. As much as I'd like to dig into this more right now I'm supposed to be avoiding unnecessary work due to wrist pains... so... have a great weekend! |
Current coverage is
|
return combineReducers({ router, ...asyncReducers }) | ||
} | ||
|
||
export const injectReducer = (store, name, asyncReducer) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: object instead of args
What I like most is the structure. It definitely deserve a go despite HMR or NOT-HMR |
One more question which came up while talking about this with a coworker: what determines the need to create a fractal? Taking a fairly trivial CRUD app, imagine switching between a dashboard which deals with a variety of models and a detail view, wouldn't they both want access to the same global state (especially if you've normalized your data)? What about user management, are there two different contexts (a global one for global state [session, etc.]), and one for each app? The structure looks appealing, but having never used it I'm wanting to better understand how it achieves modularity beyond folder structure, and how that modularity ties into a cohesive application. |
The need to create fractals would really be determined by domain requirements and constraints. The goal (obviously) is to encapsulate logic and create natural split points in your codebase. Trees (like, trees that live in a forest 🌲) are similar to fractal apps—the trunk is the router, branches are route bundles, and leaves are views composed of common/shared components/containers. Global application and UI state would ideally be placed on or close to the trunk (or perhaps at the base of a huge branch, eg. Sometimes it is also useful to reflect the route hierarchy in the project's folder structure, ie.
However, this nested route/folder structure is probably not necessary for a trivial CRUD app. Since bundling is handled by webpack compiler, the folder structure itself is really just for organization (development affordances ie. sub-repos, DLLs, etc). In small apps, it probably makes more sense to use a flat directory structure:
In both approaches, the route definitions would be virtually the same (pseudocode, don't copy-paste!): import App from 'App'
const App = {
path: '/',
onEnter: requireAuth,
component: App(store), // reducers for application/ui state (sync)
// async load route bundle and inject reducers (creates split point/branch)
getComponent: (nextState, cb) =>
System.import('./Dashboard')
.then((Dashboard) => cb(null, Dashboard(store)))
})
const Auth = {
path: '/auth'
// auth routes (anonymous): login, logout, passwordReset, etc
getComponent: (nextState, cb) =>
System.import('./Auth')
.then((Auth) => cb(null, Auth(store)))
}
const List = {
path: '/list',
component: ListLayout, // create named sidebar/main views
// async load route bundle and inject reducers (creates split point/branch)
getComponents: (nextState, cb) =>
Promise.all([
System.import('./List'),
System.import('./Detail')
]).then(([ List, Detail ]) => {
cb(null, {
sidebar: List(store),
main: Detail(store)
}))
}
}) This pattern becomes especially powerful as the app grows into multiple named routes, as you can dynamically load and replace leaf components on completely different branches of your application tree. For example, you could assign a new component to the 'sidebar' route based on an action in auth (ie. new permission) I'm not the best at breaking down complicated topics, I hope this answers your question :) |
I forgot to address your point about global normalized data... imho, creating materialized (derived) views from normalized data is really the essence of using container components with selectors, as opposed to reducing into global state atom... I added some comments to CounterContainer to explain the pattern: // Note: mapStateToProps is where you should use `reselect` to create selectors, ie:
import { createSelector } from 'reselect'
const counter = (state) => state.counter
const tripleCount = createSelector(counter, (count) => count * 3)
const mapStateToProps = (state) => ({
counter: tripleCount(state)
}) From https://github.com/reactjs/reselect docs:
|
I totally understand the container component pattern, and a huge 👍 for reselect; I think I was just getting caught up in creating too many fractals (and with them stores) where there are a variety of competing redux states (which confused me as to how you would "select" from the right state atom). Do you think https://www.npmjs.com/package/react-redux-provide becomes useful at all in this setup? |
i love the concept of redux-provide—i've been following it closely! another similar project is https://github.com/artsy/react-redux-controller. both of these look awesome, but they they are so new i'd need to see how they are maintained, tested, etc before relying too heavily on it. for now, i'm leaning towards something like this PR (maybe paired with redux-saga) and letting the dust settle. the main idea is getting our business logic into units, which we've already done to a large extent with redux modules.
i should note that fractals do not create their own stores; we pass the instantiated store to route config which uses update: i just noticed |
I like the idea. I will fork the branch and work on the internationalisation like @juanda99 did for the current react redux starter kit |
Big +1 for reselect. I really like all of the ideas presented here! @justingreenberg I think it will be important to do a good job documenting how/when/why to use the fractal pattern and link to resources on how people can get perf benefits by using code splitting when they need it |
I think it's unanimous that reselect is awesome, but I don't think it's necessary to include it in the starter kit. Right now I think a lot of people are caught up in the hype of tooling without understanding everything involved (a major source of issues for this project, and some of my own misunderstandings), so I'd really like to limit the amount of stuff included, allowing people to naturally discover tools as they understand the problems they aim to solve (though we can definitely point them in the right direction). That aside, definitely 👍 for continuing down the proposed path. |
I add ruffly the translation support on the fort made by @juanda99. Here is the branch : https://github.com/juanda99/react-redux-starter-kit/tree/fractal I will refactor the code later, time to sleep. @justingreenberg : 👍 for your RFC. Can you have a look on the branch I'm talking about, I place the redux stuff for the language in the store/modules, but i'm pretty sure there is a better place ? |
@@ -280,7 +266,7 @@ if (!__DEV__) { | |||
).forEach((loader) => { | |||
const [first, ...rest] = loader.loaders | |||
loader.loader = ExtractTextPlugin.extract(first, rest.join('!')) | |||
delete loader.loaders | |||
Reflect.deleteProperty(loader, 'loaders') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woah, never seen this before.
Master branch has been updated to stable |
@@ -297,6 +341,10 @@ Have more questions? Feel free to submit an issue or join the Gitter chat! | |||
Troubleshooting | |||
--------------- | |||
|
|||
### Redux DevTools | |||
|
|||
We recommend using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should just link to the section you added above?
…ter-kit into fractal * 'master' of https://github.com/davezuko/react-redux-starter-kit: fix(webpack/karma): add react/addons to externals
@davezuko sounds good 👍 i agree it's probably ok to merge without blueprints, since they can always be generated and then moved to a more permanent home if necessary. i'm not sure if redux-cli handles folder creation, but if not we could add a containers folder with .gitkeep to src |
@@ -2,7 +2,7 @@ import { injectReducer } from '../../store/reducers' | |||
|
|||
export default (store) => ({ | |||
path: 'counter', | |||
getComponents (location, next) { | |||
getComponent (nextState, next) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per latest release:
https://github.com/reactjs/react-router/blob/master/upgrade-guides/v2.2.0.md
looks like the roadmap has a strong emphasis on improving async routing, so this is def a step in the right direction
Also: thinking about it, I may cut a |
@justingreenberg hoping to get this merged today. I will cut a |
|
The bug in CI has been fixed here for the factal branch of my fork : https://github.com/rsilvestre/react-redux-starter-kit/tree/fractal. commit : d8c0ba5 This fork include the internationalization You need to delete this file : src/redux/configureStore.js |
@rsilvestre good catch 👍 thank you! |
any idea when this project will stabilize? This is the 2nd time I've had to start over. |
@chovy Sorry to hear that. Perhaps it deserves a better description, but this repo is more of a living project than a completely stable and static boilerplate. So much is constantly changing in the JS community that things get out of date quickly. Because of that, I don't recommend trying to stay at the very tip of the development spear just because it exists. We do it for the starter kit because we're trying to provide our interpretation of the best tools and practices, but at almost any point in time (namely tagged releases) you should be able to just take that version and run with it. A few of projects I work on, at both work and home, are based on older versions of the starter kit and work perfectly fine. If you do try to stay with the latest and greatest you're going to experience growing pains, whether it's from broken/upgraded packages, design philosophy changes, or evolving best practices. It's up to you which to choose. And, as far as this RFC, sometimes there are just better ways of doing things than we'd previously considered. @justingreenberg made a profoundly good case for moving in this direction and has received multiple nods of support, so that's the direction we'll go. I'd love to say "This is it" and declare the project stable, but that won't stop the community from continuing onward. The best I can do is create tagged releases, and, what I can do better than I am, is indicate to users what exactly those tagged releases include, so they can make the best decision for them and their team. |
@justingreenberg one final question, about |
|
||
// Use Redux DevTools chrome extension | ||
if (__DEBUG__) { | ||
if (!window.devToolsExtension) window.devToolsExtension.open() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justingreenberg this logic is actually flipped. It tries to open the extension if it doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just went ahead and fixed it: 9e03fbc
Merged 🎉 , great work @justingreenberg... as always, showing me how much I don't know. |
This project has become my defacto source for my projects so keep up the great work guys and awesome work @justingreenberg! Would you be able to explain how (and maybe add additional newbie docs) the new router/index.js works? |
All props go to @justingreenberg . @ethanve I would love to get a solid wiki in place in addition to a full README rewrite/cleanup. So much has changed that the original ( ✨ pristine ✨ ) docs are getting a bit too disorganized. |
Tried very hard to understand the "Fractal Structure", and also convert the 2.0 base project to this structure. I could tell the lazy-load benefit, but no more than that. Also confused about the global components & container vs routes module component & container. I'm appreciate all you @davezuko and @justingreenberg 's hard working. Love the project. Thanks. |
One question about the structure, where should we maintain global Redux modules, which is not belond to any routes (or say views)? Should we just bring |
@BorelTung totally, or |
RFC: Fractal Project Structure
Please provide feedback! Structure excerpt from README (Updated 4/11/16):
Fractal App Structure
Also known as: Self-Contained Apps, Recursive Route Hierarchy, Providers, etc
Small applications can be built using a flat directory structure, with folders for
components
,containers
, etc. However, this structure does not scale and can seriously affect development velocity as your project grows. Starting with a fractal structure allows your application to organically drive it's own architecture from day one.We use
react-router
route definitions (<route>/index.js
) to define units of logic within our application. Additional child routes can be nested in a fractal hierarchy.This provides many benefits that may not be immediately obvious:
Large, mature apps tend to naturally organize themselves in this way—analogous to large, mature trees (as in actual trees 🌲). The trunk is the router, branches are route bundles, and leaves are views composed of common/shared components/containers. Global application and UI state should be placed on or close to the trunk (or perhaps at the base of a huge branch, eg.
/app
route).Layouts
react-router
named components into viewsComponents
const HelloMessage = ({ name }) => <div>Hello {name}</div>
components
andcontainers
directories contain reusable componentsContainers
Containers only
connect
presentational components to actions/stateOne or many container components can be composed in a stateless function component
Tip:
props
injected byreact-router
can be accessed usingconnect
:Routes
index.js
that returns route definitionroutes
directory in a fractal hierarchy.Note: This structure is designed to provide a flexible foundation for module bundling and dynamic loading. Using a fractal structure is optional—smaller apps might benefit from a flat routes directory, which is totally cool! Webpack creates split points based on static analysis of
require
during compilation; the recursive hierarchy folder structure is simply for organizational purposes.Summary:
/counter
routemodule.hot
instrumentationreact-transform
babel plugin from development buildsredbox-react
to display render errorsReact-v15.0.0
Please leave questions/comments