fix/feat(store): createReducer: on should not be overwritten

[zhaosiyang]

createReducer should allow ons to take actions of same type and excute all ons to match the action dispatched

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

If the following is configured and initial state is 0, if increase action is dispatched, the next state is 2, because the the second on has same action type as the first on, and the first on is overwritten.

const increase = createAction('[COUNTER] INCREASE');
createReducer(0, on(increase, state = state + 1), on(increase, state = state + 2))

(Indirectly) Closes #1956

What is the new behavior?

In createReducer, on should not overwrite previous ons if they have the same action types.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

[zhaosiyang]

I have added an example of the necessity of this behaviour here (#1956 (comment))

[ngrxbot]

Preview docs changes for e8aebae at https://previews.ngrx.io/pr2103-e8aebae/

[ngrxbot]

Preview docs changes for dcc72ee at https://previews.ngrx.io/pr2103-dcc72ee/

[timdeschryver]

I'm not sure if this is a "bug".
I think both options are viable, and we'll have to decide which behavior makes the most sense and is safest to use.

[zhaosiyang]

I'm not sure if this is a "bug".
I think both options are viable, and we'll have to decide which behavior makes the most sense and is safest to use.

Yeah, it could be either feature or bug.

In my opinion, the on should simply mean "execute this function when this action is dispatched" regardless of any other settings. We thus decoupled ons in this way.

Some usage case for justifying this feature could be:

1. Update some state (that I defined) whenever some library-defined action is dispatched, for example (@ngrx/store/init, '@ngrx/store/update-reducers )
2. Create higher order reducers which inject some global ons: please see Docs: Add more explanation that you can only have one distinct action type per on function with createReducer #1956 (comment) for more detail

However, I can't think of any case that need to overwrite previous on if action type is the same.

[MikeRyanDev]

You could do the composition on the fly here instead of storing arrays for a small perf bump in the generated reducer:

if (map.has(type)) {
  const reducer = map.get(type) as ActionReducer<S, A>;
  map.set(type, (state, action) => on.reducer(reducer(state, action), action);
}
else {
  map.set(type, on.reducer);
}

[zhaosiyang]

Thanks @MikeRyanDev for the great suggestion. I have pushed a commit. :)

[ngrxbot]

Preview docs changes for 685cb32 at https://previews.ngrx.io/pr2103-685cb32/

[ngrxbot]

Preview docs changes for a32c101 at https://previews.ngrx.io/pr2103-a32c101/

[brandonroberts]

@timdeschryver you have anymore thoughts on this?

[timdeschryver]

This looks good for me @brandonroberts
I believe we should follow-up with some docs changes, if I remember correctly it mentions that the on will be overwritten.