-
Notifications
You must be signed in to change notification settings - Fork 203
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
Conversation
src/utils/request.ts
Outdated
@@ -43,3 +45,16 @@ export function assertMethod (event: CompatibilityEvent, expected: HTTPMethod | | |||
}) | |||
} | |||
} | |||
|
|||
export function getHeaders (event: CompatibilityEvent): IncomingHttpHeaders { | |||
return event instanceof IncomingMessage |
There was a problem hiding this comment.
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.
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:
this way we can also add |
Codecov Report
@@ 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Thank you for quick review!
I agree with you, I will add these methods in One question is, should we rename If you prefer the more consistent name, I will deprecated the old name as you did for |
We might even have shortcuts for common usage ( |
I reorganized header utility methods like below:
(Note: Node.js native |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks perfect. Thanks!
resolves #147
I added the following 3 utility functions as @pi0 suggested:
getHeaders(event)
for requestgetHeader(event, name)
for requestsetHeader(event, name, value)
In addition, I added a unit test for
appendHeader()
function.Do we need to make
getHeaders
andgetHeader
available for response?For your reference, Express.js provides such functions as methods on
req
andres
objects (i.e.req.get
andres.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
orevent.res
:getHeaders(event.req)
/getHeader(event.req, name)
getHeaders(event.res)
/getHeader(event.res, name)
But
event
may be anIncomingMessage
so developers must know which to pass to the function.getHeaders(event)
(event is anIncomingMessage
)Another way is to provide functions dedicated to
req
andres
:getReqHeaders(event)
/getReqHeader(event, name)
getResHeaders(event)
/getResHeader(event, name)