feature: Adding codegen config support for spm module type versions #539

BobaFetters · 2024-11-15T20:40:57Z

Adding new codegen config options for supplying a version to the SPM module type to allow for pointing to specific branches or local versions.

Closes apollographql/apollo-ios#3017

Setup codegen config options and tests for new spm module type version

svc-apollo-docs · 2024-11-15T20:41:01Z

✅ Docs Preview Ready

Configuration

{
  "repoOverrides": {
    "apollographql/apollo-ios-dev@main": {
      "remote": {
        "owner": "apollographql",
        "repo": "apollo-ios-dev",
        "branch": "feature/package-config-3017"
      }
    }
  }
}

Name	Link
Commit	`bb90255`
Preview	https://www.apollographql.com/docs/deploy-preview/18faba0733cc22d69279
Build ID	18faba0733cc22d69279
File Changes	0 new, 15 changed, 0 removed * (developer-tools)/ios/(latest)/index.mdx * (developer-tools)/ios/(latest)/code-generation/codegen-configuration.mdx * (developer-tools)/ios/(latest)/fetching/queries.mdx * graphos/reference/router/telemetry/instrumentation/standard-instruments.mdx * graphos/reference/router/rhai.mdx * graphos/reference/router/errors.mdx * graphos/reference/router/configuration.mdx * graphos/routing/customization/coprocessor.mdx * graphos/routing/customization/overview.mdx * graphos/routing/observability/subgraph-error-inclusion.mdx * graphos/routing/observability/client-id-enforcement.mdx * graphos/routing/performance/caching/entity.mdx * graphos/routing/security/authorization.mdx * graphos/routing/security/request-limits.mdx * graphos/routing/self-hosted/containerization/index.mdx
Pages	15

15 pages published. Build will be available for 30 days.

netlify · 2024-11-15T20:41:13Z

✅ Deploy Preview for apollo-ios-docc canceled.

Name	Link
🔨 Latest commit	`bb90255`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-ios-docc/deploys/67476639fb51520008257e78

netlify · 2024-11-15T20:41:14Z

✅ Deploy Preview for eclectic-pie-88a2ba ready!

Name	Link
🔨 Latest commit	`bb90255`
🔍 Latest deploy log	https://app.netlify.com/sites/eclectic-pie-88a2ba/deploys/6747663999d0f50008d32dbf
😎 Deploy Preview	https://deploy-preview-539--eclectic-pie-88a2ba.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

calvincestari

Minor comment on the case naming but otherwise I think this looks good so far. Still the swift package manager module template changes to implement right?

...o-ios-codegen/Sources/ApolloCodegenLib/CodegenConfiguration/ApolloCodegenConfiguration.swift

Finished implementation in SchemaModuleTemplate for new spm module type versions

AnthonyMDev

Thanks for doing this Zach! A couple things I'd like us to consider here before merging this.

Tests/ApolloCodegenTests/ApolloCodegenTests.swift

...o-ios-codegen/Sources/ApolloCodegenLib/CodegenConfiguration/ApolloCodegenConfiguration.swift

apollo-ios-codegen/Sources/ApolloCodegenLib/Constants.swift

apollo-ios-codegen/Sources/ApolloCodegenLib/Templates/SwiftPackageManagerModuleTemplate.swift

...o-ios-codegen/Sources/ApolloCodegenLib/CodegenConfiguration/ApolloCodegenConfiguration.swift

apollo-ios-codegen/Sources/ApolloCodegenLib/Constants.swift

Addressed

AnthonyMDev

Reviewing this again got me thinking. Is it worth adding an option for users to point their Apollo dependency to their own fork? Right now you can point to a branch or commit hash, but it's still only on the url https://github.com/apollographql/apollo-ios.git.

@BobaFetters @calvincestari do you think that's worth doing at this time?

...o-ios-codegen/Sources/ApolloCodegenLib/CodegenConfiguration/ApolloCodegenConfiguration.swift

calvincestari · 2024-11-19T17:24:05Z

Reviewing this again got me thinking. Is it worth adding an option for users to point their Apollo dependency to their own fork? Right now you can point to a branch or commit hash, but it's still only on the url https://github.com/apollographql/apollo-ios.git.

@BobaFetters @calvincestari do you think that's worth doing at this time?

I don't see any reason why we'd want to restrict it to https://github.com/apollographql/apollo-ios.git. Unless it's a strategic choice but that seems incongruous with OSS.

calvincestari · 2024-11-19T17:28:57Z

I just had the thought that maybe allowing something other than https://github.com/apollographql/apollo-ios.git makes the CLI version checking less useful BUT is this whole PR change doing that? How do we now ensure generated code compatibility with the runtime used by that generated code?

AnthonyMDev · 2024-11-19T17:40:35Z

I just had the thought that maybe allowing something other than https://github.com/apollographql/apollo-ios.git makes the CLI version checking less useful BUT is this whole PR change doing that? How do we now ensure generated code compatibility with the runtime used by that generated code?

