Docs: Add more explanation that you can only have one distinct action type per on function with createReducer

[jsgoupil]

It is possible to have 2 effects looking at one action. But it seems impossible to have 2 ons looking at one action.

platform/modules/store/src/reducer_creator.ts

Line 189 in b07ae4e

map.set(type, on.reducer);

It is not obvious and it will override my previous action reducer.

After thinking about it, a selector combining 2 variables will do the job, but it's just not obvious and it does fail quite silently!

[timdeschryver]

If you mean that your using the same action in one reducer, then yes you're right (this also true for a reducer with switch cases).

const reducer1 = createReducer(
  0, 
  on(addCount, state => state + 1), 
  on(addCount, state => state + 1)
);

If you listen to the same action in multiple createReducers then the answer is no, both reducers will listen to the action.

const reducer1 = createReducer(
  0, 
  on(addCount, state => state + 1)
);
const reducer2 = createReducer(
  0, 
  on(addCount, state => state + 1)
);

In the example above, both reducers will react to the action and update there state.

[jsgoupil]

Thanks for confirming. It's just not intuitive with the "on" keyword that we know for events coming from multiple frameworks.

I was actually doing:

const reducer1 = createReducer(
  initialState, 
  on(addCount, state => state.adds + 1), 
  on(suctractCount, state => state.substracts - 1),
  on(addCount, subtractCount, state => {
    if (state.adds > 0 && state.substracts < 0) {
        return { ... state, hasRun: true };
    }

    return state;
  })
);

Granted, this example is quite contrived and can be converted to a selector like I mentioned earlier. But, because it is not explained and fail silently, it is not simple to debug.

[jtcrowson]

I agree, we all know how switch statements will work, but when using the createReducer method, it's not intuitive. Two options I can think of:

1. Add documentation to the createReducer function.
2. Add a console warning in development mode:

for (let on of ons) {
    for (let type of on.types) {
      if (isDevMode() && map.has(type)) {
        console.warn('message with remediation step')
      }
      map.set(type, on.reducer);
   }
}

[timdeschryver]

@jtcrowson unfortunetaly we can't add a dev check - #1837

[jtcrowson]

@timdeschryver is there any way to infer whether the app is in production mode?

[jtcrowson]

Ah okay got it, it's because the reducers get created before enableProdMode() is called. That's because the reducers are just constants in a plain .ts file. However, isDevMode() can be used when the code is not executed until after the app is bootstrapped, which is the case in my issue that contributes error logging for selecting missing feature states: #1897 (btw, shameless plug, I'd love your thoughts there @timdeschryver 😄)

[timdeschryver]

Yes exactly @jtcrowson, I'm not sure if there's another way to know if it's a prod build or not. This can be added to the documentation, like you proposed.

[phillipCouto]

Does it matter if prod or dev. It is an error. I think it is justified to print the message so it can be resolved.

[alex-okrushko]

Maybe we should indeed have the error in regardless prod or dev.

Someone on my team ran into a similar issue - while transitioning onto new Action Creators, they copy-pasted and duplicated the action type by accident. They were very puzzled why "order of on functions mattered" not realizing that under the hood reducer was mapping to the same type.

[timdeschryver]

What about wrapping the isDevMode check in a try catch block?
If we get an error during the AOT build we can assume it's a prod build.

[jtcrowson]

@timdeschryver That's an interesting approach.

Also note that this issue is now explained in the docs (#1980).

[jordanpowell88]

I agree that #1980 explains this more clearly now. I think it would be valuable to have a console error throw in @alex-okrushko example where the user accidentally duplicates.

[timdeschryver]

Fyi, I'm listing some typescript rules that are useful (in my opinion) with NgRx.
It has a rule exactly for this case, for more info see ngrx-tslint-rules

[zhaosiyang]

@timdeschryver @alex-okrushko
I also ran into this problem today.
I want to share my usage case and think maybe it's better to allow duplicate actions.
Basically I was trying to create a higher order reducer to enhance a base reducer to globally handle some loading logic (loading/success/error)

export function withLoadable({loadAction, successAction, errorAction}) {
  return (initialState, ...ons) => createReducer(
      initialState,
      on(loadAction, state => ({...state, loading: true, success: false, error: null})),
      on(successAction, state => ({...state, loading: false, success: true, error: null})),
      on(errorAction, (state, action) => ({...state, loading: false, success: false, error: (action as any).error})),
      ...ons,
  );
}

And I use it like this way

const userReducer = withLoadable({
  loadAction: getUerAction,
  successAction: getUserSuccessAction,
  errorAction: getUserErrorAction,
})(
  createInitialUser(),
  on(getUserSuccessAction, (state, action) => ({...state, user: action.user}))
)

However, the getUserSuccessAction overwrites the previous reducer.

In my opinion, it doesn't really make sense to overwrite (it's error-prone this way). Instead, it should incrementally apply reducers regardless of the action type.

I have experimented by changing the source code to the following and it works very well.

export function createReducer<S, A extends Action = Action>(
  initialState: S,
  ...ons: On<S>[]
): ActionReducer<S, A> {
  const map = new Map<string, ActionReducer<S, A>[]>();
  for (let on of ons) {
    for (let type of on.types) {
      if (map.has(type)) {
        const existingReducers = map.get(type);
        map.set(type, [...existingReducers, on.reducer]);
      } else {
        map.set(type, [on.reducer]);
      }
    }
  }

  return function(state: S = initialState, action: A): S {
    const reducers = map.get(action.type);
    return reducers && reducers.length > 0 ? reducers.reduce((previousState, reducer) => reducer(previousState, action), state) : state;
  };
}

Happy to discuss. :)