Adds return type parameter to Saga #2295

lourd · 2022-05-09T22:56:22Z

Q	A
Fixed Issues?	No existing issue
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?	👍
Tests Added + Pass?	Yes
Any Dependency Changes?	No

In my application I have a function runSaga that takes a saga as a parameter and returns the Task that the saga returns when it's begun. I want to get the proper type of the Task's result, but am not able to do so with the current Saga type definition.

function* sag() {
  return 42
}

type RunSaga = <T extends any[]>(
  saga: Saga<T>,
  ...args: T
) => Task

const result = runSaga(saga).result() // type of any

By adding TReturn to Saga and making Task accept its return type as a parameter, result will be inferred correctly

type RunSaga = <T extends any[], R>(
  saga: Saga<T, R>
  ...args: T
) => Task<R>

const result = runSaga(saga).result() // type of `number | undefined`

Here's a TypeScript playground I was using to play with the types

This is a non-breaking change since it's optional with a default of any.

The use case is effectively dependent on #2241, but these changes can be reviewed and merged separately.

changeset-bot · 2022-05-09T22:56:25Z

🦋 Changeset detected

Latest commit: 2d21a2e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
@redux-saga/core	Minor
@redux-saga/types	Minor
redux-saga	Minor
babel-plugin-redux-saga	Major
@redux-saga/simple-saga-monitor	Major
redux-real-world-example	Patch

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

lourd · 2022-05-09T23:00:24Z

Sorry for the double-ping @Andarist and @neurosnap, wasn't sure how to add reviewers. Thanks in advance for reviewing this 🙏

Andarist · 2022-05-10T09:14:12Z

packages/types/types/ts3.6/index.d.ts

@@ -1,6 +1,6 @@
 import { Action } from 'redux'

-export type Saga<Args extends any[] = any[]> = (...args: Args) => Iterator<any>
+export type Saga<Args extends any[] = any[], TReturn = any> = (...args: Args) => Iterator<any, TReturn>


Note for me (and other reviewers) - this change is unique to 3.6+ types because previous versions of the Iterator didn't accept more than one generic.

Andarist · 2022-05-10T09:14:27Z

Could you add type-level tests for this change?

lourd · 2022-05-10T12:58:00Z

👍 Done.

As part of doing that I also updated the types for SagaMiddleware#run and runSaga. It made sense to propagate the return type out to those functions and use them for tests, now that the generic Task changes have been merged.

lourd · 2022-05-12T23:03:21Z

What do you think @Andarist?

Andarist · 2022-05-13T15:00:50Z

packages/core/types/ts3.6/index.d.ts