Likely you're just making some changes in your fork that don't affect codegen compatibility, but yeah any manual dependency here means you may generate incompatible code. That's something the user needs to accept and deal with on their own. If you're using anything but the default package version, we shouldn't even be using the version checker at all actually.

The version checker isn't foolproof right now anyways. It only runs on the CLI, not Swift Scripts, and if it can't find a package.resolved file, it won't run at all. But worse case, you generate code that doesn't compile properly and you know it when you try to build. This is just to attempt to detect that early and give you a clear error message.

But I'm actually wondering if we should move this into ApolloCodegenLib now. We are currently only running it when using CLI, but could probably run it while using Swift Scripts also. Doesn't need to be in this PR if we decide to do so.

calvincestari · 2024-11-19T18:20:23Z

But I'm actually wondering if we should move this into ApolloCodegenLib now. We are currently only running it when using CLI, but could probably run it while using Swift Scripts also. Doesn't need to be in this PR if we decide to do so.

Yes we probably should, and in a separate PR.

BobaFetters · 2024-11-20T00:08:10Z

Reviewing this again got me thinking. Is it worth adding an option for users to point their Apollo dependency to their own fork? Right now you can point to a branch or commit hash, but it's still only on the url https://github.com/apollographql/apollo-ios.git.

@BobaFetters @calvincestari do you think that's worth doing at this time?

I don't see a reason not to add that, I guess maybe add a url associated value that defaults to apollo-ios to the existing cases? Although we dont currently have a case that supports version numbers since for Apollo we want it to match the codegen version, so may need a new case that can support the from and exact versions.

...o-ios-codegen/Sources/ApolloCodegenLib/CodegenConfiguration/ApolloCodegenConfiguration.swift

AnthonyMDev

Love this API now. I think the changes we made are great and this is the right way forward now. Great work @BobaFetters!

I've got a couple minor suggestions, and we're still waiting on documentation. But the core of this is looking good!

Tests/ApolloCodegenTests/ApolloCodegenConfigurationCodableTests.swift

...o-ios-codegen/Sources/ApolloCodegenLib/CodegenConfiguration/ApolloCodegenConfiguration.swift

calvincestari · 2024-11-27T18:09:55Z

docs/source/code-generation/codegen-configuration.mdx

