the-dr-lazy / deox

Functional Type-safe Flux Standard Utilities
https://deox.js.org
MIT License
206 stars 12 forks source link

Make createActionCreator simpler by removing `resolve` callback from executor #122

Open the-dr-lazy opened 5 years ago

the-dr-lazy commented 5 years ago

From TypeScript 3.0 we are able to infer function's parameters with their names. So now we can remove resolve callback from executor in createActionCreator to make the API simpler.

Example:

// (name: string) => { type: "FETCH_TODOS"; }
createActionCreator("FETCH_TODOS");

// (name: string) => { type: "ADD_TODO"; payload: string; }
createActionCreator("ADD_TODO", (name: string) => ({ payload: name }));

There is one caveat point that has been encountered in #108 and that's type property in return type of callable (2nd argument of proposed createActionCreator). One possible solution for the mentioned problem which I think is ideal is to set the type of type property to undefiend.

// Types of property 'type' are incompatible.
// Type 'string' is not assignable to type 'undefined'.ts(2345)
createActionCreator("ADD_TODO", (name: string) => ({ type: 'MISTAKE', payload: name }));

We can call the callable's return type TypelessAction as it's type should be undefined.

the-dr-lazy commented 5 years ago

@LouizFC @Haaxor1689 @rlesniak can we have you guys here?

the-dr-lazy commented 5 years ago

I created a codesandbox as a demo for the proposed API.

the-dr-lazy commented 5 years ago

I clicked "close issue" button by mistake. :D

LouizFC commented 5 years ago

@thebrodmann I think that the current API is fine and removes the boilerplate of setting error: true on Error payloads.

But also I am not agains't it. The only "resistance" I have on this change is by my experience with #106 . I found very difficult to extract and modify return types correctly, but if you get it right I have no objections.

Edit: Also, backwards compatibility Edit2: Also, typescript playground now supports types from libraries, I think it is better than codesandbox when showcasing types.

the-dr-lazy commented 5 years ago

I think that the current API is fine and removes the boilerplate of setting error: true on Error payloads.

The only thing can be returned by callable is an object by payload and meta properties then we create an action by them.

Also, backwards compatibility

Yeah, unfortunately, this is not backward compatible but I think it makes the createActionCreator API more simple and also gives us an opportunity to make #108 API simpler. I will try to create a playground for createActionCreatorFactory with the proposed API.

typescript playground now supports types from libraries, I think it is better than codesandbox when showcasing types.

Ah, thank you for that, It's. This playground can show the proposed API.

the-dr-lazy commented 5 years ago

Also in this playground the extra properties are not allowed.

LouizFC commented 5 years ago

The proposed API looks fine, my only nitpick is: If you try to preview the arguments both on playground or IDE, the arguments for the function will show up as ...args: any[] (instead of, for example, name: string for ADD_TODO). I think it is a bug with the typescript language server, because you can't pass anything other than string to ADD_TODO, which is correct.

As I expressed earlier, I think we should not mess with createActionCreator because it will break backwards compatibility. I know it is a simple change, but it will require a huge amount of refactoring on my side.

the-dr-lazy commented 5 years ago

If you try to preview the arguments both on playground or IDE, the arguments for the function will show up as ...args: any[] (instead of, for example, name: string for ADD_TODO).

Actually it should work! Is your TypeScript version above 3.0?

Screen Shot 2019-09-17 at 1 18 19 PM Screen Shot 2019-09-17 at 1 19 16 PM

I think we should not mess with createActionCreator because it will break backwards compatibility.

Maybe we can be backward compatible. I will try to create a backward-compatible playground soon.

LouizFC commented 5 years ago

Actually it should work! Is your TypeScript version above 3.0?

It "works", when pressing Ctrl to see the types I get them right, also the types are applied correctly, but when you "request" the argument types, they come as ...args: any or empty, as I will show you below:

When you press Ctrl + Q on Webstorm WebStorm

When you press a new parenthesis in front of a function on the playground Playground

But this is not a blocking issue, probably just a bug in typescript language server. I will try to report it somehow

Maybe we can be backward compatible

Thank you. I think this is the best way

kotarella1110 commented 4 years ago

Hey @thebrodmann !

Also in this playground the extra properties are not allowed.

The error message caused by specifying extra properties feel strange.

I think it is better to change return value type of callable function from "{ payload?: any, meta?: any }" to never.

from:

callable?: (...args: TArguments) => TReturn & (Exclude<keyof TReturn, keyof TypelessAction> extends never ? TReturn : "{ payload?: any, meta?: any }"

to:

callable?: (...args: TArguments) => TReturn & (Exclude<keyof TReturn, keyof TypelessAction> extends never ? TReturn : never)

What do you think about this?

the-dr-lazy commented 4 years ago

The error message caused by specifying extra properties feel strange.

What makes you think so?

kotarella1110 commented 4 years ago

The ideal error and error message are as follows.

error:

スクリーンショット 2019-12-16 12 53 06

error message:

スクリーンショット 2019-12-16 12 51 25

The ideal return value type of callable function is TypelessAction , but the current return value type is { type: string; payload: string; } & "{ payload?: any, meta?: any }" I understand that the implementation is difficult because of the use of generics. Even if the return value type can be set to TypelessAction, it will be difficult to implement unless this issue(https://github.com/microsoft/TypeScript/issues/241) is fixed.

However, { type: string; payload: string; } & "{ payload?: any, meta?: any }" feel a little forced. never feel better.

the-dr-lazy commented 4 years ago
Type '{ type: string; payload: string; }' is not assignable to type 'never'.
the-dr-lazy commented 4 years ago
Type '{ type: string; payload: string; }' is not assignable to type '"{ payload?: any, meta?: any }"'.
the-dr-lazy commented 4 years ago

Type '{ type: string; payload: string; }' is not assignable to type '"{ payload?: any, meta?: any }"'.

I think this error message is more descriptive than the former one. BTW, I can't make a decision on it in this way. So let's vote on it. Please add a :+1: on the error message that you think is better.

the-dr-lazy commented 4 years ago

I will try to create a backward-compatible playground soon.

@LouizFC The backward-compatible version Playground.