Skip to content

Add Flow type definition #1007

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

Merged
merged 5 commits into from
Sep 4, 2016
Merged

Add Flow type definition #1007

merged 5 commits into from
Sep 4, 2016

Conversation

jamiebuilds
Copy link
Contributor

@jamiebuilds jamiebuilds commented Aug 17, 2016

Resolves #986

I wanted to test Flow against one of AVA's test files but it looks like the test suite isn't self-hosted and there doesn't appear to be any integration-like tests for the public api.

I would suggest doing something along those lines and testing both Flow and TypeScript against it, otherwise you'll end up with out of date type definitions like what has already happened with the TS definition and .always

@jamiebuilds
Copy link
Contributor Author

Btw the reason for this file's location and naming is so that Flow doesn't require any configuration in order to use the type definitions for AVA. npm install ava + require('ava') and you're done.

@sindresorhus sindresorhus changed the title Add flow type definition Add Flow type definition Aug 17, 2016
@sindresorhus
Copy link
Member

sindresorhus commented Aug 17, 2016

Awesome!

You also need to add this file to the files array in package.json

onFulfill?: (value: R) => Promise<U> | U,
onReject?: (error: any) => Promise<U> | U
): Promise<U>;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any plan to have this builtin in Flow? Kinda weird having to define it manually in every type definition.

@sindresorhus
Copy link
Member

Sidenote: Someone really needs to submit Flow syntax highlighting to the syntax highlighter GitHub uses.

@sindresorhus
Copy link
Member

@thejameskyle Do you know anyone familiar with Flow that could review?

@jamiebuilds
Copy link
Contributor Author

@jeffmo @marudor @ryyppy could one of you review this?

@ryyppy
Copy link

ryyppy commented Aug 17, 2016

Will have a look! It's very similar to tape's API anyways

@ryyppy
Copy link

ryyppy commented Aug 17, 2016

@thejameskyle @sindresorhus Looking good!

Seriously, I really tried to break the flow definitions... instead it safely lead me through the ava API... as a second thought, it really is very different to tape, especially without any nested tests.

Other than that: Is there a reason why you don't put this in flow-typed instead?
I thought it would be problematic to track declaration files in the maintainers repositories, because of breaking flow versions etc.?

@ryyppy
Copy link

ryyppy commented Aug 17, 2016

NOTE: I found one case which wasn't really supported by flow:

test.after.always('foo', t => {
  t.is //<-- from here flow cannot tell me what's `t`
});

@jamiebuilds
Copy link
Contributor Author

Thanks for pointing that out @ryyppy, this actually uncovers a really interesting problem with intersection types that needs to be fixed.

For now I've worked around the issue with a bit more repetitive of a type definition. Once we fix the issue I can revert the previous change.

@sindresorhus
Copy link
Member

@thejameskyle Is this good to merge?

@jamiebuilds
Copy link
Contributor Author

Yeah, this is good for now. But should update with no repetition once facebook/flow#2358 is fixed.

@sindresorhus
Copy link
Member

Yay \o/ Thank you @thejameskyle :)

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.

3 participants