Removing Swift codegen (v1) #1873

calvincestari · 2021-07-15T21:18:15Z

This PR removes the initial implementation of Swift codegen from the main branch.

It's labelled as 'experimental' and 'use at own risk' but it's not functional yet.
This code represents the implementation of the RFC: Swift Codegen Rewrite which is changing.
Gets main back to a state of supported codegen options.

It does this in distinct commits, some of which will be reverted in another branch..

Remove the expected code output, and related tests
Remove code generators for expected code output, and related tests
Remove two targets that were configured to use the Swift codegen
Remove 'swiftExperimental' as a codegen configuration

Swift codegen is not dead; there will be a new branch where v2 of the RFC will be built until it is ready for an alpha release.

AnthonyMDev

Just want to make sure this AST stuff isn't used elsewhere, I'm pretty sure this is good though!

And lets fix that test instead of deleting it?

AnthonyMDev · 2021-07-15T21:52:31Z

Sources/ApolloCodegenLib/ASTCondition.swift

@@ -1,18 +0,0 @@
-import Foundation


@designatednerd Are these AST objects being used by the Javascript Frontend? Is there even anything that is creating them currently, other than unit tests for the experimental codegen? I think we can delete these from main, but just want to make sure.

They should not be, this was being created off reading everything in using the TS CLI, you should be able to nuke them from orbit

Tests/ApolloCodegenTests/ApolloCodegenTests.swift

designatednerd

@AnthonyMDev I think all the issues I tried to address for enums are at least still in the codegen project, but it might be useful to think about leaving a bunch of test names and just throw XCTSkip on those tests for now until it's actually implemented

designatednerd · 2021-07-15T22:24:41Z

SwiftScripts/Sources/Codegen/Target.swift

        case "GitHub":
            self = .gitHub
        case "Upload":
            self = .upload
-        case "AnimalKingdom":
-            self = .animalKingdom


did we mean to delete this?

I did, it was only there to be used with swiftExperimental. It comes back in an RFC-focused branch.

designatednerd · 2021-07-15T22:25:32Z

Tests/ApolloCodegenTests/ApolloCodegenTests.swift

