-
Notifications
You must be signed in to change notification settings - Fork 176
feat(event-handler): Add support for HTTP APIs (API Gateway v2) #4714
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| describe('Class: Router - Basic Routing', () => { | ||
| describe.each([ | ||
| { version: 'V1', createEvent: createTestEvent }, |
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.
As mentioned, these tests can be extended just by adding a new test event creation function to the describe.each block.
| * @param stream - The Node.js Readable stream to convert | ||
| * @returns A Promise that resolves to a base64 encoded string | ||
| */ | ||
| async function nodeStreamToBase64(stream: Readable) { |
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.
We will need to add this back when we implement #4514.
aa81927 to
b703896
Compare
| return { | ||
| event, | ||
| context, | ||
| req: new Request('https://invalid'), |
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.
I'm not fond of creating this dummy Request object but the alternative is to make req optional, which will make the ergonomics of writing middleware much worse if users need to do a null check any time they want to interact with a request that will always be there apart from this particular corner case.
| it('re-throws non-InvalidHttpMethodError from proxyEventToWebRequest', async () => { | ||
| // Prepare | ||
| vi.doMock('../../../../src/rest/converters.js', async () => { | ||
| const actual = await vi.importActual< |
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.
I very much dislike tests like this. The reason we have to do this is because the type guards before this code path are so strict that the error code is virtually unreachable. I have to mock the proxyEventToWebRequest function to throw an exception that it will almost certainly never throw in real use.
9f3a276 to
bc5bdfc
Compare
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.
Tested it out and looks good
|
Nice! |
5f2e68e to
21ca074
Compare
|
|
@swopnildangol I made a change to how we calculate what response to return. Rather than passing the event around and using type guards, I save the response type once in the request context with a type called |



Summary
This PR adds support for HTTP API (API Gateway v2) to the event handler utility. It also adds the framework for adding other event sources like function URLs, ALB and VPC lattice.
Edit: after some testing, I've realised that as function URLs and HTTP APIs share the same event type, which means function URLs also work.
Changes
resolvemethod now has overloads to support bothAPIGatewayProxyEventandAPIGatewayProxyEventV2events. New overloads can be added for new event types. I attempted to do this with conditional types but was unable to make it it work, furthermore, the type signatures were much harder to read than simpler overrides.#resolvemethod in the Router class now returns the entire request context object. We usereqCtx.resnow as the canonical representation of the response to be returned. As the request is now always represented as a webResponseobject, converting the response required for different event sources is simpler. We also save the type of response required in the request context using theResponseTypetype.handlerResultToProxyResultfunction has been removed now that all response are converted to WebResponseobjects.#resolvemethod will allow us to handle this is a more principled way by using the request context to indicate whether a response should be encoded or not. I plan to do this work when implementing Feature request: first-class support for binary responses #4514.APIGatewayProxyEventV2objects has been added.APIGatewayProxyEventV2events to WebRequestobjects has been added.Respsonseobjects toAPIGatewayProxyReultV2objects has been added.Testing
middlewaretests. These use the exact same mechanisms as the other tests so I have decided to only add some additionalv2specific tests. I am open to persusion as to whether these should follow the other tests.curlthat requests will be fulfilled by the same code with no changes required.Issue number: closes #4713
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.