Make Cmd.run typesafe #222

domarmstrong · 2019-08-09T17:10:34Z

I notice there is a stale PR that tackles the same problem.

This PR goes a step further with argument inference while still allowing the special symbols to be used if the fn passed has arguments that would match the types of Dispatch or GetState (although I can't distinguish between them as the type symbol === symbol)

bdwain · 2019-08-18T05:58:45Z

index.d.ts

@@ -96,6 +96,9 @@ declare namespace Cmd {
  export const dispatch: symbol;
  export const getState: symbol;
  export const none: NoneCmd;
+  export type Dispatch = <A extends Action>(a: A) => void;


dispatch technically returns a promise

A promise of the action or void, ? <A extends Action>(a: A) => Promise<A>. The redux typing returns just the action:

export interface Dispatch<A extends Action = AnyAction> { <T extends A>(action: T): T }

Why do we define and export Dispatch in here at all? Let's just import it from Redux.

@domarmstrong @laszlopandy any thoughts on this? is the reason we don't just reuse the redux dispatch type because the return type is different?

and @domarmstrong i think i forgot to respond to your original comment months ago. it actually returns a promise with the new state object.

I think we should have a different type because the Redux type definitions show that dispatch returns an action, but in redux-loop we return a Promise<Action>. I think this warrants making a new type, but yes we should be consistent about this.
Also I never understood why dispatch would return anything in redux. If someone knows, I would be glad to understand it.

bdwain · 2019-08-18T06:04:06Z

Is any of this using a new typescript feature that requires a version we don't currently require, or is it a breaking change at all? I think we currently require at least 2.8, but I'm not too familiar with typescript as I don't use it myself.

bdwain · 2019-08-18T06:04:36Z

index.d.ts

@@ -115,16 +118,30 @@ declare namespace Cmd {
    args?: any[]
  ): MapCmd<A>;

-  export function run<A extends Action, B extends Action>(
-    f: Function,
+  type InferArgs<T> = T extends (...args: infer T) => any ? T : never;


what exactly does infer do here?

It basically says from this type this is the argument I want to infer my variable T to.

Its a conditional type that only matches the left hand size, its a bit of a quirky typescript syntax.

Simplified examples:

type NumberName<T> = T extends number ? 'number' : 'notNumber'; type N = NumberName<2>; // 'number' < literal type S = NumberName<'a'>; // 'notNumber' < literal

Then add never:

type NumberName<T> = T extends number ? 'number' : never; type N = NumberName<2>; // 'number' < literal type S = NumberName<'a'>; // never;

Then add infer;

type Args<T> = T extends (...args: infer A) => any ? A : never; type A = Args<(a: number, b?: string) => void>; // [number, (string | undefined)]

Hopefully that makes sense?

Is this not the same as TypeScript's built-in Parameters?
See lib.es5.d.ts:

/** * Obtain the parameters of a function type in a tuple */ type Parameters<T extends (...args: any) => any> = T extends (...args: infer P) => any ? P : never;

Yes, thanks. I've updated to use Parameters

domarmstrong · 2019-08-28T11:55:09Z

Is any of this using a new typescript feature that requires a version we don't currently require, or is it a breaking change at all? I think we currently require at least 2.8, but I'm not too familiar with typescript as I don't use it myself.

Yes it does require ^3.0.1 because of the generic rest parameter https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-0.html#generic-rest-parameters. I don't think its possible to infer the argument before that, but it is one of the more useful bits.

The version included in the package is ^3.2.2 though ¯_(ツ)_/¯

laszlopandy · 2020-02-17T11:08:46Z

index.d.ts

+    [K in keyof T]: T[K] extends Dispatch | GetState
+      ? typeof dispatch | typeof getState | T[K]
+      : T[K];
+  }


To be honest I didn't think that the type system could full express the Cmd.run API! I'm glad to learn that TypeScript is this powerful, but if we have to resort to this much magic, maybe we should revisit this API and see if it can be simplified first.

bdwain · 2020-02-25T01:44:23Z

@laszlopandy can you review this? Since it requires typescript v3, it sounds like it'll be a breaking change to introduce. How widespread is typescript v3 usage? Is it a difficult upgrade from v2?

lorefnon · 2020-03-25T09:27:05Z

TS Upgrades are generally smooth, and v3 was released several months ago, so I don't anticipate updating to TS v3 to be a major issue for people. We have been updating to every minor version sequentially in <1 day over the last two years.

But if the API significantly changes to make it easier to typecheck that can be a larger breaking change and a hurdle towards upgrading.

I think it would be good if this change is merged and any API changes (if needed) be taken up in separate breaking changes.

bdwain · 2020-03-26T04:20:51Z

@lorefnon awesome. Then I think we can just do a v6 to release this. There is another breaking change I want to release anyway

laszlopandy · 2020-04-16T06:51:53Z

index.d.ts

-      failActionCreator?: ActionCreator<A>;
-      successActionCreator?: ActionCreator<B>;
+      args?: ArgOrSymbol<Parameters<Func>>;
+      failActionCreator?: (error: Err) => FailAction;


A promise can fail with error: any. It is not always an Error. To be honest, I would leave this as any. In TypeScript Promises cannot specify the error type, so it might not be worth it to specify an error type here.

I've updated this, it seems sensible

laszlopandy · 2020-04-16T07:17:32Z

@domarmstrong

(although I can't distinguish between them as the type symbol === symbol)

Is this still true? Does this mean if you switch the arguments accidentally it doesn't give a type error?

domarmstrong · 2020-04-17T12:08:28Z

@domarmstrong

(although I can't distinguish between them as the type symbol === symbol)

Is this still true? Does this mean if you switch the arguments accidentally it doesn't give a type error?

No, I've fixed this. It should correctly error now if you use the wrong symbol, which I've tested in the loopReducer.ts file.

-    [K in keyof T]: T[K] extends Dispatch | GetState
-      ? typeof dispatch | typeof getState | T[K]
-      : T[K];
+    [K in keyof T]: T[K] extends GetState
+      ? typeof getState
+      : T[K] extends Dispatch
+        ? typeof dispatch
+        : T[K];

laszlopandy · 2020-04-17T12:21:03Z

Thanks @domarmstrong in that case I suggest we merge this now and deal with any new issues when we prepare for the next release. Thanks for your work on this.

(FYI: it's not necessary to force push to keep a single clean commit; we squash merge PRs in this repo).

index.d.ts

domarmstrong force-pushed the master branch from bec0601 to 9a2d316 Compare August 12, 2019 08:24

bdwain reviewed Aug 18, 2019

View reviewed changes

aeirola mentioned this pull request Aug 19, 2019

feat(ts): improve run signature typings #216

Closed

laszlopandy reviewed Feb 17, 2020

View reviewed changes

laszlopandy mentioned this pull request Mar 2, 2020

Improving getState and dispatch usage in Cmd.run #233

Open

bdwain mentioned this pull request Mar 25, 2020

Cmd.map() wraps actions send via dispatch() #218

Closed

domarmstrong force-pushed the master branch from 9a2d316 to 45ef7f1 Compare March 26, 2020 08:23

laszlopandy reviewed Apr 16, 2020

View reviewed changes

Make Cmd.run typesafe

dda4f46

domarmstrong force-pushed the master branch from 45ef7f1 to dda4f46 Compare April 17, 2020 12:01

laszlopandy merged commit 3ecf576 into redux-loop:master Apr 17, 2020

nweber-gh reviewed Apr 17, 2020

View reviewed changes

index.d.ts Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Cmd.run typesafe #222

Make Cmd.run typesafe #222

domarmstrong commented Aug 9, 2019

bdwain Aug 18, 2019

domarmstrong Aug 28, 2019

nweber-gh Apr 15, 2020

bdwain Apr 17, 2020

laszlopandy Apr 17, 2020

bdwain commented Aug 18, 2019

bdwain Aug 18, 2019

domarmstrong Aug 28, 2019

laszlopandy Feb 17, 2020

domarmstrong Mar 26, 2020

domarmstrong commented Aug 28, 2019 •

edited

laszlopandy Feb 17, 2020 •

edited

bdwain commented Feb 25, 2020

lorefnon commented Mar 25, 2020

bdwain commented Mar 26, 2020

laszlopandy Apr 16, 2020

domarmstrong Apr 17, 2020

laszlopandy commented Apr 16, 2020

domarmstrong commented Apr 17, 2020

laszlopandy commented Apr 17, 2020

Make Cmd.run typesafe #222

Make Cmd.run typesafe #222

Conversation

domarmstrong commented Aug 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdwain commented Aug 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domarmstrong commented Aug 28, 2019 • edited

laszlopandy Feb 17, 2020 • edited

Choose a reason for hiding this comment

bdwain commented Feb 25, 2020

lorefnon commented Mar 25, 2020

bdwain commented Mar 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laszlopandy commented Apr 16, 2020

domarmstrong commented Apr 17, 2020

laszlopandy commented Apr 17, 2020

domarmstrong commented Aug 28, 2019 •

edited

laszlopandy Feb 17, 2020 •

edited