@@ -106,7 +106,7 @@ The properties to configure `output` are:
 "output": {
  "schemaTypes": {
    "moduleType": {
-      "swiftPackageManager": {}
+      "swiftPackage": {}


It's long bugged me that we had swiftPackageManager as a ModuleType option and swiftPackage as a TestMocks option; I'm happy these are more similar now. 😄

docs/source/code-generation/codegen-configuration.mdx

calvincestari

Looks good to me, nice work!

calvincestari · 2024-11-27T18:59:50Z

...o-ios-codegen/Sources/ApolloCodegenLib/CodegenConfiguration/ApolloCodegenConfiguration.swift

@@ -283,7 +283,12 @@ public struct ApolloCodegenConfiguration: Codable, Equatable {
      case embeddedInTarget(name: String, accessModifier: AccessModifier = .internal)
      /// Generates a `Package.swift` file that is suitable for linking the generated schema types
      /// files to your project using Swift Package Manager.
+      /// Attention: This case has been deprecated, use .swiftPackage(apolloSDKVersion:) case instead.
      case swiftPackageManager


How do we eventually remove these cases if we can't signal ahead of time with the deprecation attribute? Removing them in a future major version seems impossible if we're relying on people reading the documentation.

Think it just has to be removed in a major version and noted in release notes. Not much else to do without adding the actual deprecation warning.

…539)

…pollographql/apollo-ios-dev#539)

…for spm module type versions git-subtree-dir: apollo-ios git-subtree-mainline: c528c4d git-subtree-split: 38eef8f5109fd9677ce9a709f2632b8783536ac3

cfe27487 feature: Adding codegen config support for spm module type versions (#539) git-subtree-dir: apollo-ios-codegen git-subtree-split: cfe274876fa5e45b4bebef97beae278331681fd1

…support for spm module type versions git-subtree-dir: apollo-ios-codegen git-subtree-mainline: dbf486f git-subtree-split: cfe274876fa5e45b4bebef97beae278331681fd1

Codegen config options and tests

b32dcc9

Setup codegen config options and tests for new spm module type version

BobaFetters requested review from calvincestari and AnthonyMDev November 15, 2024 20:40

BobaFetters self-assigned this Nov 15, 2024

calvincestari reviewed Nov 15, 2024

View reviewed changes

...o-ios-codegen/Sources/ApolloCodegenLib/CodegenConfiguration/ApolloCodegenConfiguration.swift Outdated Show resolved Hide resolved

Finish Implementation

a9ddd8d

Finished implementation in SchemaModuleTemplate for new spm module type versions

BobaFetters marked this pull request as ready for review November 16, 2024 04:36

BobaFetters requested a review from calvincestari November 16, 2024 04:36

AnthonyMDev previously requested changes Nov 17, 2024

View reviewed changes

Addressing feedback

60579a5

BobaFetters commented Nov 18, 2024

View reviewed changes

...o-ios-codegen/Sources/ApolloCodegenLib/CodegenConfiguration/ApolloCodegenConfiguration.swift Outdated Show resolved Hide resolved

BobaFetters requested a review from AnthonyMDev November 18, 2024 18:10

AnthonyMDev reviewed Nov 18, 2024

View reviewed changes

Addressing feedback

5f2bc7b

BobaFetters requested a review from AnthonyMDev November 19, 2024 16:29

AnthonyMDev reviewed Nov 19, 2024

View reviewed changes

...o-ios-codegen/Sources/ApolloCodegenLib/CodegenConfiguration/ApolloCodegenConfiguration.swift Outdated Show resolved Hide resolved

...o-ios-codegen/Sources/ApolloCodegenLib/CodegenConfiguration/ApolloCodegenConfiguration.swift Outdated Show resolved Hide resolved

Adding custom package type case

f8e9713

BobaFetters commented Nov 20, 2024

View reviewed changes

...o-ios-codegen/Sources/ApolloCodegenLib/CodegenConfiguration/ApolloCodegenConfiguration.swift Outdated Show resolved Hide resolved

Change case name and add tests

38a8700

BobaFetters requested a review from AnthonyMDev November 21, 2024 20:24

Addressing feedback

d4fa49f

AnthonyMDev reviewed Nov 25, 2024

View reviewed changes

Tests/ApolloCodegenTests/ApolloCodegenConfigurationCodableTests.swift Outdated Show resolved Hide resolved

...o-ios-codegen/Sources/ApolloCodegenLib/CodegenConfiguration/ApolloCodegenConfiguration.swift Outdated Show resolved Hide resolved

BobaFetters added 2 commits November 26, 2024 19:17

Custom codable for SDKVersion enum

7be0e0c

Documentation

d2e9dcf

BobaFetters requested a review from AnthonyMDev November 27, 2024 17:33

calvincestari reviewed Nov 27, 2024

View reviewed changes

Addressing docs feedback

bb90255

calvincestari approved these changes Nov 27, 2024

View reviewed changes

BobaFetters merged commit 58adaf3 into main Nov 27, 2024
34 checks passed

BobaFetters deleted the feature/package-config-3017 branch November 27, 2024 22:54

BobaFetters added a commit that referenced this pull request Nov 27, 2024

feature: Adding codegen config support for spm module type versions (#…

c528c4d

…539)

BobaFetters added a commit to apollographql/apollo-ios-codegen that referenced this pull request Nov 27, 2024

feature: Adding codegen config support for spm module type versions (a…

cfe2748

…pollographql/apollo-ios-dev#539)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Adding codegen config support for spm module type versions #539

feature: Adding codegen config support for spm module type versions #539

BobaFetters commented Nov 15, 2024 •

edited

Loading

svc-apollo-docs commented Nov 15, 2024 •

edited

Loading

netlify bot commented Nov 15, 2024 •

edited

Loading

netlify bot commented Nov 15, 2024 •

edited

Loading

calvincestari left a comment

AnthonyMDev left a comment

AnthonyMDev left a comment

calvincestari commented Nov 19, 2024

calvincestari commented Nov 19, 2024

AnthonyMDev commented Nov 19, 2024

calvincestari commented Nov 19, 2024

BobaFetters commented Nov 20, 2024

AnthonyMDev left a comment

calvincestari Nov 27, 2024

calvincestari left a comment

calvincestari Nov 27, 2024

BobaFetters Nov 27, 2024

feature: Adding codegen config support for spm module type versions #539

feature: Adding codegen config support for spm module type versions #539

Conversation

BobaFetters commented Nov 15, 2024 • edited Loading

svc-apollo-docs commented Nov 15, 2024 • edited Loading

✅ Docs Preview Ready

netlify bot commented Nov 15, 2024 • edited Loading

✅ Deploy Preview for apollo-ios-docc canceled.

netlify bot commented Nov 15, 2024 • edited Loading

✅ Deploy Preview for eclectic-pie-88a2ba ready!

calvincestari left a comment

Choose a reason for hiding this comment

AnthonyMDev left a comment

Choose a reason for hiding this comment

AnthonyMDev left a comment

Choose a reason for hiding this comment

calvincestari commented Nov 19, 2024

calvincestari commented Nov 19, 2024

AnthonyMDev commented Nov 19, 2024

calvincestari commented Nov 19, 2024

BobaFetters commented Nov 20, 2024

AnthonyMDev left a comment

Choose a reason for hiding this comment

calvincestari Nov 27, 2024

Choose a reason for hiding this comment

calvincestari left a comment

Choose a reason for hiding this comment

calvincestari Nov 27, 2024

Choose a reason for hiding this comment

BobaFetters Nov 27, 2024

Choose a reason for hiding this comment

BobaFetters commented Nov 15, 2024 •

edited

Loading

svc-apollo-docs commented Nov 15, 2024 •

edited

Loading

netlify bot commented Nov 15, 2024 •

edited

Loading

netlify bot commented Nov 15, 2024 •

edited

Loading