Adjust TypeScript definition for places where actions may not be dispatched

[nweber-gh]

As discussed in issue #243 actions are not always dispatched from loops. This PR defaults the Action generic to never in the various spots where an action may not be dispatched.

[laszlopandy]

Thanks for working on this!
I haven't tried this out in my apps yet (but I plan to). For now I only have one comments after reading your diff.

I don't think we have to give a default of never value for Cmd.run. TypeScript allows you to give multiple type definitions for the same function. For example, the following tells TypeScript to first consider the first function (ie if there aren't any action handlers, use RunCmd<never>):

  export function run(
    f: Function,
    options?: {
      args?: any[];
      forceSync?: boolean;
      testInvariants?: boolean;
    }
  ): RunCmd<never>;

  export function run<A extends Action, B extends Action>(
    f: Function,
    options?: {
      args?: any[];
      failActionCreator?: ActionCreator<A>;
      successActionCreator?: ActionCreator<B>;
      forceSync?: boolean;
      testInvariants?: boolean;
    }
  ): RunCmd<A>;

[nweber-gh]

Good call on the overloads, that is a better idea.

[laszlopandy]

Right "overloads" -- I knew there was a word for it that I was forgetting!

Anyway this was just my first attempt at crafting an overload. Perhaps we can improve it by making more which are more specific. I'm not sure how to determine what will work best; it's really just trial and error with these kind of overloads.

[nweber-gh]

@laszlopandy I took a stab at the overloads. Let me know what you think! I agree that it'll take some iterating to get this right.

[nweber-gh]

@laszlopandy I added this so that you can optionally delineate between the set of actions that a reducer consumes vs the set of actions that will be dispatched via loops. I think that this is needed since the set of actions being dispatched may not match, or even be a subset of, the actions being consumed. The Loop actions default to the consumed actions though, so this remains backwards compatible. Thoughts?

[bdwain]

i think we should consider #247 when changing LoopReducer

[nweber-gh]

Did a push with the default action enforcement discussed in #247

[nweber-gh]

@bdwain @laszlopandy As I've been working through this, there's a fundamental thing that I'm wondering about.. Do we need to enforce type checking on actions emitted via Redux Loop? i.e., do we need to force the user to enumerate the types of actions that various commands can emit?

export type Loop<S, A extends Action = never> = [
  S,
  CmdType<A>
];

export interface LoopReducer<
  S,
  ReducerActions extends Action = AnyAction,
  LoopActions extends Action = never
> {
  (
    state: S | undefined,
    action: WithDefaultActionHandling<ReducerActions>,
    ...args: any[]
  ): S | Loop<S, LoopActions>;
}

Right now the PR has the slight alternation to master above, which indicates that a loop reducer returns a loop that is a set of known actions. So one might have:

type ReducerActions = Action<'reducerAction1'>;
type LoopActions = Action<'loopAction1'>;

const myReducer: LoopReducer<FooState, ReducerActions, LoopActions> = (state, action) => {
  case "reducerAction1":
    return loop(state, Cmd.action('loopAction1'));
  default:
    return state;
}

And if you made a change..

const myReducer: LoopReducer<FooState, ReducerActions, LoopActions> = (state, action) => {
  case "reducerAction1":
    return loop(state, Cmd.list([Cmd.action('loopAction1'), Cmd.action('differentAction')]));
  default:
    return state;
}

You would end up with an error since differentAction is not in the LoopActions definition.

I'm not sure that we need to be enforcing this. It doesn't really matter what actions Loop emits. It could be any action in the system and I don't see why we need to force the user to enumerate the types of actions Loop cares about.

I do think we should enumerate the types of actions handled, since not handling an expected action is probably a bug.

I also think we need to specify action information where we have action creators arguments (such as RunCmd) so that we can be type safe in providing the right types of arguments.

But.. It feels overly verbose to force the user to enumerate the types of actions that Loop is emitting into the Redux system.

If we dropped this enforcement, we could dramatically simplify the type system. Especially since we have commands that take multiple actions which makes doing the CmdType union pretty difficult, and really complicates the types.

Thoughts?

[bdwain]

I think that makes sense @nweber-gh

[nweber-gh]

Did a push with the proposed changes. Tests are clean. I also added actual linting for the TypeScript stuff, since the linter was all confused.