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

Move Generated Client to separate module #486

Open
wants to merge 21 commits into
base: trunk
Choose a base branch
from

Conversation

andrewdmontgomery
Copy link
Contributor

@andrewdmontgomery andrewdmontgomery commented Oct 10, 2024

Closes #477

Description

This PR's main purpose is to encapsulate the generated code into a separate module, and then expose types with appropriate access levels using typealias declarations in the Gravatar module.

To accomplish this, this PR also makes the following changes:

  • Simplifies the generate command:
    • Calls: docker run --rm openapitools/openapi-generator-cli:"v7.5.0" generate
      • It fetches the official docker container instead of using the locally fetched generator in a custom docker container
      • The install-and-generate Makefile task has been removed, as it is no longer required
    • Generates all file types by default (instead of just models)
      • Uses .openapi-generator-ignore to selectively ignore which file/folders should be created
    • Passes new options to --additional-options
  • Creates Podspec.mustache to override the default generated Podspec during generate
  • Creates Sources/Gravatar/OpenAPIExports.swift for the typealias exports
  • Moves generated source files out of Gravatar
  • Moves all openapi-generate activity into the opeapi directory
    • Files are generated here, and are copied elsewhere as needed
      • The generated Podspec is copied to the root of the repo
      • The generated source files are copied to Sources/GravatarOpenAPIClient

Notes:

  • This will create a new public CocoaPod for us: GravatarOpenAPIClient
  • After this merges, you can (and should) delete the folder openapi-generator. Since docker is now fetching an image that contains the openapi-generator-cli, we no longer need to fetch our own copy, etc.
  • It might make sense to make this GravatarOpenAPIClient an entirely separate package/repo. But we can iterate.

Module vs Swift Package Dependency

The generate command produces a Package.swift for the OpenAPIClient. I started with this approach, including the OpenAPIClient as a local package dependency.

But the OpenAPIClient implementation has a third-party dependency: AnyCodable. However, the only reference seems to be a single import statement, which is wrapped in #if canImport(AnyCodable). This dependency doesn't seem to be necessary. And we don't want it.

So instead of modifying the Package.swift, I've decided to use a module instead of a local package dependency.

About the podspec

