Strong typecheck for take*, throttle and debounce effects when action…

[fyodore82]

… type is provided

Q	A
Fixed Issues?	Fixes #1543
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Any Dependency Changes?

Adds strong typecheck for take, takeLatest, takeLeading, takeEvery, throttle and debounce effects if type explicitly provided. For example,

interface ACTION1 {
    type: 'ACTION1'
}
take<ACTION1>('ACTION1'); // Only 'ACTION1' can be provided as first argument of take effect 

[changeset-bot]

🦋 Changeset is good to go

Latest commit: 96b1334

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Andarist]

I'm not a TS expert so I might be wrong in what I am going to say - let's discuss this.

While it's not possible right now (i think), but assuming it would be, would support for this solve your issue?

const ac: Action1 = yield take((ac: Action1) => ac.type === 'ACTION_1')

[fyodore82]

Presented line of code has several parts related to typing

1. Type, that yield returns. Change, I've proposed is not related to this. And I'm not sure that typing yield is possible with TypeScript. But again, it is not related
2. In sample, take takes predicate as first argument, Type of predicate's argument is just Action. It is not strictly typed because predicate will be called with every action. So only thing that is known about action is that it has type property of any type. Everything else should be checked in run-time and we should't relate on TypeScript.

[Andarist]

1. It's not supported with yield, but we can work around it by using yield* and wrappers around effects - that is right now typeable with TS@>=3.6.

Could you explain to me what it brings exactly for users? Especially for i.e. takeEvery effects - as for bare take this doesn't bring much, right? I would appreciate examples of code which is not possible or unsafe right now and how your changes improve the situation.

cc @aikoven @gilbsgilbs - would love you take a look at this as well

[fyodore82]

Proposed change solve following problem.
Suppose we have such type definition for Action1

    interface Action1 extends Action {
        type: 'ACTION1';
        payload: string;
    }

    const action1: Action1 = { type: 'ACTION1', payload: 'some payload' }

Above code creates interface Action1 with two properties. type property will have type ACTION1. This means, that action1.type may have only ACTION1 as value. It cannot be null, undefined, empty string or any other string. Only ACTION1. Such type allows using discriminated unions in TS and frequently used to define action types for Redux.

So when we're going to use effect we can strictly type it like below

    take<Action1>('ACTION1')
    takeLatest<Action1>('ACTION1', function *() {})

We've provided exact type for take and takeLatest effects, so assuming that proposed change is merged, TS will require that first argument of take or takeLatest effect be exactly ACTION1. Now first argument can be any string, number or Symbol which is incorrect.

This is all it did.

And it is not related to yield typing. And yield typing will not affect this change

[Andarist]

Please bear with my questions as I just want to understand this to get a better picture of all possible implications.

I still don't quite get why this would be needed for take as in its case it only brings a duplication, sort of. Yes - you can put an extra constraint on a passed argument, but I don't see how this is helpful. What value does it bring? Seems like extra validation, but in this case - what will validate your generic parameter 😉 ?

What I currently see in it is this:

acceptString<'foo'>('foo')

And this doesn't quite make sense to me.

As to take* - does this help to narrow down the type received by a saga parameter? Like without this parameter in the sample code beflow ac can't know about having .bar property?

// { type: 'FOO', bar: 42 }
takeEvery('FOO', ac => {})

[fyodore82]

I'm here to clarify everything :-)

For take my generic parameter will be validated by defined actions.
For example, I create file taskStore.ts to keep tasks list for my SPA. Inside this file I define several actions as interface, like this

interface REQUEST_TASKS extends Action { type: 'REQUEST_TASKS' }
interface RECEIVE_TASKS extends Action { type: 'RECEIVE_TASKS'; taskslist?: Taskslist; error?: string }
// And so on

Then I define saga like below

function* getTasks() {
    while (true) {
       yield take<REQUEST_TASKS>('REQUEST_TASKS'); 
       // and so on
    }
}

As type constant I can put either REQUEST_TASKS and RECEIVE_TASKS and nothing else. No more actions defined. All other interfaces, if present, will not be accepted, as type argument of take is required to extend Action. Than, with proposed changes, first argument of take must be exactly REQUEST_TASKS. Without proposed change, it can be any string.

For takeEvery and similar effects things get more interesting.
Consider code

takeEvery<RECEIVE_TASKS >('RECEIVE_TASKS ' (action) => {
    // action is o type RECEIVE_TASKS and has taskslist and error properties
}

Right now, takeEvery already correctly type action. This is implemented. But all take effect ignore type for first argument. Proposed change adds type checking for first argument.

Or we may write

takeEvery('RECEIVE_TASKS ' (action: RECEIVE_TASKS) => {
    // action is o type RECEIVE_TASKS and has taskslist and error properties
}

Here, TS will infer first type argument for takeEvery and with proposed changes will enforce type of first argument of takeEvery (in this example it is RECEIVE_TASKS)

[fyodore82]

Hi @Andarist , @aikoven @gilbsgilbs
if you have any questions or suggestions regarding this PR? I'm here to answer