Skip to content
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

Merged
merged 7 commits into from
Nov 14, 2023
Merged

Conversation

rebello95
Copy link
Collaborator

@rebello95 rebello95 commented Nov 3, 2023

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 or package.

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 that ConnectNIO cannot be consumed via CocoaPods today because NIO does not support CocoaPods. For this reason, the shared gRPC interfaces between Connect and ConnectNIO 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:

.
├── Examples
│   ├── ElizaCocoaPodsApp
│   ├── ElizaSharedSources
│   │   ├── AppSources
│   │   └── GeneratedSources
│   │       └── connectrpc
│   │           └── eliza
│   │               └── v1
│   └── ElizaSwiftPackageApp
│       └── ElizaSwiftPackageApp
├── Libraries
│   ├── Connect
│   │   ├── Internal
│   │   │   ├── Generated
│   │   │   │   └── grpc
│   │   │   │       └── status
│   │   │   │           └── v1
│   │   │   ├── Interceptors
│   │   │   ├── Locks
│   │   │   ├── Streaming
│   │   │   └── Unary
│   │   ├── PackageInternal
│   │   ├── Public
│   │   │   ├── Implementation
│   │   │   │   ├── Clients
│   │   │   │   ├── Codecs
│   │   │   │   └── Compression
│   │   │   └── Interfaces
│   │   │       ├── Interceptors
│   │   │       └── Streaming
│   │   │           ├── AsyncAwait
│   │   │           └── Callbacks
│   │   └── proto
│   │       └── grpc
│   │           └── status
│   │               └── v1
│   ├── ConnectMocks
│   └── ConnectNIO
│       ├── Internal
│       │   └── Extensions
│       └── Public
├── Plugins
│   ├── ConnectMocksPlugin
│   ├── ConnectPluginUtilities
│   └── ConnectSwiftPlugin
└── Tests
    ├── ConnectLibraryTests
    │   ├── ConnectConformance
    │   ├── ConnectMocksTests
    │   ├── ConnectTests
    │   ├── Generated
    │   │   ├── connectrpc
    │   │   │   └── conformance
    │   │   │       └── v1
    │   │   └── server
    │   │       └── v1
    │   └── TestResources
    └── ConnectPluginUtilitiesTests

@@ -186,3 +187,18 @@ extension GRPCInterceptor: StreamInterceptor {
}
}
}

private final class Locked<T>: @unchecked Sendable {
Copy link
Collaborator Author

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

Base automatically changed from get-support to main November 13, 2023 16:32
@rebello95 rebello95 requested a review from eseay November 13, 2023 16:38
@rebello95 rebello95 marked this pull request as ready for review November 13, 2023 16:38
Copy link
Contributor

@eseay eseay left a 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])
Copy link
Contributor

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.

Copy link
Collaborator Author

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

Copy link
Contributor

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, archiving seemed to work fine:
Screenshot 2023-11-13 at 3 45 27 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok cool!

Copy link
Collaborator Author

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

@rebello95 rebello95 requested a review from eseay November 13, 2023 20:35
@rebello95 rebello95 merged commit c3a8716 into main Nov 14, 2023
8 checks passed
@rebello95 rebello95 deleted the public-interfaces branch November 14, 2023 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants