-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 Flow type ambiguity #1164
Fix Flow type ambiguity #1164
Conversation
Thanks for this, @leebyron! Can't wait for this to be merged 🎆. In the meantime, is there a convinent way to apply this patch, do you know? |
Hm. I tried to apply this locally but ran into some trouble. If anyone has any ideas, please let me know! Thanks! |
I haven't looked at (these, or any, really) Flow definitions much. From the Besides that, if Flow-users could try out this PR that'd be great. I don't really know how to test it myself. |
Fixes avajs#1114 As reported in avajs#1114, flow reports ambiguity when defining tests: ```js test('my test', t => {}) ``` Flow will report that it could apply either the `Test` type or the `Macro` type. The `Test` type is an obvious fit, but `Macro` also applies since the `...args` resolves to the empty array, and the `title?` attribute happens to be not provided. My suggested fix is to merge the `Test` and `Macro` types together since `Macro` is really a superset of the `Test` type, and we don't lose any type safety by just expecting to always get Macros. This super set type is called `TestImplementation` which better matches how AVA names these things. I tested this locally on a flow-typed repository and confirmed that the type ambiguity errors are solved.
689152f
to
1fd801b
Compare
Great suggestion - I cleaned up the names of the types. Anyone who would like to test this out locally, here's how I'm doing it:
|
@leebyron Ah, I guess you could say Jokes aside, I managed to apply your fix locally and things work smoothly in my project, thanks! |
Thanks for helping us out with this one @leebyron. We really appreciate it. No one on the team has much experience with Flow. |
Thanks for the merge. Feel free to shout my way if you ever need help with anything related to type definitions. |
Fixes #1114
As reported in #1114, flow reports ambiguity when defining tests:
Flow will report that it could apply either the
Test
type or theMacro
type. TheTest
type is an obvious fit, butMacro
also applies since the...args
resolves to the empty array, and the optionaltitle?
attribute happens to be not provided.My suggested fix is to merge the
Test
andMacro
types together sinceMacro
is really a superset of theTest
type, and we don't lose any type safety by just expecting to always get Macros.I tested this locally on a flow-typed repository and confirmed that the type ambiguity errors are solved.