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

API Refactoring #273

Merged
merged 6 commits into from
Nov 9, 2022
Merged

API Refactoring #273

merged 6 commits into from
Nov 9, 2022

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Oct 7, 2022

refactoring the API for 1.0

@tomerd tomerd marked this pull request as draft October 7, 2022 16:45
@tomerd tomerd 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 Oct 7, 2022
@tomerd tomerd added this to the 1.0.0-alpha.1 milestone Oct 7, 2022
@tomerd tomerd changed the title RFC: API Refactoring API Refactoring Oct 7, 2022
@tomerd tomerd force-pushed the api-refactor branch 5 times, most recently from 0fe7b23 to 4f01da2 Compare October 8, 2022 00:04
motivation: define stable API in preperation 1.0 release

changes:
* require swift 5.7, remove redundant backwards compatibility code
* make LambdaHandler, EventLoopLambdaHandler, and ByteBufferLambdaHandler disjointed protocols to reduce API surface area
* create coding wrappers for LambdaHandler and EventLoopLambdaHandler to provide coding bridge
* reuse output ByteBuffer to reduce allocationsi
* add no-op initializer for simple landa use cases
* update callsites and tests
@tomerd
Copy link
Contributor Author

tomerd commented Oct 8, 2022

@fabianfett ptal

@tomerd tomerd marked this pull request as ready for review October 8, 2022 00:18
Examples/ErrorHandling/Lambda.swift Outdated Show resolved Hide resolved
Examples/JSON/Lambda.swift Outdated Show resolved Hide resolved
Examples/LocalDebugging/MyLambda/Lambda.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntime/Lambda+Codable.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntime/Lambda+Codable.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntime/Lambda+Codable.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntimeCore/LambdaHandler.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntimeCore/LambdaHandler.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntimeCore/LambdaHandler.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntimeCore/LambdaHandler.swift Outdated Show resolved Hide resolved
Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Great progress! Love the Handlers that don't depend on each other anymore. However we should preserve the types were feasible. And also I don't think we can get rid of the required initializer that easily.

Sources/AWSLambdaRuntimeCore/Terminator.swift Show resolved Hide resolved
Sources/AWSLambdaRuntimeCore/LambdaHandler.swift Outdated Show resolved Hide resolved
Sources/AWSLambdaRuntimeCore/LambdaHandler.swift Outdated Show resolved Hide resolved
@@ -33,7 +33,7 @@ internal final class LambdaRunner {
/// Run the user provided initializer. This *must* only be called once.
///
/// - Returns: An `EventLoopFuture<LambdaHandler>` fulfilled with the outcome of the initialization.
func initialize<Handler: ByteBufferLambdaHandler>(logger: Logger, terminator: LambdaTerminator, handlerType: Handler.Type) -> EventLoopFuture<Handler> {
func initialize(handlerType: (some ByteBufferLambdaHandler).Type, logger: Logger, terminator: LambdaTerminator) -> EventLoopFuture<any ByteBufferLambdaHandler> {
Copy link
Member

Choose a reason for hiding this comment

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

We must enforce 5.7 if we want to use this syntax. Currently we require 5.6. Also IMO we def. should not return any here. We should preserve the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this PR already changes the requirement to 5.7, unless I missed something?

Copy link
Member

Choose a reason for hiding this comment

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

It only changes this for all the examples. It also drops the 5.4 and 5.5 Package.swift. But the Package.swift without version only requires 5.6.

Copy link
Member

Choose a reason for hiding this comment

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

Also I think you are required to use the old generic syntax here, if we don't want to drop the types. Since we need create a connection between the input and the output type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the Package.swift without version only requires 5.6.

good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I think you are required to use the old generic syntax here, if we don't want to drop the types. Since we need create a connection between the input and the output type.

I don't think this can work with the old syntax. the Handler type is stored in LambdaRuntime which needs to loose it old generic signature, so they do not align any more

return Handler.makeHandler(context: context)

return handlerType.makeHandler(context: context)
.map { $0 as any ByteBufferLambdaHandler }
Copy link
Member

Choose a reason for hiding this comment

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

don't drop types here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was required to make the type system happy. do you have a better way to do this?

Copy link
Member

Choose a reason for hiding this comment

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

we don't need to drop here, if we keep the old syntax above.

@@ -58,7 +60,7 @@ internal final class LambdaRunner {
}
}

func run<Handler: ByteBufferLambdaHandler>(logger: Logger, handler: Handler) -> EventLoopFuture<Void> {
func run(logger: Logger, handler: any ByteBufferLambdaHandler) -> EventLoopFuture<Void> {
Copy link
Member

Choose a reason for hiding this comment

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

no type dropping here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

@@ -19,34 +19,58 @@ import NIOCore
/// `LambdaRuntime` manages the Lambda process lifecycle.
///
/// Use this API, if you build a higher level web framework which shall be able to run inside the Lambda environment.
public final class LambdaRuntime<Handler: ByteBufferLambdaHandler> {
public final class LambdaRuntime {
Copy link
Member

Choose a reason for hiding this comment

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

We did we drop the type here?

Copy link
Contributor Author

@tomerd tomerd Oct 10, 2022

Choose a reason for hiding this comment

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

this was required to remove the conformance to ByteBufferLambdaHandler from the other protocols, as otherwise initializing LambdaRuntime would require it. Do you have a better solution in mind?

Co-authored-by: Yim Lee <yim_lee@apple.com>
* Preserve Types.
* Go inline!
@tomerd
Copy link
Contributor Author

tomerd commented Oct 19, 2022

@fabianfett now with your changes. thank you

@tomerd tomerd merged commit c915322 into swift-server:main Nov 9, 2022
@benrosen78
Copy link

Hi there. Is there any update on when v1.0 will be released with the Swift async/await support? I am excited to use this in my software!

@tomerd
Copy link
Contributor Author

tomerd commented Jan 11, 2023

hi @benrosen78 we are planning to tag the first 1.0 alpha in the next few weeks. you can use it already by pulling the "main" branch in your dependency

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.

6 participants