swift-aws-lambda-runtime
swift-aws-lambda-runtime copied to clipboard
swift-server
Name clashes with NIOHTTP1
Currently AWSLambdaEvents defines public symbols HTTPHeaders, HTTPMethod and HTTPResponseStatus. These clash with the symbols of the same name in NIOHTTP1. While you can differentiate between the two sets by prepending NIOHTTP1., I'm not sure modules should be defining symbols with the same name as NIO equivalents.
What's wrong with using the NIOPHTTP1 versions of these objects, they are already imported by AWSLambdaRuntimeCore?
nio's versions are a bit different from the ones in AWSLambdaEvents, the latter are designed for JSON payload decoding/encoding
we could consider adding a prefix to more easily disambiguate.
ALR* [A]WS [L]amba [R]untime? Maybe you already have some sort of common prefix? In SwiftNIO of course we use NIO for that purpose.
Of course that makes the type names a bit ugly. How are the users most commonly interfacing with those types?
well i'd put them in Lambda.* then
but that's not really the point since they are not connected to the lambda runtime but to the aws event. Maybe we should namespace them in AWSHTTPTypes. But this could also create a naming conflict with the AWS SDK.
@fabianfett There isnt any AWSHTTPTypes and I'm not intending to add any. Even if I did, we'll be able to discriminate between them using the framework name. I'm trying to use the NIO types as much as possible
the latter are designed for JSON payload decoding/encoding
what? :-)
Apart from all the duping, wouldn’t you want to use the NIO types in the public API? (not that this is necessary, but if you want, you could still use those types internally)
This is not about things like a V2.Request but about the basic http types.
Note that this even has functional implications (and duping), e.g. header lookup is not CI?
@helje5 IIRC the main issues was that we would need to be extending these types to add functionality to them (Codable conformance mainly IIRC). rule of thumb we have been following in library development is to avoid extending public types from other libraries (SwiftNIO in this case) as it may lead to conflicts with other libraries that may do the same.
cc @weissi @fabianfett
You don't need to extend the types to decode them (yes, Codable can only be added to the original type itself, but that is not required here). You can decode the necessary data in init(decoder:) and then construct the actual model. Kinda similar to this:
https://github.com/SwiftBlocksUI/SwiftBlocksUI/blob/develop/Sources/SlackBlocksModel/InteractiveRequest/BlockActions.swift#L92
As mentioned in the Slack, a reasonable argument for the duping is that NIO isn't actually necessary for Lambdas. Don't know.
You don't need to extend the types to decode them (yes, Codable can only be added to the original type itself, but that is not required here). You can decode the necessary data in init(decoder:) and then construct the actual model. Kinda similar to this:
I guess if we decide to adopt the NIO types after all we could also use property wrappers
As mentioned in the Slack, a reasonable argument for the duping is that NIO isn't actually necessary for Lambdas. Don't know.
that too. personally, I don't love leaking types where not necessary. there is the argument of copying so we need to trade that off
in the case of HTTPHeaders, there is also a set of mutability functionality that comes with that type that may not makes sense to expose in the Lambda request use case which is more immutable in nature
there is the argument of copying so we need to trade that off They are not even plain copies, e.g. the AWS method is a struct (which is actually the better choice), while the NIO one is an enum w/ an associated type. It's crap and weird if you need to convert them back.
Wrt HTTPHeaders, the NIO ones are actually better than the plain Dict you have in AWS because the latter doesn't account for the header case insensitivity. Yes, AWS breaks out headers in weird ways (single/multi value), but there is no real reason to not combine them back into NIO headers after decoding.
I don't get the mutability thing. NIOHeaders are a regular CoW struct just like Dictionary, right?
In the end it really depends on whether you consider NIO a must have dependency or not. If it is, it's weird not to use its HTTP header types. If not (and they are good arguments for dropping it IMO for this specific thing), I think it's OK to keep it the way it is.