@@ -67,7 +67,7 @@ class ApolloCodegenTests: XCTestCase {
    let namespace = "ANameSpace"
    let prefix = "MyPrefix"

-    let options = ApolloCodegenOptions(codegenEngine: .swiftExperimental,


for what it's worth I think it's worth keeping the swiftExperimental case since it's still eventually going to be used, just for a completely different setup

It comes back in an RFC-focused branch. My opinion was that if it stayed in main then it should blow up, aka fatalError, because it does nothing at this point. That led me to think it was better to nuke from main and bring it back where we actually use it.

eh, fair enough. I'm always more in favor of just leaving it but I see your point too.

designatednerd · 2021-07-16T02:12:45Z

Sources/ApolloCodegenLib/ApolloCodegenOptions.swift

@@ -58,7 +58,6 @@ public struct ApolloCodegenOptions {
  }

  let codegenEngine: CodeGenerationEngine
-  let additionalInflectionRules: [InflectionRule]


just please please please remember to re-add this, it solves so many goddamned problems

AnthonyMDev · 2021-07-16T20:35:42Z

If we are (temporarily) not using some of these dependencies any more, we should remove them from SPM and the podfile.

I'm pretty sure that Stencil, PathKit, and InflectorKit are all not being used after we kill this code. We're going to need to re-add them on the branch for the codegen RFC at some point, but we don't need our consumers to be downloading and including them in their projects right now for the time being.

designatednerd

Revoking my approval: Please don't remove InflectorKit and the related code completely. That can be used for pluralization/depluralization as needed, which we absolutely will need regardless of whether we use stencil or not. I'm fine with removing the code from the initializer for now since it's not being used, but this is a case where I really don't think removing it just to put it back in later exactly as it was is worth it.

I'm fine with losing Stencil (which IIRC is where the PathKit dependency comes from) since @AnthonyMDev it sounds like we're not going to use it going forward.

calvincestari · 2021-07-19T17:27:04Z

@designatednerd I feel quite strongly that we shouldn't have code in main that isn't actively used. I understand your argument re. "I really don't think removing it just to put it back in later exactly as it was is worth it" but we don't yet know if it'll go back in exactly as it was; please note I'm not disputing it's value.

@AnthonyMDev - do you have an opinion on this or the workflow?

AnthonyMDev · 2021-07-19T19:17:42Z

I don’t really feel strongly either way. I see both sides and don’t really have any additional arguments to add to either of them.

FWIW, I think I lean towards removing them, even if we are going to re-add them later, but not passionate about that.

Also, the more I think about it and research, the more I think it’s likely we will end up using Stencil.

designatednerd · 2021-07-19T21:35:12Z

In my research InflectorKit is about the only thing that is out there for this kind of pluralization/depluralization in Swift (that's why I originally revived it and contributed swift wrappers to it; It was subsequently updated to be in Swift). There is absolutely no question in my mind we will need something for pluralization/depluralization and probably sooner rather than later.

I'm fine with ripping out the other stuff as there are other ways to get from point a to point b, but to me it feels like a waste to rip out something that's already set up and tested just to put it right back in when we start needing to do basic pluralization/depluralization of types, which we'll need to do quite quickly when we get the codegen going.

liuliu · 2021-07-22T19:30:51Z

It is pretty hard to navigate, but where is the "legacy" codegen resides? I suppose that is some Typescript code somewhere in some other repo, but have a hard time to find exact where it is and how we packaged up to legacy_cli that we use.

Edit:

Found it in https://github.com/apollographql/apollo-tooling/tree/master/packages/apollo-codegen-swift, apollo-tooling (instead of apollo-cli) is where tripped me initially.

* main: (856 commits) Add execution tests for ApolloClient clearCache callback queue (apollographql#1901) Use the provided callback queue instead of the store's default. (apollographql#1904) chore(deps): update dependency path-parse to 1.0.7 [security] (apollographql#1899) Release - 0.46.0 (apollographql#1897) Update subscriptions tutorial to be compatible with recent changes (apollographql#1893) Add docs and improve merging of records from WebSockets into cache. (apollographql#1892) Publish response from the `WebSocketTransport` to the `ApolloStore` (apollographql#1889) fix(deps): update dependency gatsby-theme-apollo-docs to v4.7.14 Removing Swift codegen (v1) (apollographql#1873) Update toolchain versions used by circleci (apollographql#1875) fix(deps): update dependency gatsby-theme-apollo-docs to v4.7.13 Community Updates - ROADMAP/README (apollographql#1867) [Release] - 0.45.0 (apollographql#1862) WebSocket Fixes - Revert to Starscream 3.x and invert dependency (apollographql#1861) Docs/discussions_2_community branch changes (apollographql#1858) Replace spectrum with Discourse (apollographql#1857) fix(deps): update dependency gatsby-theme-apollo-docs to v4.7.12 Configure Renovate (apollographql#1854) Revert "Reconfiguring renovate 2/2" Reconfiguring renovate 2/2 ... # Conflicts: # Sources/Apollo/GraphQLQueryWatcher.swift # Sources/ApolloWebSocket/WebSocketTransport.swift

calvincestari added 3 commits July 14, 2021 15:09

Remove expected output code and related tests

caa8615

Remove code generators and related tests

798391c

Remove starWarsSwiftCodegen - using swiftExperimental

2a2edfb

calvincestari requested review from AnthonyMDev and designatednerd July 15, 2021 21:18

AnthonyMDev reviewed Jul 15, 2021

View reviewed changes

designatednerd reviewed Jul 15, 2021

View reviewed changes

designatednerd approved these changes Jul 16, 2021

View reviewed changes

hwillson added this to the MM-2021-07 milestone Jul 16, 2021

calvincestari force-pushed the remove-swift-codegen-v1 branch from 1d6eb19 to 3133906 Compare July 16, 2021 20:53

calvincestari added the include-in-changelog Indicates that changes from a PR should be noted in the changelog for their release. label Jul 16, 2021

calvincestari added 4 commits July 16, 2021 14:29

Remove Animal Kingdom - using swiftExperimental

05c46a1

Remove swiftExperimental from codegen

f084ca7

Don't remove the useful test

ccca7e7

Remove inflection option, pluralizer and dependency

42a75f2

calvincestari force-pushed the remove-swift-codegen-v1 branch from 3133906 to 42a75f2 Compare July 16, 2021 21:58

calvincestari added 2 commits July 16, 2021 15:20

Remove unused dependencies

02e9d6a

Remove unused extensions and related tests

0452aad

designatednerd suggested changes Jul 18, 2021

View reviewed changes

AnthonyMDev approved these changes Jul 20, 2021

View reviewed changes

calvincestari merged commit 88ee167 into main Jul 20, 2021

calvincestari deleted the remove-swift-codegen-v1 branch July 20, 2021 19:36

hwillson added the 2021-07 label Jul 29, 2021

hwillson removed this from the MM-2021-07 milestone Jul 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing Swift codegen (v1) #1873

Removing Swift codegen (v1) #1873

calvincestari commented Jul 15, 2021 •

edited

Loading

AnthonyMDev left a comment

AnthonyMDev Jul 15, 2021

designatednerd Jul 15, 2021

designatednerd left a comment

designatednerd Jul 15, 2021

calvincestari Jul 15, 2021 •

edited

Loading

designatednerd Jul 15, 2021

calvincestari Jul 15, 2021 •

edited

Loading

designatednerd Jul 16, 2021

designatednerd Jul 16, 2021

AnthonyMDev commented Jul 16, 2021

designatednerd left a comment

calvincestari commented Jul 19, 2021

AnthonyMDev commented Jul 19, 2021

designatednerd commented Jul 19, 2021 •

edited

Loading

liuliu commented Jul 22, 2021 •

edited

Loading

Removing Swift codegen (v1) #1873

Removing Swift codegen (v1) #1873

Conversation

calvincestari commented Jul 15, 2021 • edited Loading

AnthonyMDev left a comment

Choose a reason for hiding this comment

AnthonyMDev Jul 15, 2021

Choose a reason for hiding this comment

designatednerd Jul 15, 2021

Choose a reason for hiding this comment

designatednerd left a comment

Choose a reason for hiding this comment

designatednerd Jul 15, 2021

Choose a reason for hiding this comment

calvincestari Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

designatednerd Jul 15, 2021

Choose a reason for hiding this comment

calvincestari Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

designatednerd Jul 16, 2021

Choose a reason for hiding this comment

designatednerd Jul 16, 2021

Choose a reason for hiding this comment

AnthonyMDev commented Jul 16, 2021

designatednerd left a comment

Choose a reason for hiding this comment

calvincestari commented Jul 19, 2021

AnthonyMDev commented Jul 19, 2021

designatednerd commented Jul 19, 2021 • edited Loading

liuliu commented Jul 22, 2021 • edited Loading

calvincestari commented Jul 15, 2021 •

edited

Loading

calvincestari Jul 15, 2021 •

edited

Loading

calvincestari Jul 15, 2021 •

edited

Loading

designatednerd commented Jul 19, 2021 •

edited

Loading

liuliu commented Jul 22, 2021 •

edited

Loading