Some suggestions for the HelloWorld example

[mongjong59]

I know that I don't have to follow the project structure and coding style of the HelloWorld example thanks to the flexibility of this gem. However, people are new to Redux will follow it and some part of it can be misleading (at least seems to me, if I missed any point, please let me know). Here are some problems I've noticed:

1. unnecessary wrapper for the root app state
In the HelloWorld example generated, we have:

// Redux expects to initialize the store using an Object, not an Immutable.Map
const initialState = {
  $$helloWorldStore: $$helloWorldState.merge({
    name,
  }),
};

I don't quite see the point of nesting the $$helloWordStore inside initialState. I guess the intent is to show that how to use Immutable.js with Redux in the example. But this nesting structure seems to imply that all the state related to hello world app to go inside this $$helloWorldStore immutable object. So the state will look like:

{$$helloWorldStore: Immnutable.Map({name: "John Doe"})}

Later, if we want to have a "person" instead of "name". It will look like:

{$$helloWorldStore: Immnutable.Map({
  person: Immnutable.Map({name: "John Doe", age: "25"})
})}

This adds much unnecessary complexity to the code and the benefit is not obvious to me. Why not just have something like this as our root state:

{person: Immnutable.Map({name: "John Doe"})}

As there will be only one store in an app, IMO there won't be any sibling of $$helloWorldStore. I think the point of using Immnutable.js with React is for shallow comparison to work properly so that shouldComponentUpdate can return expected result. As shallow comparison in React will "iterate on the keys of the objects being compared and returning true when the values of a key in each object are not strictly equal", All we need is that any value in the root state to be Immutable (which should give us what we want). So, I don't see the value of wrapping our whole app state in an Immutable object.

Also for "Redux expects to initialize the store using an Object, not an Immutable.Map" issue mentioned in the comment, it's not Redux exactly. It's that combineReducers() treats state as plain object. If we really want to make our root state an Immutable object (which I think is unnecessary), we should go with redux-immutable. If there's no good reason of the $$helloWorldStore nesting, I think it's not appropriate for people to follow.

2. unnecessary "export" of initialStates from reducer
helloWorldStore.jsx

const initialState = {
  $$helloWorldStore: $$helloWorldState.merge({
    name,
  }),
};
//
const store = storeCreator(reducer, initialState);

$$helloWorldState is exported all the way from reducers/helloWorldReducer.jsx and reducers/index.jsx. But according to the doc, the second argument of createStore is something you may optionally specify it to hydrate the state from the server in universal apps, or to restore a previously serialized user session. If we want to manually pre-define the state, passing it into reducers should be good enough.

3. combineReducers() is called in helloWorldStore.jsx instead of reducers/index.jsx
Most Redux tutorials usecombineReducers in files like reducers/index.jsx. As what combineReducers() returns is also a reducer, it's much more straightforward to call it in reducers/index.jsx.

[justin808]

@jbhatab, I believe you set this up. Any comments?

CC: @Judahmeek @robwise

[justin808]

@nostophilia Please create a github repo as you believe the generated code should be so we have something to discuss. It's best if you can put the recommended generated code in a PR so we can comment on it.

[mongjong59]

@justin808 I created a PR: #584
Another issue of the hello world example I found recently is here:

const HelloWorld = (props) => {
  const { dispatch, $$helloWorldStore } = props;
  const actions = bindActionCreators(helloWorldActionCreators, dispatch);
  const { updateName } = actions;
  const name = $$helloWorldStore.get('name');

   // This uses the ES2015 spread operator to pass properties as it is more DRY
   // This is equivalent to:
   // <HelloWorldWidget $$helloWorldStore={$$helloWorldStore} actions={actions} />
   return (
     <HelloWorldWidget {...{ updateName, name }} />
   );
 };

If we call bindActionCreators like this, every time HelloWorld component is rendered, bindActionCreators will be called, producing a set of bound actions with the same code but different references. This will produce unexpected result for PureComponents or components with PureRenderMixin. If we passed any bound actions to it with the code above, shouldComponentUpdate will always be true even when it should be false. We can just pass selected actions as a plain object as the second argument to connect(), they will be bound by the connect() function.

[justin808]

@nostophilia I think you have some good points. However, we're trying to get the simplest possible code generated to show the hookup. @jbhatab wrote this. Maybe he can comment.

[jbhatab]

@nostophilia yeah lots of best practices have changed. You are most likely right on this one and it would just require some reworking. I do think the tutorial is more about simplicity than perfection, but a change up would be great.