[TypeScript 3.6+] flow() | Inferred Generator type is not assignable to IterableIterator

[nulladdict]

Feature request

Is your feature request related to a problem? Please describe.
Prior to TS 3.6 flow() yield return types would be typed as any due to typescript limitations. Those limitations are being addressed in TS 3.6 release as described in this iteration plan

Describe the solution you'd like
Full support for yield return types in flow()

Describe alternatives you've considered
If the full inference is not possible due to some TS limitations (I haven't completely dived into new generators typings) I wouldn't mind having to manually declare generic parameters in flow()

Additional context
I've seen some mentions in this PR, but I have not seen the actual issue for tracking this particular case

Are you willing to (attempt) a PR?

• Yes
• No

[RichiCoder1]

With some guidance I'd be interested in this. I believe this is a Mobx issue that MST inherits though?

[RichiCoder1]

Added issue in the mobx repo: mobxjs/mobx#2091

[fruitraccoon]

I see that the linked mobx issue mentioned above has been closed as the TS typing was deemed not powerful enough. However, I get compile errors when attempting to use TS 3.6.2 with MST flow because of the new types.

The code below shows me a typescript error (sorry, I couldn't find anywhere online to create a demo):

import { types, flow, applySnapshot } from 'mobx-state-tree';

function getItems() {
  return new Promise<string[]>(resolve => resolve(['one', 'two']));
}

const ItemsRepo = types
  .model('ItemsRepo', {
    items: types.array(types.string),
  })
  .actions(self => {
    const load = flow(function*() { // <-- error here under the `function` keyword
      const content: string[] = yield getItems();
      applySnapshot(self, {
        items: content,
      });
    });

    return { load };
  });

The error itself is:

(local function)(): Generator<Promise<string[]>, void, string[]>
Argument of type '() => Generator<Promise<string[]>, void, string[]>' is not assignable to parameter of type '() => IterableIterator<Promise<string[]>>'.
  Type 'Generator<Promise<string[]>, void, string[]>' is not assignable to type 'IterableIterator<Promise<string[]>>'.
    Types of property 'next' are incompatible.
      Type '(...args: [] | [string[]]) => IteratorResult<Promise<string[]>, void>' is not assignable to type '(...args: [undefined] | []) => IteratorResult<Promise<string[]>, any>'.
        Types of parameters 'args' and 'args' are incompatible.
          Type '[undefined] | []' is not assignable to type '[] | [string[]]'.
            Type '[undefined]' is not assignable to type '[] | [string[]]'.
              Type '[undefined]' is not assignable to type '[string[]]'.
                Type 'undefined' is not assignable to type 'string[]'.ts(2345)

The best workaround that I've found looks like this:

.actions(self => {
    const load = flow(function*(): Generator<Promise<any>, void, any> {
      const content: string[] = yield getItems();
      applySnapshot(self, {
        items: content,
      });
    });

This explicitly types the returned generator. The any as the third generic parameter fixes/hides the issue.

Perhaps there's a way to update flow so that this explicit typing isn't required? (ideally in a backward-compatible way)

[nulladdict]

Upon further research, new TS typings are indeed not flexible enough to provide strongly typed next(), since it would require to define relationships between yield'ed invocations, which is not possible (at least as of yet).

However, as @fruitraccoon mentioned we now get compile errors, since the new inferred Generator<…> type is not assignable to IterableIterator<…>, because of the ability to distinguish between yield and return types.

I believe this issue should be about compatibility with TS 3.6+, but I am not quite sure that there's a backward-compatible change to be made, so that's a real problem

[elektronik2k5]

Thanks for looking into it @nulladdict!
I'm getting the same errors and see two ways to proceed with TS@3.6 compatibility:

1. A simple breaking change (I'd assume semver major) which requires TS@3.6
2. Doing the same fix, but instead of mandating a breaking change utilize TS@3.1's typesVersions property for backwards compatibility (up to 3.1, obviously)

I think the second option is obviously better :)

[nulladdict]

Sorry for slacking on this. For now I've come up with this temporary solution:

import { flow as _flow } from 'mobx-state-tree'
import { FlowReturnType } from 'mobx-state-tree/dist/core/flow'

export type Flow = <T extends Promise<any>, R, Args extends any[]>(
    generator: (...args: Args) => Generator<T, R, T extends Promise<infer Y> ? Y : never>
) => (...args: Args) => Promise<FlowReturnType<R>>

export const flow = _flow as Flow

Good parts:

• Works with inferred Generator type
• TS now requires Promise to be yielded, which is required by flow() but wasn't type checked before
• TS now can infer something for each yielded "awaited" Promise

Bad part is that whatever TS infers has to be compatible with every single one of the yield calls.
In practice it turns into something like this:

flow(function*() {
    const x = yield Promise.resolve(42)
    const y = yield Promise.resolve('42')
    const z = yield Promise.resolve(true)
    return null
})

The return type is inferred correctly, but type for x, y and z is number | string | boolean.

Bottom line is, in case of having only one yield call TS works as expected, otherwise we get a union type, which is in my opinion still better than having any, since it's at least partially type checks.

[fruitraccoon]

Bottom line is, in case of having only one yield call TS works as expected, otherwise we get a union type, which is in my opinion still better than having any, since it's at least partially type checks.

At the moment, when there are multiple yields, I'm falling back to this approach so that it's easier to explicitly type the return variables

flow(function*(): Generator<Promise<any>, void, any> {
  const x: number = yield Promise.resolve(42)
  const y: string = yield Promise.resolve('42')
  const z: boolean = yield Promise.resolve(true)
  // do something with the return values that requires they are typed correctly

Without the Generator<Promise<any>, void, any>, TypeScript shows errors as it does not like trying to narrow the union type directly.

I'm keen to hear if anyone has a better approach?

[nulladdict]

I would argue, that falling back to Promise<any> is a shaky solution.
It is quite unsound, so you might forget to specify a type or specify a wrong type and TS would not complain about it.

Inferred union type is more verbose(you're forced to use "useless" type guard or as cast), but it is also more sound.
Consider this: (playground link)

type A = string | number
const x: A = /* ... */
const y: string = x as string // TS knows it could be done
const z: boolean = x as boolean // TS would complain here

In this example as cast is pretty safe, because starting at some version TS would complain about casting non-overlapping types.

Downside is you can still choose an incorrect type from a union, and TS would fail to notice: (playground link)

function* f(): Generator<string | number, void, string | number> {
    const x = (yield '42') as string // Safe, but ugly and a little awkward
    const y = (yield '42') as number // Unsound, ERROR at runtime, but OK in terms of casting
    const z = (yield '42') as boolean // Sound, error at compile time
}

For my case it is a trade-off I'm willing to take in favour of having any

[fruitraccoon]

Of course, any is the opt-out of type checking. It was just what we had before, so at least it wasn't going backwards ;)

I had totally forgotten to try using as casting - that's a much better idea. Thanks @nulladdict!

[RWOverdijk]

I'd gladly go back to any if it means this will keep working

[akphi]

you guys should check out mobx-utils actionAsync thanks to the great work of @xaviergonz! I could rewrite all of my flows using that pretty easily.

As for flow() post TS3.6, I think instead of Generator<Promise<any>, void, any> I would use Generator<Promise<unknown>, void, unknown> so that Typescript will force me to cast type.

[nulladdict]

@blacksteed232 thanks, that's actually exactly what I needed.
actionAsync is a little verbose, but I'll take extra task() call for TS compatibility

[thienmint]

@blacksteed232 How do you replace flows where it involves altering the tree? Such as below:

const x = flow(function* () => {
  const y = yield (Promise);
  self.attribute = y; // this still gives me error for altering the tree in non-actions after converting to actionAsync
}

[xaviergonz]

actionAsync is an alternative to mobx flows, not mobx state tree ones. To make them work with mobx state tree it would need its own particular implementation (same than there's a different implementation for flows in mobx).

That being said I'll cut a new version of mst with the fix for flow typings, but it will make ts 3.6 the new minimum version when using flows

[xaviergonz]

v3.15.0 released, please give it a try :)

How to use it:

    const M = types
        .model({
            title: types.string
        })
        .actions(self => ({
            setTitleAsync: flow(function* (newTitle: string) {
                yield delay(10)
                self.title = newTitle
                return 23
            }))
        })

const m = M.create({})
const n = await m.setTitleAsync("new title") // n is correctly inferred to be a number

The only thing that is still not quite working is getting the proper return type from yields, so those will still return any for now and should be type casted. e.g.

const whatever: string = yield somethingThatReturnsPromiseString()

[RWOverdijk]

@xaviergonz I'm now able to remove the icky typing and I can confirm it works. Thank you!

[ArmorDarks]

Am I correct that yield still results in any type?

For instance:

import { types, flow } from 'mobx-state-tree'

const User = types
  .model('User', {
    id: types.string
  })
  .actions((self) => ({
    getSuggestions: flow(function * () {
      const response = yield window.fetch(`http://localhost:3001/suggestions`) // results in any
      const suggestions = (yield response.json()) as string[]
      self.wishlist.items.push(...suggestions)
    })
  }))

response here yield to any.

It's impossible to address that issue as of right now without a forced typecasting, right?

[mweststrate]

.json() produces any itself, so I am not sure what else could be inferred here? Op wo 20 nov. 2019 11:35 schreef Serj Lavrin <notifications@github.com>:

…

Am I correct that yield still results in any type? For instance: import { types, flow } from 'mobx-state-tree' const User = types .model('User', { id: types.string }) .actions((self) => ({ getSuggestions: flow(function * () { const response = yield window.fetch(`http://localhost:3001/suggestions`) // results in any const suggestions = (yield response.json()) as string[] self.wishlist.items.push(...suggestions) return suggestions }) })) response here yield to any. It's impossible to address that issue as of right now without a forced typecasting, right? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1378?email_source=notifications&email_token=AAN4NBB4Y3IE64TFITDVMJ3QUUOHRA5CNFSM4IQAYWJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEERVW7Y#issuecomment-555965311>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBDIGFP3AYI66JWZA7LQUUOHRANCNFSM4IQAYWJQ> .

[nulladdict]

It's impossible to address that issue as of right now without a forced typecasting, right?

It is still somewhat impossible, you can try tricking TS into inferring union type of all yields as described in my comment above, but if you return one of the yielded results from the generator TS freaks out and gives up typing, falling back to any

edit: and even in union case you still have to use as casting or type assertions to distinguish between unioned types

[ArmorDarks]

.json() produces any itself, so I am not sure what else could be inferred
here?

It's not about response.json(), but about window.fetch() wich actually resolves to Promise<Response>

It is still somewhat impossible, you can try tricking TS into inferring union type of all yields as described in my comment above

That's kinda sad. And, if I'm correct, asyncTask from Mobx won't work with MST as it is?

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.

[mweststrate]

Linked PR released in 3.16.0