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 to override host header. #9092

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nilo85
Copy link

@nilo85 nilo85 commented Feb 25, 2024

As dns is not working on my network I decided to workaround the issue for now and figured this might actually be a feature we want here =)

@nilo85
Copy link
Author

nilo85 commented Feb 26, 2024

I no longer need this hack, however, there might be situations you need to override host header (proxy use etc) so might still make sense to support

@mcspr
Copy link
Collaborator

mcspr commented Feb 27, 2024

Perhaps the client should allow addHeader to override Host:?
Meaning, when headers are prepared it would check whether it was added or not; and if not, use the default _host. Right now it is restricted, don't see why it should be tbh

@nilo85
Copy link
Author

nilo85 commented Feb 27, 2024

@mcspr I suppose, however to be completely http compliant, you can pass multiple values for same header, most other http implementations do setHeader and addHeader, where setHeader sets a single header and replaces whatever would have been there before, and addHeader would send multiple values.

However in this case, judging by the code I suspect the restriction here is to kep the code simple as the host header is hardcoded in the request call.

I would be fine rejecting this PR as I no longer need it, but possible it could make sense to consider adopting more http client features in the future, if needed... I suspect for most applications using these microchips, explicit proxy settings etc is overkill so I personally would consider it a corner case

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.

2 participants