feat(store): add createReducer function

[alex-okrushko]

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?

Introduces createReducer function, that has the same functionality as reducer from ts-actions (created by @cartant).

It helps remove Action unions and dramatically reduce the code needed for reducers. Compare:

However, there are a few caveats that are caused by microsoft/TypeScript#3755 and microsoft/TypeScript#7547 (comment) with the latter acknowledging current TS limitation.

Sometimes reducer functions in on(...) have to be explicitly typed with their return type. Let's look at the example:

export interface State {
  showSidenav: boolean;
}

const initialState: State = {
  showSidenav: false,
};

export const reducer = createReducer<State>(
  [
    // Explicit 'State' return type for the 'on' functions is needed because
    // otherwise the return types are narrowed to showSidenav: false instead of
    // showSidenav: boolean (that State is asking for).
    on(LayoutActions.closeSidenav, (): State => ({ showSidenav: false })),
    on(LayoutActions.openSidenav, (): State => ({ showSidenav: true })),
  ],
  initialState
);

If the State in on(LayoutActions.closeSidenav, (): State is not provided it would narrow the type to {showSidenav: false} and {showSidenav: true} correspondingly, which would not match the interface of State (which is {showSidenav: boolean}).
TS highlights this as error, so providing the State is required

Sometimes typos could be missed

Let's look at another example (reduced for brevity):

export interface State {
  error: string | null;
  pending: boolean;
}

export const initialState: State = {
  error: null,
  pending: false,
};

export const reducer = createReducer<State>(
  [
    on(AuthApiActions.loginFailure, (state, { error }) => ({
      ...state,
      error,
      ending: false, // <--- 'pending' with typo here - missed letter 'p'.
    })),
  ],
  initialState
);

Note the ending: false, that missed p. The original intention was to spread the state and override with pending: false, however because of the typo pending won't be overridden and instead extra ending property will be added.
This will not match the State interface, but State could be extended from this interface with type (because entire state of type State is spread).

switch-based reducer function catches these types of errors, because providing the return type of the reducer function is a norm: function reducer(state: State = initialState, action: Action): State.

Solution: This unfortunate error can be also caught by explicitly typing the reducer within on:
on(AuthApiActions.loginFailure, (state, { error }): State => ({..})),

For me, that's an acceptable solution. Maybe we should recommend to always type it. I wish the createSelector<State> would've caught it, but it's not currently possible due to TS limitations mentioned above.

Note: if the State typed could not be extracted from the returned object literal, the error would be displayed property even without explicit return type, e.g.:

 on(AuthApiActions.loginFailure, (state, { error }) => ({
      // ...state, comment out the spread
      error,
      ending: false, // <--- 'pending' with typo here - missed letter 'p'.
     // Error would be provided for this case, because State interface is not fulfilled.
    })),

[ ] Bugfix
[x] 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:

Closes # #1724

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

[ngrxbot]

Preview docs changes for 3f2befd at https://previews.ngrx.io/pr1746-3f2befd/

[brandonroberts]

The creation of reducers is very nice, and as you mentioned, having to add more explicit type checking on each function is unfortunate. Both that and having to wrap the reducers for each feature feels like a step back in ease of use upfront even if you gain a less verbose way of creating a reducer function.

[ngrxbot]

Preview docs changes for dd3d865 at https://previews.ngrx.io/pr1746-dd3d865/

[brandonroberts]

@alex-okrushko is this initial set of work ready to merge?

[alex-okrushko]

is this initial set of work ready to merge?

This one uses InjectionToken instead of passing reducers fn directly
Also, this one has on instead of when that you recommended. I'm unsure about that naming still. I also considered case, that should smoother the transition... by I like the brevity of on. Thoughts? @timdeschryver @MikeRyanDev

[brandonroberts]

is this initial set of work ready to merge?

This one uses InjectionToken instead of passing reducers fn directly
Also, this one has on instead of when that you recommended. I'm unsure about that naming still. I also considered case, that should smoother the transition... by I like the brevity of on. Thoughts? @timdeschryver @MikeRyanDev

We can land with the token now, and follow-up with the fn if you prefer.
I'd rather stay away from case as a reserved word. I like when because it provides more context. We don't refer to actions as "on this occurrence", but more of "when this event happens".

[ngrxbot]

Preview docs changes for 5704f62 at https://previews.ngrx.io/pr1746-5704f62/

[alex-okrushko]

FYI,
(): State => ({ showSidenav: false })
is solved with
state => ({ showSidenav: false })

state argument is unused (which is unfortunate) but helps with inference.

[ngrxbot]

Preview docs changes for 0bb7409 at https://previews.ngrx.io/pr1746-0bb7409/

[timdeschryver]

My vote goes to using on, mostly because some state machines I've worked with also use on as trigger.

[timdeschryver]

How would we feel about having the initial state as first argument?

It would allow to skip the array, but perhaps it's useful to have the distinction between the two?

export const reducer = createReducer<State>(
  initialState,
  on(AuthApiActions.loginSuccess, (state, { user }) => ({ ...state, user })),
  on(AuthActions.logout, () => initialState)
);

Implementation:

export function createReducer<S>(
  initialState: S,
  ...ons: { reducer: On<S>; types: string[] }[]
): Reducer<S> {
  const map = new Map<string, On<S>>();
  for (let on of ons) {
    for (let type of on.types) {
      map.set(type, on.reducer);
    }
  }
  return function(state: S = initialState, action: Action): S {
    const reducer = map.get(action.type);
    return reducer ? reducer(state, action) : state;
  };
}

[alex-okrushko]

How would we feel about having the initial state as first argument?

I thought about that as well, but the Array.prototype.reduce has it as a last argument, so I left it there. Definitely something that we can discuss.

[brandonroberts]

As an additional note, you won't be able to use createReducer with a single reducer in a feature such as StoreModule.forFeature('feat', reducer) without wrapping it there also.

export function reducer(state, action) {
  return createReducer(...);
}

StoreModule.forFeature('feat', reducer)

[alex-okrushko]

I like when because it provides more context. We don't refer to actions as "on this occurrence", but more of "when this event happens".

We want to treat Actions like Events... In DOM, events listeners use 'on' prefix, so that's another ➕for on.

As an additional note, you won't be able to use createReducer with a single reducer

Will look into that as well.

[brandonroberts]

@alex-okrushko sounds like on is the better choice there. You don't have to do anything about the forFeature. With AOT that's just the way it is right now.

[brandonroberts]

LGTM other than Tim's comments

[ngrxbot]

Preview docs changes for c3816d4 at https://previews.ngrx.io/pr1746-c3816d4/

[ngrxbot]

Preview docs changes for be5f4d8 at https://previews.ngrx.io/pr1746-be5f4d8/

[alex-okrushko]

Should be ready to go 😀

[alex-okrushko]

@timdeschryver @brandonroberts
I'd like to revise this

How would we feel about having the initial state as first argument?

I was trying to remove the need to provide the generic for the createReducer function - and making the initialState to be the first parameter makes the rest of the on functions correctly infer the type State.

I'd like to change it to the following now:

export const reducer = createReducer(
  initialState,
  on(AuthApiActions.loginSuccess, (state, { user }) => ({ ...state, user })),
  on(AuthActions.logout, () => initialState)
);

[realtomaszkula]

As an additional note, you won't be able to use createReducer with a single reducer in a feature such as StoreModule.forFeature('feat', reducer) without wrapping it there also.

export function reducer(state, action) {
  return createReducer(...);
}

StoreModule.forFeature('feat', reducer)

I believe it would be beneficial if this was also included in the docs as an AoT comment.

[brandonroberts]

Will you leave a comment about this in #1762?

[realtomaszkula]

Will do :)

[morenoisidro]

Hi @alex-okrushko ! Would mind giving a little background on why this change was made? When would you suggest using the injection token approach over the standard one?

Thanks!