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] Drop event label from handle methods in LambdaHandlers #225

Merged
merged 2 commits into from
Sep 22, 2021

Conversation

fabianfett
Copy link
Member

@fabianfett fabianfett commented Aug 25, 2021

Drop event label from handle methods in LambdaHandlers protocols

Motivation:

In Lambda functions user's handle events from different sources. Those events can be S3Events, but also APIGatewayRequests. Let's look at a Lambda, that integrates with APIGateway.

@main
struct EchoLambda: AsyncLambdaHandler {
    typealias Event = APIGatewayV2Request
    typealias Output = APIGatewayV2Response

    init(context: Lambda.InitializationContext) async throws {}

    func handle(event: APIGatewayV2Request, context: Lambda.Context) async throws -> APIGatewayV2Response {
        APIGatewayV2Response(statusCode: .ok, body: event.body)
    }
}

The argument in the handle method is labeled event even though, in this case request would be clearly a better name. The user could use request as a parameter name by explicitly specifying the parameter name:

func handle(event request: APIGatewayV2Request, context: Lambda.Context) async throws -> APIGatewayV2Response

However event request is not easy to read and the parameter name overwrite is not the most known swift feature. For this reason I would advice against this option.

For this reason I propose dropping the argument label and specifying a parameter name event to guide users. New function signature:

func handle(_ event: In, context: Lambda.Context) async throws -> Out

This would allow users to write lambdas, that integrate with APIGateway as such:

func handle(_ request: APIGatewayV2Request, context: Lambda.Context) async throws -> APIGatewayV2Response

Modifications:

  • Drop the event label in the handle LambdaHandler protocol methods
  • Rename typealias in EventLoopHandler protocol to Event and Output (from In and Out)

Result:

A cleaner, more flexible API.

@fabianfett fabianfett added this to the 1.0.0-alpha.1 milestone Aug 25, 2021
@fabianfett fabianfett added the ⚠️ needs-major-version-bump For PRs that when merged cause a bump of the major version, ie. x.0.0 -> (x+1).0.0 label Aug 25, 2021
@fabianfett fabianfett requested a review from tomerd August 25, 2021 09:52
@adam-fowler
Copy link
Member

This change makes sense. It'll cause some annoyance as it changes the signature of a protocol requirement but I can live with that.

@kneekey23
Copy link

From a different perspective in all the lambda docs we do refer to the object being passed to lambda as the event. If you lack familiarity with aws and swift runtime is your first encounter removing the label could be confusing for some people. Just another thought here...

@kneekey23
Copy link

The word event is used for example in this doc several times even in reference to an api gateway request: https://docs.aws.amazon.com/lambda/latest/dg/lambda-services.html

@fabianfett
Copy link
Member Author

cc @bmoffatt

@tomerd
Copy link
Contributor

tomerd commented Aug 27, 2021

it is the "swift way" to drop label on first arguments when their meaning / usage can be easily inferred from the method name or the general context. I dont have a strong opinion here (there are two sides to this coin, as @kneekey23 points out), and in any case we should use the term "event" in the API docs to connect it back to the AWS terminology, even if we dont have a label

@fabianfett fabianfett marked this pull request as ready for review August 28, 2021 12:35
@fabianfett
Copy link
Member Author

@swift-server-bot test this please

@fabianfett fabianfett force-pushed the ff-drop-event-label branch 2 times, most recently from 5104f27 to 3e1d7bb Compare September 21, 2021 19:48
@fabianfett
Copy link
Member Author

@tomerd Updated the name of the event In type to Event. Out is now called Output. This should make adoption even easier. Please re-review.

Copy link
Contributor

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

looks great!

@tomerd
Copy link
Contributor

tomerd commented Sep 21, 2021

some CI failures, seems like some test code needs to be further adjusted

@fabianfett fabianfett merged commit e5aa488 into swift-server:main Sep 22, 2021
@fabianfett fabianfett deleted the ff-drop-event-label branch September 22, 2021 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ needs-major-version-bump For PRs that when merged cause a bump of the major version, ie. x.0.0 -> (x+1).0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants