fix(proxy): fix header overwrite #3922
Conversation
* header key is case-insensitive * delete header if value is null or undefined
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3922 +/- ##
==========================================
- Coverage 91.31% 91.29% -0.03%
==========================================
Files 168 168
Lines 10673 10773 +100
Branches 3167 3075 -92
==========================================
+ Hits 9746 9835 +89
- Misses 926 937 +11
Partials 1 1 ☔ View full report in Codecov by Sentry. |
|
Hi @BarryThePenguin. Thank you very much for the report. I appreciate it. I think you're also thinking about performance in #3921, but I'd like to consider that in a separate PR. I'd like to fix the problem of not being overwritten correctly in this PR, but what do you think? |
|
@yusukebe |
BarryThePenguin
left a comment
BarryThePenguin
left a comment
There was a problem hiding this comment.
I think you're also thinking about performance in #3921, but I'd like to consider that in a separate PR. I'd like to fix the problem of not being overwritten correctly in this PR, but what do you think?
Sounds good 👍🏻
I was having a hard time keeping the PR focused. This PR solves the immediate issue without changing anything else
| Array.isArray(requestInit.headers) || | ||
| requestInit.headers instanceof Headers |
There was a problem hiding this comment.
This makes sense 👍🏻
By excluding type Headers | string[][], only type Record<string, string | undefined> can be used to remove specific headers from a request.
I think it makes the API more complicated, but the behaviour is clearly documented, so it should be fine.
One alternative would be dropping type Headers | string[][], but that might make the API too restrictive 🤔
| // delete header if value is null or undefined | ||
| headers.delete(key) | ||
| } else { | ||
| headers.set(key, value) |
There was a problem hiding this comment.
I think this is the least surprising choice 👍🏻 Using Headers#append was confusing things for me
|
@BarryThePenguin Thank you for your comment! As you pointed out, the proxy helper behaves as follows.
This is precisely what is intended, as it is ideal for the pattern described in the documentation: ' to allow for easy removal or overwriting in a declarative way using https://hono.dev/docs/helpers/proxy If |
|
@usualoma @BarryThePenguin Thank you both! Looks good. Merging. |
3 participants
fixes #3920
The author should do the following, if applicable
bun run format:fix && bun run lint:fixto format the code