combine / universal-react-redux

🧐 A sensible universal starter kit for React + Redux
110 stars 18 forks source link

Is there a reason that store.js is split between dev and prod ? #12

Closed hartym closed 8 years ago

hartym commented 8 years ago

Before I suggest any code about that, I was wondering if there is actually a reason for the split. I understand that the two factories are not doing the exact same thing, but using conditions that can even be simplified by webpack, one factory can achieve the goals of both dev and prod.

What do you think? If there is no reason, I still need a bit of cleanup in my code but I have a working simplification here.

Also, I'm wondering if it would be ok with you to define the same variables as the webpack "DefinePlugin" defines as globals in server.js, so one can do conditionals like if (__DEVELOPMENT) or if(__PRODUCTION__), etc.

Depending on what you agree on or not, I'll submit patches.

hartym commented 8 years ago

Also, I'm wondering if the choice of using component based react devtools instead of chrome extension devtools is a choice, or just a coincidence ?

calvinl commented 8 years ago

1) The reason is primarily because of Redux Devtools, which recommends the split. It is a bit more maintenance, and originally I had a single file using conditionals, but I thought about other users who might want to include additional tooling in development, and this makes it easier and clearer to ensure you're adding it to the right place.

That said, I don't mind removing the component based redux devtools in favor of adding something to the README that recommends using the chrome extension instead. I didn't know the chrome extension existed at the time I published this starter kit.

2) re: server environment variables. I don't really mind doing this, but I can see one downside, and that's confusion to the user thinking that the variables are being defined by Webpack, when they're not. The server only uses the isomorphic webpack configuration for assets.

I say go ahead and submit a patch for the server variables, but I'd recommend leaving the separate dev/prod stores.