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

Axios new options definition #3681

Merged
merged 7 commits into from
Jan 1, 2020

Conversation

quanlinc
Copy link
Contributor

Other notes:
Also did a little bit refactor to remove "any" type in definition and get tests passed

@LoganBarnett
Copy link
Contributor

Full disclosure: I helped @quanlinc with this out of band. For what it's worth, it looks good to me :)

@@ -4,16 +4,16 @@ declare module 'axios' {

declare type AxiosTransformer<T> = (
data: T,
headers?: { [key: string]: any }
) => any;
headers?: {| [key: string]: mixed |},
Copy link
Member

Choose a reason for hiding this comment

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

I don't think an exact object works here.

Later in the diff you used { [key: string]: mixed, ...} for such case, any special thing here?

Copy link
Contributor Author

@quanlinc quanlinc Dec 30, 2019

Choose a reason for hiding this comment

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

Good call, I think the original thought was flow will match up the type instead of literal key string, will this fixed and test covered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I misunderstood your comment, @quanlinc, but I'd like to clarify: We don't want to enumerate the possible keys here (they can be anything). I think we just need to make this inexact instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be just me not expressing myself clearly, I agree with what @pascalduez said, I had a wrong interpretation of using exact type, inexact is the way to go.

@pascalduez pascalduez merged commit b09080b into flow-typed:master Jan 1, 2020
@PresentProgrammer
Copy link

@quanlinc , with "flow-bin": "0.81.0",, the following error happens:

Error ---------------------------------------------------------------------------- flow-typed/npm/axios_v0.19.x.js:10:42

Unexpected token }

   10|     headers?: { [key: string]: mixed, ...},
                                                ^

@quanlinc
Copy link
Contributor Author

quanlinc commented Jan 2, 2020

@PresentProgrammer Sorry for breaking your build, didn't expect someone is still using 0.81, when inexact type is not introduced. I could fix this issue by splitting Axios libdef into two pieces like most other libraries do. But I also think it's consumers extra support burdens when using an older version of flow, or any tool. Which also reveals there's a gap in test between different flow-typed versions, which didn't catch your problem. I've done the work with the best of my knowledge by then and passed all tests. I'll leave the discussion to the community, at what extent do we need to support old version given this PR is already merged(Exactly how you run into this issue I presume)

@pascalduez
Copy link
Member

Sorry that slipped through my review.
We usually introduce inexact object notation from version 0.104.x.
I someone is willing to submit a PR to fix this, would be rad.

@PresentProgrammer
Copy link

#3689 — related issue

@pascalduez
Copy link
Member

Should be fixed now.

@PresentProgrammer
Copy link

Yes, issue with Flow 0.81 solved. Thank you!

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.

None yet

4 participants