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

fix(types): Added flow type definitions for await and awaitPromises. #520

Closed

Conversation

critocrito
Copy link

Summary

Using most with function chaining doesn't type check using flow when using awaitPromises with the following error: Cannot call fromEvent(...).map(...).awaitPromises because property awaitPromises is missing in Stream. The fix adds a type definition for the await and awaitPromises property and allows flow to type check.

I saw that the original type definitions were removed in #494. From my understanding flow doesn't require a type definition for this. But since I'm fairly new to flow I might be missing something. I use flow version 0.73.

@briancavalier
Copy link
Member

Hi @critocrito. The problem is that the awaitPromises method should only be callable when this is Stream<Promise<A>>. Unfortunately, while Flow has good inference of this, it lacks the capability to specify constraints on this. That means Flow would silently hide a mistake like:

const s1 = most.of(Promise.resolve(1))
const s2: Stream<string> = s1.awaitPromises() // Ideally would fail, but doesn't

Since one goal of using a type system should be to prevent such mistakes, we removed the method type definition. It will be great if Flow adds support for this constraints. Please voice your support here: facebook/flow#452

As a workaround, you can use the function version of awaitPromises, which does provide the proper constraint. You can still have a fluent API by using thru:

fromEvent(...).map(...).thru(awaitPromises)

Hope that helps!

@critocrito
Copy link
Author

Hey @briancavalier, thanks a lot for your quick and informative response. I learned a thing about flow and a thing about most. The workaround solves my problem.

@briancavalier
Copy link
Member

Happy to help, @critocrito.

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.

2 participants