Using combineReducers to define shape of the state

[tomazy]

This is another PR extracted from #1236

Please help me to review it.

[samit4me]

This looks really good, I find this to be much cleaner and easier to read. Tested it locally and it works as per normal and without error 👍

Should the routeReducer also be updated for consistency (example below)?

function locationBeforeTransitions(state = null, action) {
  switch (action.type) {
    /* istanbul ignore next */
    case LOCATION_CHANGE:
      return action.payload;
    default:
      return state;
  }
}
const routeReducer = combineReducers({ locationBeforeTransitions });

[coveralls]

Coverage increased (+0.02%) to 99.267% when pulling a362da0 on tomazy:ssr-refactor-reducers into 4613da7 on react-boilerplate:dev.

[tomazy]

@samit4me good point! I've updated the code.

[coveralls]

Coverage decreased (-0.3%) to 98.905% when pulling 08ea03f on tomazy:ssr-refactor-reducers into 4613da7 on react-boilerplate:dev.

[samit4me]

LGTM ✨

Looks like AppVeyor and Chrome strikes again.

@gihrig @KarandikarMihir Are you able to cast your eyes over this?

[gihrig]

generators need updating - see testing comment.

[gihrig]

A generated route shows the Not Found Page in production mode, OK in dev mode.

I tested according to the following script.

-- Test script --
yarn && npm start
Test Dev, Dev tunnel, Features, lang = de, NotFound, Production, off-line
npm run clean
npm run generate component TestComp /w defaults
npm run generate container TestPage /w defaults
npm run generate route to TestPage - /test
use TestComp on TestPage -> bypass all tests in TestComp and TestPage (set true = true)
npm start > localhost:3000/test
npm test (expect test failure due to incomplete test coverage)
npm run build

**Problem:** npm run start:prod  > localhost:3000/test <--- _shows Not Found Page_

Kill server  > localhost:3000 _OK_ ~/test _ site can’t be reached_ - see docs > server config

[gihrig]

This looks really good, I find this to be much cleaner and easier to read.

Yes but, It's a break from the "standard" redux reducer pattern most people are familiar with.

To expect that a new immutable state object will be returned when the reducer returns a boolean, just make no "sense" (there must be some "magic" happening somewhere).

I'm not arguing against this PR, but another, similar proposal was made to use redux-actions to greatly simplify the creation of actions in #1476 and that idea was shot down for this reason.

If we are to go forward with the change, I would suggest:

1. The change needs to be documented. In code comments for sure, existing docs need to be reviewed as well.
2. The rationale for making the change needs to be spelled out, perhaps just in the opening comment of this PR.
3. If redux-immutable is a good idea, redux-actions is probably a good idea (for actions, not reduceres) for the same reasons.
4. While not a breaking change, IMO, this is sufficiently different enough that that it's inclusion should trigger a minor version bump to 3.5.0 and 3.4.1 should be released first.

Overall I like the change, boilerplate reduction is (usually) a good thing. 😄

[coveralls]

Coverage decreased (-0.3%) to 98.905% when pulling 939aa4c on tomazy:ssr-refactor-reducers into 2d09df1 on react-boilerplate:dev.

[tomazy]

@gihrig thanks for your input. I've updated the templates.

A generated route shows the Not Found Page in production mode, OK in dev mode.

That's weird - the changes in this PR don't introduce anything that might cause that. The reducers should work exactly the same as previously. I've just tested your scenario and I can't reproduce it.

This looks really good, I find this to be much cleaner and easier to read.

Yes but, It's a break from the "standard" redux reducer pattern most people are familiar with.

I'm not sure about that. Combining reducers is mentioned in the redux docs and it's a quite standard thing IMO.

If we are to go forward with the change, I would suggest: (...)

I'm afraid I will not find time to address your suggestions. I've been quite busy with other stuff and it becomes more difficult for to keep the #92 branch alive (that's the main reason for this PR). If you guys are not happy with this PR then we can close it and I'd remove these changes from #92 (they are not actually critical for it) to keep it leaner.

[gihrig]

#92 Yikes, I had no idea this project (SSR) went back that far. It's been a long haul!

If you guys are not happy with this PR then we can close it and I'd remove these changes from #92

@tomazy It was absolutely not my intention to discourage. But I am concerned about the approachability of the project. It's got plenty of difficult stuff as is.

Combining reducers is mentioned in the redux docs and it's a quite standard thing IMO.

I totally agree about combining reducers, no issue there. It's the change from returning an obvious state object to returning a primitive value.

e.g. changing from:

      // Delete prefixed '@' from the github username
      return state
        .set('username', action.name.replace(/@/gi, ''));

to:

      // Delete prefixed '@' from the github username
      return action.name.replace(/@/gi, '');

It looks to me like a simple string is being returned rather than an immutable state object. Obviously I don't grok what's really happening here, that's all.

I'm afraid I will not find time to address your suggestions. I've been quite busy with other stuff and it becomes more difficult for to keep the #92 branch alive.

I totally get it about difficulty finding time! 😬

What do you, and others think about creating an SSR branch in this project?

Kept up as a super-set of dev, that would allow these related PRs to have a place to live as an evolving sub-project. This PR for example would be right at home in a branch dedicated to SSR and the issues I raised could be addressed separately by the community. I'm hopeful that would invite wider contribution to the SSR feature and still facilitate release as a part, or all?, of RBP 4.0.

With huge props to @tomazy for all the work put in on this long journey, I'd like to hear input from others. I think SSR is too big a feature to let slip away.

[tomazy]

@gihrig

@tomazy It was absolutely not my intention to discourage.

I'm sure about that.

But I am concerned about the approachability of the project. It's got plenty of difficult stuff as is.

I understand it's hard to find balance between the approachability vs. best practices. I know it's all subjective but I'd put more emphasis on the latter.

What do you, and others think about creating an SSR branch in this project?

Sounds good to me

[gihrig]

Cloned a new copy of your branch tested as before, all pass ✨

So this PR has my approval, but I'll let others have a say as to whether to merge into dev or create an SSR branch.

[blling]

@tomazy ping!
Conflicts need resolve:)

[coveralls]

Coverage decreased (-0.4%) to 99.625% when pulling ee9a4f9 on tomazy:ssr-refactor-reducers into 2c276a5 on react-boilerplate:dev.

[tomazy]

@blling here you go

[gretzky]

Nice work

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.