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

feat: add utilities for http headers #157

Merged
merged 2 commits into from Aug 3, 2022
Merged

feat: add utilities for http headers #157

merged 2 commits into from Aug 3, 2022

Conversation

NozomuIkuta
Copy link
Member

@NozomuIkuta NozomuIkuta commented Aug 2, 2022

resolves #147

I added the following 3 utility functions as @pi0 suggested:

  • getHeaders(event) for request
  • getHeader(event, name) for request
  • setHeader(event, name, value)

In addition, I added a unit test for appendHeader() function.


Do we need to make getHeaders and getHeader available for response?

For your reference, Express.js provides such functions as methods on req and res objects (i.e. req.get and res.get), so it's obvious from what object the functions get headers.

There are 2 ways to provide same functionalities with normal functions.

One is to let users pass event.req or event.res:

  • getHeaders(event.req) / getHeader(event.req, name)
  • getHeaders(event.res) / getHeader(event.res, name)

But event may be an IncomingMessage so developers must know which to pass to the function.

  • getHeaders(event) (event is an IncomingMessage)

Another way is to provide functions dedicated to req and res:

  • getReqHeaders(event) / getReqHeader(event, name)
  • getResHeaders(event) / getResHeader(event, name)

@NozomuIkuta
Copy link
Member Author

I'm not sure how it is going about #73 (related: #123).
So, if you tell me the details, I will fix this PR as needed.

@@ -43,3 +45,16 @@ export function assertMethod (event: CompatibilityEvent, expected: HTTPMethod |
})
}
}

export function getHeaders (event: CompatibilityEvent): IncomingHttpHeaders {
return event instanceof IncomingMessage
Copy link
Member

Choose a reason for hiding this comment

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

CompatibilityEvent always has .req and .res props. We do not need to handle Node.js req directly being passed.

@pi0
Copy link
Member

pi0 commented Aug 3, 2022

Thanks for PR @NozomuIkuta <3 I think you are right. We might better have explicit names for utils operating on request or response. What do you think of these names:

  • getRequestHeaders
  • getRequestHeader
  • setResponseHeader

this way we can also add getReponseHeader/getResponseHeaders

@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #157 (aadd4b0) into main (56bfe0a) will increase coverage by 0.20%.
The diff coverage is 60.00%.

❗ Current head aadd4b0 differs from pull request most recent head 3ed6684. Consider uploading reports for the commit 3ed6684 to get more accurate results

@@            Coverage Diff             @@
##             main     #157      +/-   ##
==========================================
+ Coverage   65.97%   66.17%   +0.20%     
==========================================
  Files          14       14              
  Lines         867      887      +20     
  Branches      176      184       +8     
==========================================
+ Hits          572      587      +15     
+ Misses        124      121       -3     
- Partials      171      179       +8     
Impacted Files Coverage Δ
src/utils/request.ts 60.00% <60.00%> (ø)
src/utils/response.ts 62.68% <60.00%> (+4.62%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@NozomuIkuta
Copy link
Member Author

NozomuIkuta commented Aug 3, 2022

@pi0

Thank you for quick review!


What do you think of these names:

  • getRequestHeaders
  • getRequestHeader
  • setResponseHeader

this way we can also add getReponseHeader/getResponseHeaders

I agree with you, I will add these methods in src/utils/request.ts and src/utils/response.ts, respectively.

One question is, should we rename appendHeader to appendResponseHeader for more clarity and consistency?
It's only response object to which we can append headers, so it might be a little bit too obvious and verbose.

If you prefer the more consistent name, I will deprecated the old name as you did for useQuery and so on.

@pi0
Copy link
Member

pi0 commented Aug 3, 2022

appendResponseHeader also makes sense. I think we could keep other utils simple as is because header is on the few cases is shared between request and response.

We might even have shortcuts for common usage (getHeader => shortcut for getRequestHeader), (setHeader => setResponseHeader) too.

@NozomuIkuta
Copy link
Member Author

@pi0

I reorganized header utility methods like below:

  • getRequestHeaders(event, headers) (alias: getHeaders)
  • getRequestHeader(event, name) (alias: getHeader)
  • setResponseHeaders(event, headers) (alias: setHeaders)
  • setResponseHeader(event, name, value) (alias: setHeader)
  • appendResponseHeaders(event, headers) (alias: appendHeaders)
  • appendResponseHeader(event, name, value) (alias: appendHeader)

(Note: headers is an object of header names and their values)

Node.js native IncomingMessage has both getHeaders and getHeader, so I applied this relationship to the other methods as well.

@NozomuIkuta NozomuIkuta requested a review from pi0 August 3, 2022 16:00
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Looks perfect. Thanks!

@pi0 pi0 merged commit 272f883 into unjs:main Aug 3, 2022
@NozomuIkuta NozomuIkuta deleted the feat/http-header-utils branch August 3, 2022 16:02
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.

Add helper function for headers
2 participants