The default generated podspec had some limitations that were hard to work around:

  • The target os versions are lower than ours. This shouldn't be a problem. But our linting didn't like that at all. So the template makes it match our SDK
  • The value forswift-versions (in the podspec) should ideally be passed as a parameter to --additional-properties when calling generate. The problem is that the value includes a comma, which is the delimmeter between properties. In theory, placing the whole value in quotes should work: swiftVersion="[ '5.10', '5.9' ]". If I do that, the comma is included in the value correctly, but the value is added to the podspec with the double-quotes (see below). So again, we are using the same version.rb` to set this.
  • Since the path to the source files of the podspec is nested, I needed to just customize the path. But since we are also using a 'Swift Package" format, setting the path using --additional-properties overrides the path to the Swift Package. So our template allows for this. (At the moment, we don't specify more than one version of swift. But specifying multiple versions of swift is supported.)
swift-versions: "[ '5.10', '5.9' ]"

Testing Steps

@andrewdmontgomery andrewdmontgomery requested review from pinarol and etoledom and removed request for pinarol October 10, 2024 01:19
@wpmobilebot
Copy link

wpmobilebot commented Oct 10, 2024

Gravatar UIKit Prototype Build📲 You can test the changes from this Pull Request in Gravatar UIKit Prototype Build by scanning the QR code below to install the corresponding build.
App NameGravatar UIKit Prototype Build Gravatar UIKit Prototype Build
Build Number1476
Version1.0
Bundle IDcom.automattic.gravatar-sdk-demo-uikit.prototype-build
Commitb3b9f4c
App Center BuildGravatar SDK Demo - UIKit #255
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link

wpmobilebot commented Oct 10, 2024

Gravatar SwiftUI Prototype Build📲 You can test the changes from this Pull Request in Gravatar SwiftUI Prototype Build by scanning the QR code below to install the corresponding build.
App NameGravatar SwiftUI Prototype Build Gravatar SwiftUI Prototype Build
Build Number1476
Version1.0
Bundle IDcom.automattic.gravatar-sdk-demo-swiftui.prototype-build
Commitb3b9f4c
App Center BuildGravatar SDK Demo - SwiftUI #255
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@andrewdmontgomery andrewdmontgomery changed the base branch from trunk to release/3.0.0 October 10, 2024 01:50
@andrewdmontgomery andrewdmontgomery changed the base branch from release/3.0.0 to trunk October 10, 2024 01:50
Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

This looks pretty good! 🎉

I have just one question:

Generates all file types by default (instead of just models)

Any special motive for this?
We are not using the client code generated (we decided to keep our own client we developed in previous iterations). And it adds many concurrency warnings.

I saw the Swift6 generator PR, it adds @unchecked Sendable to many types 🤔

One other thing: There's an update on the templates on the v3.0.0 branch. It would be good to update from it before continuing with this one.

@andrewdmontgomery
Copy link
Contributor Author

andrewdmontgomery commented Oct 10, 2024

Generates all file types by default (instead of just models)

Any special motive for this?
We are not using the client code generated (we decided to keep our own client we developed in previous iterations). And it adds many concurrency warnings.

Good question. This came about because I wanted to use the generated PodSpec and potentially the generated Package.swift. To get those, I needed to make the following change:

generate -i openapi.yaml \
-    --global-property models \
+    --global-property models,supportingFiles \
    -t templates \
    -g swift5 \
    -o ./generated \
    -p packageName=Gravatar \
	--additional-properties=useJsonEncodable=false,readonlyProperties=true

Doing that generated the files I needed. But it also generates a lot of other support files.

The official documentation for global properties suggests using a different approach:

Allows the user to define which supporting files will be generated.
Prefer using the more robust .openapi-generator-ignore.

So that's what I've done. If you look at openapi/GravatarOpenAPIClient/.openapi-generator-ignore, you'll see that I have already surpassed a few paths:

.gitignore
.swiftformat
Cartfile
git_push.sh
Sources/GravatarOpenAPIClient/APIs/

For example, you can see there that it surpresses the APIs directory from being generated. If we want, we can suppress more.

And it adds many concurrency warnings

That's a good point. We already have some that we need to deal with, and I wondered if the swift6 generator handle those. So I tried (as you saw) and it looked promising.

But if we don't need these source files, generating them doesn't help us. And since none of this is available outside the SDK package, they don't help anyone else either. Good call, let's remove them.

I will update the .openapi-generator-ignore file with the following:

.gitignore
.swiftformat
Cartfile
git_push.sh
- Sources/GravatarOpenAPIClient/APIs/
+ Sources/GravatarOpenAPIClient/
+ !Sources/GravatarOpenAPIClient/Models/

This will suppress all generated source files except the Models directory, just like we have now.

One other thing: There's an update on the templates on the v3.0.0 branch. It would be good to update from it before continuing with this one.

Oh nice. I'll retarget v3.0.0 and merge it into this branch. If we decide to merge it before release, we'll be ready. And if we decide to wait, we'll just merge it to trunk after.

@andrewdmontgomery andrewdmontgomery changed the base branch from trunk to release/3.0.0 October 10, 2024 17:37
Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

I love this! ❤️

Thank you for this!


s.ios.deployment_target = ios_deployment_target

s.dependency 'GravatarOpenAPIClient', s.version.to_s
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIU, GravatarOpenAPIClient's version is Gravatar's version. This makes me wonder what happens when the specs change and we re-generate GravatarOpenAPIClient. When I use the new models in Gravatar what should i set here so that cocoapods also builds fine in CI...

@@ -2,28 +2,28 @@ import Foundation

/// An avatar that the user has already uploaded to their Gravatar account.
///
package struct Avatar: Codable, Hashable, Sendable {
package enum Rating: String, Codable, CaseIterable, Sendable {
public struct Avatar: Codable, Hashable, Sendable {
Copy link
Contributor

@pinarol pinarol Oct 14, 2024

Choose a reason for hiding this comment

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

Is there maybe a way to keep package ? I think the initial intention was to not open these because we would then need to reflect the changes to the version number. AFAIK there's nothing that prevents a 3rd party to import GravatarOpenAPIClient so i guess the question is, are we ready to maintain a 3rd public package with semantic versioning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK there's nothing that prevents a 3rd party to import GravatarOpenAPIClient

This is a good question. My understanding of Swift Packages (which turned out to be incomplete) was that only modules that are exposed as libraries (via .library() in products: [...] in the Package.swift) would be available to third parties who integrate this Package.

And since this would not expose the generated client as a .library(), then my thinking went, it would not be available to third parties via our SDK.

But as it turns out, it isn't quite that simple. It's true that by not listing it as a .library(), the GravatarOpenAPIClient library is NOT available to be added to targets:
image
image

But since Gravatar imports the GravatarOpenAPIClient module, the public types in that module are implicitly exposed – and importable – to third parties who import the Gravatar library, even though they cannot add the underlying library.

As a result, you are right. If we could retain those package access controls, these types wouldn't be importable. And therefore, this whole approach doesn't address the original problem: how can we hide the types that don't need to be public?

so i guess the question is, are we ready to maintain a 3rd public package with semantic versioning?
That's the question. I think we aren't yet ready for that.

For one, as we discussed separately, as it currently stands, the versioning of this OpenAPIClient will need to be independent of the version of the SDK. This is because:

  • By default, all types in the OpenAPIClient are public, so any change to them will affect the API
  • But changes to the OpenAPIClient don't necessarily affect the SDK API

I think there are two directions we can take this:

  1. Publish a fully public GravatarOpenAPIClient, with its own versioning (and likely its own repository)
    • Add that as a dependency of our SDK
  2. Produce a version of the GravatarOpenAPIClient with more granular access controls
    • Include that as a separate, integrated module with our SDK, like in this PR
    • Or just embed it as we currently do

Both options will require some effort, and will introduce some complexity. But I think option 2 probably serves the needs of the SDK more directly:

  • If someone really wants their own GravatarOpenAPIClient, they can make their own quite easily. So maintaining one doesn't useful.
  • Integrating a fully public version into our SDK can be confusing since we don't need or use most of what gets generated

@andrewdmontgomery
Copy link
Contributor Author

andrewdmontgomery commented Oct 18, 2024

I love this! ❤️

Thank you for this!

I will make a separate PR with only the simplification of the generate command

@andrewdmontgomery andrewdmontgomery mentioned this pull request Oct 21, 2024
2 tasks
Base automatically changed from release/3.0.0 to trunk October 24, 2024 17:43
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.

Move generated code into separate module/package
4 participants