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

Added support for headers #20

Merged
merged 6 commits into from Aug 7, 2019

Conversation

telekid
Copy link
Contributor

@telekid telekid commented Jul 30, 2019

This is an attempt to get @asBrettisay's PR that adds support for headers (#10) merged in. There were a few comments that were never addressed that are currently blocking #10 from merging.

Note that #13 made a number of changes to the way headers are handled by Svelte, which made this PR a little more complex than I initially anticipated. See 65b7c90 for the modifications to #10 that were required to make @asBrettisay's PR compatible with master.

Headers passed in during the request will override headers passed into configuration.

This PR transfers responsibility of request header merging from RESTClient to OperationBuilder.

I'm not entirely sold on one API choice made in the original PR, which is that headers are passed along to an operation in the same Hash as all other parameters. This opens up the potential for parameter name collisions, since AFAIK there is nothing in the swagger spec that prevents headers from sharing names with query, path or form params (https://swagger.io/docs/specification/2-0/describing-parameters/). Any suggestions or thoughts regarding this point?

Copy link

@whenceforth whenceforth left a comment

Choose a reason for hiding this comment

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

👍

This was referenced Jul 30, 2019
Copy link
Contributor

@brafales brafales left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@brafales brafales merged commit d205de4 into notonthehighstreet:master Aug 7, 2019
@brafales
Copy link
Contributor

brafales commented Aug 7, 2019

I'll organise a version bump and release soon.

@telekid
Copy link
Contributor Author

telekid commented Aug 7, 2019

Thanks for the quick review and merge! 🎉

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