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

Add response type to axios libdef #2006

Merged
merged 3 commits into from
Mar 29, 2018
Merged

Add response type to axios libdef #2006

merged 3 commits into from
Mar 29, 2018

Conversation

adambrgmn
Copy link
Contributor

This PR adds a separate response type to axios calls, earlier the definitions
used the same infered type for config.data and response.data. This has been mentioned in #1975 and #1539.

But hopefully this wont break backwards compatability since I added the
original type (T) as default for the new response type (R). Also verified by tests.

As of now I have only updated the 0.18.x-version. But the same issue is present in earlier versions as well. If you think I should update the old ones as well I'd be happy to do so.

THis commit adds a separate response type to axios calls, earlier the definitions
used the same infered type for config.data and response.data.

But hopefully this wont break backwards compatability since I added the
original type (T) as default for the new response type (R).
@AndrewSouthpaw AndrewSouthpaw self-requested a review March 29, 2018 13:33
Copy link
Contributor

@AndrewSouthpaw AndrewSouthpaw left a comment

Choose a reason for hiding this comment

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

I think it's fine to leave the older definitions alone for now.

const promise1: AxiosPromise<{ foo: string }, { bar: string }> = axios({ url: '/', method: 'post', data: { foo: 'bar' }})
promise1.then(({ data }) => {
// $ExpectError
data.foo;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a check for data.bar is a string, ensure that carries through. :)

@gantoine gantoine added changes requested Changes have been requested by a reviewer libdef Related to a library definition labels Mar 29, 2018
@adambrgmn
Copy link
Contributor Author

Thanks for the response (pun intended...)! I added a test to verify that "data.bar" is a string and all tests passes on my machine.

@AndrewSouthpaw AndrewSouthpaw merged commit 9dcc978 into flow-typed:master Mar 29, 2018
@AndrewSouthpaw
Copy link
Contributor

Thanks @adambrgmn!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested Changes have been requested by a reviewer libdef Related to a library definition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants