fix Flow typings for flow()

[vonovak]

This fixes flow typings for flow() which are now fundamentally broken. Flow() returns a cancellable promise (fixed) and a signature that is not valid was removed.

usage:

// @flow
import { flow, type CancellablePromise } from 'mobx';

export class FlowTest {
  promise: CancellablePromise<string>;

  constructor() {
    this.promise = this.fetchSomeString(2);
    this.promise.cancel();
  }

  fetchSomeString = flow<string, number>(function*(pageNumber: number) {
    try {
      const someString = yield this.fetchStringSomehow(pageNumber);
      return someString;
    } catch (error) {
      return '123';
    }
  });

  fetchStringSomehow = (pageNumber: mixed): Promise<string> => {
    console.log(pageNumber);
    return Promise.resolve('123');
  };
}

PR checklist - I think none of these apply for this PR.

• Added unit tests
• Updated changelog
• Updated /docs. For new functionality, at least API.md should be updated
• Added typescript typings
• Verified that there is no significant performance drop (npm run perf)

Feel free to ask help with any of these boxes!

The above process doesn't apply to doc updates etc.

[danielkcz]

I believe this is duplicate to #2160 :) @xaviergonz what do you think, something useful here?

[xaviergonz]

This seems to be a fix for flows too, but for "flow" (the facebook thingy) :)

That being said I think the declaration could be improved as

declare export function flow<R, Args extends any[]>(
  generator: (...args: Args) => Generator<any, R, any> | AsyncGenerator<any, R, any>
): (...args: Args) => CancellablePromise<R>

Like the TS one for the fix.

The one presented here (after the fix) has the same def duplicated twice and casts arguments as any[] rather than inferring them from the original function (though I'm not sure if facebook flow does support actually that).

Also in TS R | Promise<R> is not required in TS, but again I'm not an expert of facebook flow.

[xaviergonz]

Nevermind about the definition duplication part, I just saw the extra name argument :) The other things might still apply though.

[xaviergonz]

Wait, actually I don't think flows support a name argument at all, they seem to take the name from the inner generator function

[vonovak]

@xaviergonz

I don't think flows support a name argument at all, they seem to take the name from the inner generator function

if you're talking about (?)

declare export function flow<T>(
    name: string,
    fn: (...args: any[]) => T
): (...args: any[]) => Promise<T>

then I have removed it because it does not match the signature of how it's implemented. Not sure why it was there.

As for the improvements you suggested

• Args extends any[] - there is no such thing in Flow that I'm aware of

has the same def duplicated twice and casts arguments as any[] rather than inferring them from the original function (though I'm not sure if facebook flow does support actually that).

I'm not aware of such support. The best I can think of is having this - I'm in favor of adding this if you agree because it's more accurate.

declare export function flow<T>(
    fn: () => Generator<any, T | Promise<T>, any>
): () => CancellablePromise<T>

declare export function flow<T, A>(
    fn: (A) => Generator<any, T | Promise<T>, any>
): (A) => CancellablePromise<T>

declare export function flow<T, A, B>(
    fn: (A, B) => Generator<any, T | Promise<T>, any>
): (A, B) => CancellablePromise<T>

// and so on, until let's, say 4 params?

Also in TS R | Promise is not required in TS, but again I'm not an expert of facebook flow.

this is required in Flow

[xaviergonz]

if you're talking about (?)

declare export function flow<T>(
    name: string,
    fn: (...args: any[]) => T
): (...args: any[]) => Promise<T>

then I have removed it because it does not match the signature of how it's implemented. Not sure why it was there.

Yep, exactly :)

As for the improvements you suggested

• Args extends any[] - there is no such thing in Flow that I'm aware of

has the same def duplicated twice and casts arguments as any[] rather than inferring them from the original function (though I'm not sure if facebook flow does support actually that).

I'm not aware of such support. The best I can think of is having this - I'm in favor of adding this if you agree because it's more accurate.

declare export function flow<T>(
    fn: () => Generator<any, T | Promise<T>, any>
): () => CancellablePromise<T>

declare export function flow<T, A>(
    fn: (A) => Generator<any, T | Promise<T>, any>
): (A) => CancellablePromise<T>

declare export function flow<T, A, B>(
    fn: (A, B) => Generator<any, T | Promise<T>, any>
): (A, B) => CancellablePromise<T>

// and so on, until let's, say 4 params?

Shame, that seems like a good workaround (that's how it used to be in TS actually before they added support for that). I'd make it 8-10 args though.

Also in TS R | Promise is not required in TS, but again I'm not an expert of facebook flow.

this is required in Flow

Ahh shame. What about

Generator<any, T | Promise<T>, any> | AsyncGenerator<any, T | Promise<T>, any>

is that supported?

[vonovak]

Also in TS R | Promise is not required in TS, but again I'm not an expert of facebook flow.

why is that a shame? I mean, returning T or Promise are two different things, so Flow makes a distinction there.

the PR should be good for merge now

[xaviergonz]

LGTM, just to make sure, arg names are not required in flow, just the types?

[vonovak]

arg names are not required in flow, just the types?

yes, I tested this locally and it is working fine

[danielkcz]

What about MobX4, does it need same tweaks?

[vonovak]

@FredyC this feature works the same there afaik, so no

[danielkcz]

@vonovak Wait, so in MobX4 the types were already fixed in the past? Or what do you mean by that? Can you verify it, please?

[vonovak]

sorry, I misunderstood your comment, yes, you should cherry pick this for mobx 4 as well. When do you think this will go public?

[danielkcz]

@vonovak Well, I don't really understand what's going on here. Can you cherry-pick, please? :) We certainly won't publish till both branches are in sync.