Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved flow typing (alternate version) #1827

Merged
merged 11 commits into from
Jan 21, 2019
Merged

Improved flow typing (alternate version) #1827

merged 11 commits into from
Jan 21, 2019

Conversation

xaviergonz
Copy link
Contributor

@xaviergonz xaviergonz commented Dec 4, 2018

  • Added unit tests
  • Updated changelog
  • Updated docs (either in the description of this PR as markdown, or as separate PR on the gh-pages branch. Please refer to this PR). For new functionality, at least API.md should be updated
  • Added typescript typings
  • Verified that there is no significant performance drop (npm run perf)

This PR is kind of like #1825 but with some improvements, since the other PR forces the user give the type of return types AND arguments.

This pr:

  • Argument and return types will be correctly inferred automatically. The only exception is when the last returned value is a Promise, but in this case the TS user can write this instead (this is a very rare case anyway):
const x = yield Promise.resolve(2);
return x as number;
  • Flow will now use as iterator the standard IterableIterator type rather than its custom one

Additionally:

  • Adds tslint-config-prettier to disable tslint rules covered by prettier already
  • Removes some unneeded rules (due to prettier usage) from tslint.json

Some examples

// type is correctly inferred as act(x: number, y: string): CancellablePromise<5>
const act = flow(function*(x: number, y: string) {
    yield Promise.resolve()
    return 5
})

// type is correctly inferred as act(x: number, y: string): CancellablePromise<void>
const act = flow(function*(x: number, y: string) {
    yield Promise.resolve()
})

// type is correctly inferred as act2(x: number, y: string): CancellablePromise<number>
CancellablePromise<void>
const act2 = flow(function*(x: number, y: string) {
    const x: number = yield Promise.resolve(2)
    return x;
})

@xaviergonz
Copy link
Contributor Author

xaviergonz commented Dec 4, 2018

Future: once TS implements Partial type argument inference (microsoft/TypeScript#26349) then "arguments" type could be set to be inferred and then the user would be able to only specify the return type (in this case it would only still be needed for promises) and castFlowReturn could become deprecated.

E.g.

const act2 = flow<number, _>(function*(x: number, y: string) {
    yield Promise.resolve()
    return Promise.resolve(2)
})

Still I think the use of castFlowReturn is easier to grasp.

@coveralls
Copy link

coveralls commented Dec 4, 2018

Coverage Status

Coverage increased (+0.6%) to 94.301% when pulling 949b5e8 on better-flow-ts into c74c9f8 on master.

@mayorovp
Copy link
Contributor

But how nested promise even possible?..

@xaviergonz
Copy link
Contributor Author

In case the async action wants to return a promise as result, but it should be a really really rare use case. Still supported though

@mayorovp
Copy link
Contributor

But this is not supported by runtime!

> new Promise(resolve => resolve(Promise.resolve(42)))
< Promise {<resolved>: 42}

@xaviergonz
Copy link
Contributor Author

ahh, so when a flow actually returns a promise it waits for it and returns its result!
I'll make changes then

@xaviergonz
Copy link
Contributor Author

fixed and updated the top comment

@xaviergonz
Copy link
Contributor Author

Another option of course is just to remove castFlowReturn and force typescript users to yield over it and return the yielded result (now that I know that actually returned promises are awaited first)

@xaviergonz
Copy link
Contributor Author

Removed castFlowReturn as the user can just write instead

const x = yield Promise.resolve(2);
return x as number;

@mweststrate
Copy link
Member

LGTM, Thanks!

@mweststrate mweststrate merged commit 3186d2a into master Jan 21, 2019
@mweststrate
Copy link
Member

Released in 4.9.0 / 5.9.0

@danielkcz danielkcz deleted the better-flow-ts branch August 19, 2019 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants