-
Notifications
You must be signed in to change notification settings - Fork 113
Improves Developer Ergonomics for LambdaHandler Conformances #272
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
Conversation
…ng. Try putting the override as an instance function of the initialization context struct
…etter developer ergonomics for users that are trying to implement a simple Lambda but don't care to do any setup during initialization.
…tocol. This allows a developer to drop the requirement for defining their own typealiases in their LambdaHandler conformances
… on 5.6 and such)
Can one of the admins verify this patch? |
14 similar comments
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Backtrace.install() | ||
var logger = Logger(label: "Lambda") | ||
logger.logLevel = configuration.general.logLevel | ||
) throws -> Int { |
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.
The changes here is just code cleanup that should make the binary a little smaller. The logic here is exactly the same and removes the implicitly unwrapped optional we had for var result: Result<Int, Error>!
I can revert this if you don't like it.
} | ||
var general: General = .init() | ||
var lifecycle: Lifecycle = .init() | ||
var runtimeEngine: RuntimeEngine = .init() |
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.
This is probably unnecessary if you don't like it but I updated this to be easier to read. You don't necessarily need the init functions you had before if these are just structs. Updating to vars for these internal structs still keeps the same ability to initialize these structs with the default values you had defined in your previous init functions.
Let me know if you don't want this!
@@ -14,13 +14,12 @@ | |||
|
|||
#if compiler(>=5.6) | |||
@preconcurrency import Dispatch | |||
@preconcurrency import Logging | |||
@preconcurrency import NIOCore |
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.
Xcode 14 complains about these. I don't think they're needed anymore? If this breaks something before Xcode 14 I may have to remove this
#if compiler(>=5.6) | ||
@preconcurrency import NIOCore | ||
@preconcurrency import NIOHTTP1 | ||
#else |
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.
Same as before, Xcode 14 says these aren't needed. Will revert this change if it breaks something before Swift 5.7
|
||
init() { | ||
self.storage = Storage() | ||
} |
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.
More code cleanup. You don't need the init if all you're doing is providing a default value to the private storage
variable
XCTAssertEqual(String(describing: shouldFailWithError!), String(describing: error), "expected error to mactch", file: file, line: line) | ||
default: | ||
XCTFail("invalid state") | ||
func assertLambdaRuntimeResult(_ result: @autoclosure () throws -> Int, shouldHaveRun: Int = 0, shouldFailWithError: Error? = nil, file: StaticString = #file, line: UInt = #line) { |
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.
Using an autoclosure, we can guarantee that the result
closure can get called with try
without having to update the test function definitions to using throws
.
This also takes advantage of swift error handling which allows us to clean up this logic. It's more readable in my opinion but I can revert this if you don't like it.
@@ -16,7 +16,7 @@ import NIOCore | |||
extension EventLoopLambdaHandler where Event == String { | |||
/// Implementation of a `ByteBuffer` to `String` decoding. | |||
@inlinable | |||
public func decode(buffer: ByteBuffer) throws -> String { | |||
public func decode(buffer: ByteBuffer) throws -> Event { |
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.
These changes help to solve ambiguities in the swift compiler across default implementations of EventLoopLambdaHandler
when importing AWSLambdaRuntime
or running tests for the package in Xcode
thanks @hectormatos2011, @fabianfett and myself have been discussing some API changes to the main branch that should help address some of the points you bring up. should be able to put up a PR soon and then we can maybe see what else is missing and you can help get it over the finish line. wdyt? |
@tomerd I'd love to contribute! Thanks for taking a look. Would you like me to close this one? There's some good nuggets of code in here that should at least help with the type inference issue |
lets keep it open and focus on landing #273. then we can see what else we need to bring over |
hi @hectormatos2011 would you like to revisit this now that we have the 1.0 API in place? |
Let me take a look at 1.0 and close the PR if need be |
Closing this PR since most of the improvements have been implemented in the 1.0 release! @tomerd Is it currently possible to have code in the Lambda that makes it always run with |
that is not possible right now, to protect users from accidentally deploying the mock / debug server |
This PR aims to improve developer ergonomics for simple
LambdaHandler
conformances when trying to quickly put together a local server to test against.Motivation:
Swift 5.7 added primary associated types for protocols where you can start using generic bracket syntax for protocols. An issue for this was opened not too long ago in this repo (#266) with the primary motivation of improving developer ergonomics around types conforming to
LambdaHander
. However, swift's type inference system should be able to handle the aforementioned issue just fine without using primary associated types (although support for that could still be useful when defining properties conforming toLambdaHandler
when you want to constrain the types).Swift's type inference system does not pick up the
Event
andOutput
types for objects conforming toLambdaHandler
because those types are defined in the covariant protocolEventLoopLambdaHandler
. This means that you need to add type aliases forEvent
andOutput
when conforming toLambdaHandler
- resulting in less than ideal developer ergonomics when trying to spin up a quick little local server.Additionally, the initializer for
LambdaHandler
is currently a required function in the protocol. This is super useful for setup logic for your Handlers but degrades the developer ergonomics of the most simple/minimal use-case for LambdaHandlers. Developers should be able to continue having the capability of writing simple lambda handlers like they could do in 0.5.2:The current state of affairs in comparison is to explicitly define the
Event
andOutput
associated types ofLambdaHandler
when writing a simple lambda as well as being forced to include an init that isn't necessarily always required to successfully run a simple lambda:With the changes on main, a developer loses the ease of writing a three-line lambda that they could do in 0.5.2.
On top of all of this, when needing to run a server locally in Xcode, a developer would need to add an environment variable to their local scheme. This isn't a much used feature in Xcode for developers coming from iOS who are looking to maybe start writing swift on the server. This isn't too big of a deal, but could be a bit of pain to do when trying to set yourself up with Lambda for the first time. It also introduces the possibility of having to manage two different environment variables if you're switching between debugging in Xcode or running the server locally in your Terminal. It would be nice to have the capability of defining this in code so a developer wouldn't have to open a separate window to toggle a server running locally.
I may be wrong about this (not a lot of experience writing swift on the server), but if a developer is running a local server in Xcode, it should be safe to assume that the server isn't running on an environment like AWS Lambda or Docker. I think that if a server is running their Lambda in Xcode,
LOCAL_LAMBDA_SERVER_ENABLED
should be enabled by default. Doing so would enable developers to package ready-to-go example local servers with their example mobile apps (maybe they want to demonstrate the usage of an SDK or something).Currently, developers in this situation would have to instruct their users to set up this
LOCAL_LAMBDA_SERVER_ENABLED
environment variable. If a user is a swift developer already, they might have the urge to open that Package in Xcode and just hit run without knowing they need to set that variable up.With the support of a variable for enabling running as a local server in Lambda directly, a developer could package a ready-to-go local server package with their example apps that would only require opening the package and running in Xcode with no extra set up.
Modifications:
LambdaHandler
,EventLoopLambdaHandler
, andByteBufferLambdaHandler
. Thehandle(_:context:)
functions shared across all three prevented the swift compiler from properly inferring theEvent
andOutput
types betweenLambdaHandler
andEventLoopLambdaHandler
. Thehandle(_:context:)
functions are now more descriptive with their primary parameter to match the protocol they are associated with:LambdaHandler.handle(_:context)
has now been changed toLambdaHandler(request:context:)
EventLoopLambdaHandler.handle(_:context)
has now been changed toEventLoopLambdaHandler(event:context:)
ByteBufferLambdaHandler.handle(_:context)
has now been changed toByteBufferLambdaHandler(buffer:context:)
LambdaHandler
protocolinit(context:)
that calls through toinit()
- this allows fully qualified structs conforming toLambdaHandler
to omit their initializers if no setup is required for the Lambda handlerisLocalServerEnabled
to theByteBufferLambdaHandler
protocoltrue
when running in Xcode, but still respects theLOCAL_LAMBDA_SERVER_ENABLED
environment variable.false
if running outside of XcodeResult:
With all of the changes in this PR, a developer could package a local lambda handler Swift Package with an example app that could look like this:
All previous logic is still retained but the optional code needed is now opt-in with sensible defaults.