[DNM][RFC] API-2 #21
[DNM][RFC] API-2 #21
Conversation
There was a problem hiding this comment.
This PR got quite large and changed so many things, that it is quite hard to follow. Especially since it changes some low level API design and propagates the changes to the higher level APIs. I would like to use this as a playground where we can pick ideas from and open up a new PR with smaller changes based on the things we can agree on. This way we would tackle problem by problem.
| /// LambdaCodableCodec is an abstract/empty implementation for codec which does `Encodable` -> `[UInt8]` encoding and `[UInt8]` -> `Decodable' decoding. | ||
| /// Default implementation for `CodableLambdaHandler` codec which uses JSON via `LambdaCodableJsonCodec`. | ||
| /// Advanced users that want to inject their own codec can do it by overriding this. | ||
| public extension CodablePromiseLambdaHandler { |
| var codec: LambdaCodableCodec<In, Out> { get } | ||
|
|
||
| func handle(context: Lambda.Context, payload: In, promise: EventLoopPromise<Out?>) |
There was a problem hiding this comment.
I'm not sure if we want to go with an Out? here. If developers just handle a SNS.Message, they have to come up with some type that they will never return. That's why I would go with two protocols. One with an Out and one with none.
| public protocol LambdaCodableHandler: LambdaHandler { | ||
| var codec: LambdaCodableCodec<In, Out> { get } | ||
|
|
||
| func handle(context: Lambda.Context, payload: In, callback: @escaping CodableLambda.CompletionHandler<Out>) |
There was a problem hiding this comment.
Do we want to offer this as a protocol? I would have just implemented this as a convenience method on top of CodablePromiseLambdaHandler on Lambda. I'm unsure because the use case is hard for me to get:
On a full-control but more difficult to easy to use axis we have:
full-control <--------------------------------------------------------------> easy-to-use
CodablePromiseLambdaHandler CodableLambda.Closure<Out> direct on Lambda
And I'm not sure where the CodableLambdaHandler lands here? Is it the middle? Are Developers who are willing to use this not able to go all in on CodablePromiseLambdaHandler? My idea would be to maybe go with one full-control option and one batteries included option first. Since it's hard to deprecate APIs later, maybe fill in the middle later as needed/requested by the users.
|
I think we can agree on a couple of things and we should get them into master one by one:
/// A processing protocol for a Lambda that takes a `ByteBuffer` and returns an optional `ByteBuffer` asynchronously via an `EventLoopPromise`.
public protocol LambdaHandler {
func handle(context: Lambda.Context, payload: ByteBuffer, promise: EventLoopPromise<ByteBuffer?>)
}Maybe we should also try to get #13 and #19 landed before we begin with the once listed here, to keep merge conflicts as small as possible. |
| private let allocator = ByteBufferAllocator() | ||
| private let httpClient: HTTPClient | ||
| internal extension Lambda { | ||
| struct RuntimeClient { |
There was a problem hiding this comment.
What do you think about naming this RuntimeAPIClient? Only client is a little to generic for my taste...
There was a problem hiding this comment.
its internal type so not an issue imo
| func encode(_: Out) -> Result<[UInt8], Error> { fatalError("not implmented") } | ||
| func decode(_: [UInt8]) -> Result<In, Error> { fatalError("not implmented") } | ||
| func encode(_: Out) -> Result<ByteBuffer, Error> { fatalError("not implmented") } | ||
| func decode(_: ByteBuffer) -> Result<In, Error> { fatalError("not implmented") } |
There was a problem hiding this comment.
I'm not sure, if we should stay with the Result here. I understand the personal preference 😉, but I don't know if this is in the interest of the platform/language.
There was a problem hiding this comment.
ill try the other way
| } | ||
| } | ||
|
|
||
| /// LambdaCodableJsonCodec is an implementation of `LambdaCodableCodec` which does `Encodable` -> `ByteBuffer` encoding and `ByteBuffer` -> `Decodable' decoding | ||
| /// using JSONEncoder and JSONDecoder respectively. | ||
| // This is a class as encoder amd decoder are a class, which means its cheaper to hold a reference to both in a class then a struct. | ||
| private final class LambdaCodableJsonCodec<In: Decodable, Out: Encodable>: LambdaCodableCodec<In, Out> { |
There was a problem hiding this comment.
Do we maybe want to move the default Codable implementation into another package? This way we could not use Foundation at all in the main package. We could use the same naming as NIOFoundationCompat so for example AWSLambdaRuntimeFoundationCompat. We could even use @_exported within this package so that "normal" users only have to import this one package. This would allow developers, like myself, who want to be as barebones as possible, to not link Foundation at all.
There was a problem hiding this comment.
Btw. in the AWSLambdaRuntimeFoundationCompat package we could also offer callbacks based on Foundation.Data, so we don't have to use [UInt8] in there, something that looks quite strange to the average iOS developer.
yes this is the idea |
|
replaced by #33 |
No description provided.