@@ -166,7 +166,7 @@ export interface SagaMiddleware<C extends object = {}> extends Middleware {
   * @param saga a Generator function
   * @param args arguments to be provided to `saga`
   */
-  run<S extends Saga>(saga: S, ...args: Parameters<S>): Task
+  run<Args extends any[], TReturn>(saga: Saga<Args, TReturn>, ...args: Args): Task<TReturn>


This is a change in the declared signature. What does it give us? Can you come up with any real-life scenario?

I know some stuff about the difference between the two but I'd like to hear out your thoughts and motivation.

It is a change, but given it's just changing inferred generic type parameters, I don't think it's a breaking change. I don't think it's common for people to specify the saga's type, they just pass the saga.

I'm not sure what kind of description or specific use cases you're looking for beyond the example I gave in the PR description, and of course the tests showing that the task's type was not being inferred before.

In my application I have a function runSaga that takes a saga as a parameter and returns the Task that the saga returns when it's begun. I want to get the proper type of the Task's result, but am not able to do so with the current Saga type definition.

Please let me know if I can be clearer or if you think the tests don't demonstrate the improvement — getting a specific, inferred type for the task's return instead of any

But does it provide a meaningful improvement anywhere? Can we add a test that would "prove" that?

I'm confused, are you not seeing the two tests that are a part of this PR?

I see them but I don't see from them how exactly changing the generic signatures of run/runSaga have made them better.

What I mean is that to make them work it would probably be enough to just extract the return type like this:

type SagaReturnType<S extends Saga<any, any>> = S extends Saga<any, infer R> ? R : never // ... run<S extends Saga>(saga: S, ...args: Parameters<S>): Task<SagaReturnType<S>>

Ah I see — you understand the point and value of adding the return type to Saga, but you have a way you would prefer to implement this piece of it.

I don't have a preference between the two forms, the way I wrote it was just how I'd figured out how to do it.

Went ahead and changed it to your suggested form 👍

Andarist · 2022-06-12T19:40:30Z

packages/types/types/ts3.6/index.d.ts

-export type Saga<Args extends any[] = any[]> = (...args: Args) => Iterator<any>
+export type Saga<Args extends any[] = any[], TReturn = any> = (...args: Args) => Iterator<any, TReturn>
+
+export type SagaReturnType<S extends Saga<any, any>> = S extends Saga<any, infer R> ? R : never


There is a case in which we might get never back from this. I don't see any particular problems with this... but it's fine somewhat coincidentally. It's fine because the computed return type is only used within toPromise and result.

It's fine for toPromise as a never-returning iterator should, well, never complete - so the returned promise won't ever resolve and thus it doesn't quite matter.

It's fine for result because T is there included within a union with undefined, never is always eliminated from unions, so the only leftover will always be just undefined in such a case and that's correct.

This TS playground might show what kind of edge cases I was thinking about while reviewing this change.

I just wonder if we should have any tests to check against potential regressions here. 🤔 Maybe I'm overthinking this though.

It's also worth noting that T in Task is covariant and thus Saga<any, never> satisfies Saga<any, any>. It's always worth re-checking this because the same is not true for contravariant parameters: TS playground

Andarist · 2022-06-13T19:46:29Z

packages/core/types/ts3.6/runSaga.test.ts

@@ -7,7 +7,8 @@ declare const effect: StrictEffect
 declare const iterator: Iterator<any>

 function testRunSaga() {
-  const task0: Task = runSaga<{ foo: string }, { baz: boolean }, () => SagaIterator>(
+  // $ExpectType Task<void>
+  const task0 = runSaga<{ foo: string }, { baz: boolean }, Saga<undefined[], void>>(


It feels like you might have wanted to just provide no arguments:

Suggested change

const task0 = runSaga<{ foo: string }, { baz: boolean }, Saga<undefined[], void>>(

const task0 = runSaga<{ foo: string }, { baz: boolean }, Saga<[], void>>(

Tried that and got this error:

ERROR: 11:60 expect Compile error in typescript@3.7 but not in typescript@3.8. Fix with a comment '// Minimum TypeScript Version: 3.8' just under the header. Type 'Saga<[], void>' does not satisfy the constraint 'Saga<any[], any>'. Type 'any[]' is not assignable to type '[]'. Types of property 'length' are incompatible. Type 'number' is not assignable to type '0'.

So left as-is.

packages/core/types/ts3.6/runSaga.test.ts

Andarist

LGTM, let's just wait a day or two to see if @neurosnap has any additional thoughts on this one before merging this

neurosnap

Thanks for diving into this @Andarist -- I'm happy with the changes proposed here.

neurosnap · 2022-06-21T18:43:30Z

@lourd We are ready to merge, we just need a changeset. Could you please create one for this PR? Thanks!

lourd · 2022-06-21T21:12:34Z

@neurosnap can you point me towards directions on how to do that? I tried before but couldn't figure it out

neurosnap · 2022-06-21T21:27:48Z

If you run yarn changeset it should walk you through the process.

Also feel free to read more about it here https://github.com/changesets/changesets

neurosnap added the Typings label May 10, 2022

Andarist reviewed May 10, 2022

View reviewed changes

lourd requested a review from Andarist May 10, 2022 12:58

Andarist reviewed May 13, 2022

View reviewed changes

lourd requested a review from Andarist May 18, 2022 00:17

Andarist reviewed Jun 12, 2022

View reviewed changes

Andarist reviewed Jun 13, 2022

View reviewed changes

packages/core/types/ts3.6/runSaga.test.ts Outdated Show resolved Hide resolved

Andarist requested a review from neurosnap June 13, 2022 19:47

Andarist approved these changes Jun 13, 2022

View reviewed changes

Updates types to eliminate void-emitting channel pitfall

158b6c6

neurosnap approved these changes Jun 14, 2022

View reviewed changes

Creates changeset

2d21a2e

neurosnap merged commit bed4458 into redux-saga:master Jun 27, 2022

lourd deleted the saga-return-type branch June 27, 2022 17:05

lourd mentioned this pull request Aug 11, 2022

Recent improvements not published #2319

Closed

This was referenced Aug 13, 2022

Version Packages #2280

Closed

Version Packages #2322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds return type parameter to Saga #2295

Adds return type parameter to Saga #2295

lourd commented May 9, 2022 •

edited

Loading

changeset-bot bot commented May 9, 2022 •

edited

Loading

lourd commented May 9, 2022

Andarist May 10, 2022

Andarist commented May 10, 2022

lourd commented May 10, 2022

lourd commented May 12, 2022

Andarist May 13, 2022

lourd May 13, 2022

lourd May 13, 2022 •

edited

Loading

Andarist Jun 6, 2022

lourd Jun 6, 2022

Andarist Jun 11, 2022 •

edited

Loading

lourd Jun 11, 2022

Andarist Jun 12, 2022

Andarist Jun 13, 2022

lourd Jun 14, 2022

Andarist left a comment

neurosnap left a comment

neurosnap commented Jun 21, 2022

lourd commented Jun 21, 2022

neurosnap commented Jun 21, 2022

	const task0 = runSaga<{ foo: string }, { baz: boolean }, Saga<undefined[], void>>(
	const task0 = runSaga<{ foo: string }, { baz: boolean }, Saga<[], void>>(

Adds return type parameter to Saga #2295

Adds return type parameter to Saga #2295

Conversation

lourd commented May 9, 2022 • edited Loading

changeset-bot bot commented May 9, 2022 • edited Loading

🦋 Changeset detected

lourd commented May 9, 2022

Choose a reason for hiding this comment

Andarist commented May 10, 2022

lourd commented May 10, 2022

lourd commented May 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lourd May 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist Jun 11, 2022 • edited Loading

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

Andarist left a comment

Choose a reason for hiding this comment

neurosnap left a comment

Choose a reason for hiding this comment

neurosnap commented Jun 21, 2022

lourd commented Jun 21, 2022

neurosnap commented Jun 21, 2022

lourd commented May 9, 2022 •

edited

Loading

changeset-bot bot commented May 9, 2022 •

edited

Loading

lourd May 13, 2022 •

edited

Loading

Andarist Jun 11, 2022 •

edited

Loading