swift-server / swift-aws-lambda-runtime

Swift implementation of AWS Lambda Runtime
https://swiftpackageindex.com/swift-server/swift-aws-lambda-runtime
Apache License 2.0
1.15k stars 105 forks source link

Name clash with AWSSDKSwift v4.x #119

Closed adam-fowler closed 3 years ago

adam-fowler commented 4 years ago

The AWSLambdaEvents define enum namespaces for the Events (S3, SNS, SES, SQS...). These mean you cannot use v4.x of AWSSDKSwift with swift-aws-lambda-runtime. This is partly the fault of AWSSDKSwift as in v4.x the framework name is the same as the main struct name, so you cannot differentiate between S3 in AWSSKDSwift and S3 in AWSLambdaEvents.

This is resolved with the AWSSDKSwift v5.0 alpha as the framework names have been prepended with AWS. Thus won't be an issue in the future.

I'm not sure if anything will be done about this now, given the issue will disappear in the future, but I must say using such general terms for a very specific part of a service wasn't necessarily the best idea.

fabianfett commented 4 years ago

I must say using such general terms for a very specific part of a service wasn't necessarily the best idea.

@adam-fowler Any suggestions for improvement?

adam-fowler commented 4 years ago

@fabianfett I would have thought removing one layer, something like this, would work

public struct SNSEvent: Decodable {
    public struct Record: Decodable {
        ...
        let sns: Message
    }
    public struct Message {
        ...
    }
}
eneko commented 4 years ago

While might not be the best solution, Swift does allow prefixing types with module names, to differentiate types (eg. AWSSKDSwift.S3 vs AWSLambdaEvents.S3). In the example above, @adam-fowler, you could use AWSLambdaEvents.SNS.Event to refer to this type.

Furthermore, Swift supports creating type aliases as needed:

typealias SNSEvent = AWSLambdaEvents.SNS.Event

Hope that helps.

adam-fowler commented 4 years ago

@eneko unfortunately AWSSDKSwift v4.x is split into multiple libraries, one for each service, with each library named after the service. eg SNS, S3, etc. And the problem is the library's main class is also called the same name. This means to access the AWSSDKSwift S3 I would have to use S3.S3 but this confuses the compiler and it doesn't know which S3 I am talking about.

This has been resolved in the alpha of version 5.x as we have prefixed all the library names with 'AWS'.

eneko commented 4 years ago

I see, thanks for clarifying.

tomerd commented 4 years ago

being able to easily use AWSSDKSwift together with AWSLambdaEvents is a good goal

unfortunately name clashes of this sort are bound to happen in libraries that are in similar domain until Swift gets a better name-spacing solution. the work mentioned above to make it easier to disambiguate the module and main type in AWSSDKSwift is important and will certainly help

we could also explore adding a namespace of sorts for the events/triggers defined in AWSLambdaEvents

zdnk commented 4 years ago

Even having a seprarate file where you would import AWSSDKSwift only and make typealiases for that does not work? (or for AWSLambdaEvents)

adam-fowler commented 4 years ago

Even having a seprarate file where you would import AWSSDKSwift only and make typealiases for that does not work? (or for AWSLambdaEvents)

Yes that would work, but would be a pain in the ass. I posted this issue without really thinking there was much that could be done, but to make sure issues like this were thought about for future commits. With the alpha version of AWSSDKSwift 5 this issue disappears.

fabianfett commented 4 years ago

@tomerd @kneekey23 Also informed me that these names might become a problem in the future. She proposed that we add an "Event" immediately after the service name like S3Event instead of namespacing like S3.Event. I guess we should revisit this before 1.0 latest.

tomerd commented 4 years ago

I guess we should revisit this before 1.0 latest.

sgtm

eneko commented 3 years ago

Now that Soto 5.0.0 has been released, this issue could be closed.

Might be still a good idea to consider updating these types to avoid collisions with other frameworks or libraries.

kneekey23 commented 3 years ago

I still would like this to be revisited and I would prefer the name spacing of S3Event to S3.Event to prevent future naming collisions with other frameworks/libraries on the horizon.

fabianfett commented 3 years ago

We fixed this in swift-server/swift-aws-lambda-events#2