hono icon indicating copy to clipboard operation
hono copied to clipboard

RFC(adapter/aws-lambda): generalize `LambdaEvent`, `EventProcessor`

Open NamesMT opened this issue 1 year ago • 5 comments

Resolves #2723 - Edit: unsure, see https://github.com/honojs/hono/pull/2726#issuecomment-2120316641

Lambda's event could come from any source, e.g: manual invokations, AWS triggers like S3, EventBridge, etc. So it should have the type of unknown

This PR generalizes EventProcessor to accept an unknown event, with the old processor logic for "Request"-based event sources moved to RequestEventProcessor

Because the processor's logic was moved to a different file, for easier reviewing what have really changed, the maintainer could check specifically the commit: fix: generalize LambdaEvent


The author should do the following, if applicable

  • [ ] Add tests
  • [x] Run tests
  • [x] bun denoify to generate files for Deno
  • [x] bun run format:fix && bun run lint:fix to format the code

NamesMT avatar May 20 '24 08:05 NamesMT

Hello @yusukebe,

Out of the PR's scope, is there an ETA to remove binding of top-level requestContext?, It seems to have marked as deprecated since v3.10.0 but is still in the code and bloating it. Github Search also doesn't show any repository using .env.requestContext

NamesMT avatar May 20 '24 08:05 NamesMT

As a contributor to adapter/aws-lambda, it would be great if tests were added to demonstrate this change fixes #2723.

It appears that the issue reporter attempted to let Hono handle Lambda's Function URL: image In my opinion, adding a new type for Function URL is a straightforward solution for #2723. We could gain a help from a type for Function URL in @types/aws-lambda.

I am not sure making Hono handle unknown (arbitrary events), including non-HTTP events from S3 or EventBridge, is reasonable from maintenance PoV. I am new to Hono, so such use case might make sense, so it would be great if you elaborate such use cases.

exoego avatar May 20 '24 11:05 exoego

Basically, I think that Hono's standard repository should handle HTTP requests. I personally disagree with making the role of the adaptor too fat. As for non-http adaptor, I think it is good to manage it as non-http adaptor from @hono/aws/lambda for example.

watany-dev avatar May 20 '24 11:05 watany-dev

Hello @yusukebe,

Out of the PR's scope, is there an ETA to remove binding of top-level requestContext?, It seems to have marked as deprecated since v3.10.0 but is still in the code and bloating it.

Github Search also doesn't show any repository using .env.requestContext

Sorry, I failed to delete this in v4.

watany-dev avatar May 20 '24 11:05 watany-dev

@exoego You're right, this PR is more about adding support for "unknown" event processing than to fix #2723, I changed the PR's title to "Request For Comment" for now.

I'm also unsure about adding unknown event handling to the official Hono adapter, that's why I have previously created a personal fork of the adapter, though IMO Hono as a "framework", the official adapter supporting the platform's feature does make sense, And now, because it didn't work with the featured graphql middleware, I created the PR.

However, coming back to the issue with the GraphQL middleware (#2723), I'm not familiar with GraphQL and previously I thought graphql connects using it's own protocol, hence the issue of "requestContext" undefined, but reading more about it, it's using GET/POST HTTP method, Function URL is using "APIGateway V2 Payload" so it should be already supported and requestContext should exists. So we might have a different bug here.

I'll set up a reproduction and find the real cause for #2723 later when I have time.

NamesMT avatar May 20 '24 11:05 NamesMT