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

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

NamesMT
Copy link
Contributor

@NamesMT NamesMT commented May 20, 2024

Resolves #2723 - Edit: unsure, see #2726 (comment)

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
  • Run tests
  • bun denoify to generate files for Deno
  • bun run format:fix && bun run lint:fix to format the code

@NamesMT
Copy link
Contributor Author

NamesMT commented May 20, 2024

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 NamesMT changed the title fix: generalize LambdaEvent fix(adapter/aws-lambda): generalize LambdaEvent May 20, 2024
@NamesMT NamesMT changed the title fix(adapter/aws-lambda): generalize LambdaEvent fix(adapter/aws-lambda): generalize LambdaEvent, EventProcessor May 20, 2024
@exoego
Copy link
Contributor

exoego commented May 20, 2024

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.

@NamesMT NamesMT changed the title fix(adapter/aws-lambda): generalize LambdaEvent, EventProcessor RFC(adapter/aws-lambda): generalize LambdaEvent, EventProcessor May 20, 2024
@watany-dev
Copy link
Contributor

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
Copy link
Contributor

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.

@NamesMT
Copy link
Contributor Author

NamesMT commented May 20, 2024

@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.

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.

hono/aws-lambda + @hono/graphql-server GET not working!
3 participants