-
Notifications
You must be signed in to change notification settings - Fork 24
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
Reorganize project files & reduce public interface #212
Conversation
@@ -186,3 +187,18 @@ extension GRPCInterceptor: StreamInterceptor { | |||
} | |||
} | |||
} | |||
|
|||
private final class Locked<T>: @unchecked Sendable { |
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.
Re-implements Locked
using NIO's locks so that the Connect.Locked
can stay internal
fdf8ed4
to
ea7b4f5
Compare
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.
Actually - quick question before approval.
@@ -75,6 +77,9 @@ let package = Package( | |||
"buf.work.yaml", | |||
"proto", | |||
"README.md", | |||
], | |||
swiftSettings: [ | |||
.unsafeFlags(["-package-name", packageName]) |
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.
Have we confirmed that this is not going to cause issues? I have heard and experienced at some point in the past that using unsafe flags causes issues with downstream consumers of the dependency.
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.
Given the Eliza SwiftPM app builds fine with the Connect
package I think it's okay? Do you know what else we should check? I'm not familiar with those issues
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.
@rebello95 could we try archiving the Eliza app as a Release IPA? I think that would tell us.
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.
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.
Ok cool!
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.
Seems you were right 🙃 #217
Reorganizes files in the project to be more clearly and consistently organized by visibility (whether the types within each file should be internal to the package or public). Also reduces the public interface of the module by moving some types to
internal
orpackage
.Note that
package
is specific to SPM, so the keyword cannot be used in conjunction with CocoaPods. For this reason, a few of the types are guarded with#if COCOAPODS
. It's also worth noting thatConnectNIO
cannot be consumed via CocoaPods today because NIO does not support CocoaPods. For this reason, the shared gRPC interfaces betweenConnect
andConnectNIO
don't need to be exposed to CocoaPods at all. Additionally,package
requires Swift 5.9, so the Swift package version has been updated as well.New directory structure: