Apollo-ios: Call For Suggestions: Swift Code Generation

[designatednerd]

Hi everyone! As I've been working on the Codegen stuff (particularly diving into the TS codebase this last week), it's become abundantly clear that we should move the "Generation" portion of the codegen out of our tooling repo and into Swift code.

I've been trying to clean up some small stuff, but clearly anything big is going to require some really significant changes. And frankly, if I'm going to make a significant change to this code, I'd rather bring it into Swift.

Please be aware that this will not start until after I finish moving back to the US (last 2 weeks of August) at the earliest, but I wanted to start soliciting feedback on my overall plan. Realistically, this is going to be blocked by the release of Xcode 11's final version, since a bunch of it is probably going to be tied to Swift Package Manager things.

Moving the "generation" bit to Swift has several benefits:

• Easier to maintain + accept 3rd party contributions. Frankly, I am teh suck at typescript and have a considerably better understanding of the way Swift handles edge cases (especially around String handling, which is critical for codegen). From all appearances, I am not alone in this in the iOS world.

  It would be way better to have the codegen in the same language as the library so both I and other contributors can more easily test, maintain, and improve it going forward. There's also a number of issues that are less than transparent to me in terms of how to support them through our current TypeScript setup, but would probably be easier to deal with once we moved to Swift.

• Easier to take advantage of SPM. Swift Package Manager integration in Xcode 11 should mean it's going to be easier to run CLI apps as part of your build script. This will make it considerably easier to add support for direct Swift integration
• Fewer version problems/shenanigans with Node. Right now every time we update the library, we also need to update our node dependency to make sure the appropriate changes have been rolled in from the codegen. This is...a bit of a mess, and it would be great to tie the codegen to the lib version more clearly.
• Opportunity to make things Swiftier. This should make it way less of a pain in the butt to add support for things like Codable, and potentially also for a few other ideas I have in terms of making it easier to represent fragments as a Protocol (NO PROMISES!!).

Of course, this also has a couple of drawbacks:

• Parsing still relies on Node. This is less than ideal, but it's more ideal than "rewriting an entire GraphQL syntax parser in Swift." Not saying that's totally out of the question, but it's a considerably larger can of worms than the codegen, and (IMO) for lower reward.

  Additionally, the parsing logic is considerably more stable than the codegen logic, so in theory the need to constantly be updating or tying specifically to a given minor version of the Apollo CLI will be reduced.

• Intermediate Representation Shenanigans. Right now our Android codegen relies on an outdated form of our intermediate representation being exported to JSON in order to generate code.

  We need to figure out a way to export the current IR to JSON so we can use that more effectively. I'll be filing an issue on the apollo-tooling repo about this. This is probably our biggest blocker, but is also probably the easiest least horrific to fix.

OK! Big wall of text, but I think that covers most of what I'm thinking. I would love to hear from you all (you can copy/paste and quote this in your responses if you'd like):

• Do you think this is a good idea?
• Optional follow-up: Why or why not?
• What, in your reading of this proposal, am I not thinking about that I should be?
• Do you have any additional suggestions for things which would make this project better?

[joninsky]

Please {
}
No
{
}

[designatednerd]

@joninsky

query WouldYouCare {
    to
    elaborate
}

[joninsky]

Ah! Yeah that was not a very constructive comment. I guess what I am trying to say is that the current generated code is slightly... off. Here is a code sample from my API.swift file

  public var createdAt: Swift.Optional<timestamptz?> {
    get {
      return graphQLMap["created_at"] as! Swift.Optional<timestamptz?>
    }
    set {
      graphQLMap.updateValue(newValue, forKey: "created_at")
    }
  }

And here is how xCode usually generates code.

    public var createdAt: Swift.Optional<timestamptz?> {
        get {
            return graphQLMap["created_at"] as! Swift.Optional<timestamptz?>
        }
        set {
            graphQLMap.updateValue(newValue, forKey: "created_at")
        }
    }

I know complaining about spacing is a reach but my larger point is ...

If the code generator managed to generate code that was closer to what swift developers like to see then it will facilitate community involvement.

So maybe a swift linter that can accept community proposals and changes and will lint any generated swift code against it?

[designatednerd]

Oh yeah, this is absolutely one of the reasons I want to move the codegen to swift - that way it will be easier for the community to make PRs when something like spacing is off. I think at that point you're talking about 2 vs 4 space indents, correct?

It's not Xcode itself generating the code - it's the Node apollo package, which contains our CLI client that includes our Swift code generator (which is currently written in TypeScript). That's why you have to add all the goofy nonsense to a Run Script build phase.

I want to make that way simpler, both from a standpoint of running and a standpoint of understanding how swift code is generated. That's basically the underlying idea of this project.

[Robin-Kunde-ck]

I've been gearing up to create an internal fork of apollo-tooling to address some Swift codegen issue, so this is welcome news. A couple of things I'd like to address:

• eliminate double optionals (why Swift.Optional<Int?> instead of just Int?)
• option to disable generation of setters (we only use immutable objects, so no setters would be a significant compile time and code size improvement)

[designatednerd]

In turn:

Optionals

The double optionals thing is a bit of a pain because of the way the server handles whether an optional property which is sent to the server is present or absent. The issue is that you can have three potential cases:

1. The value is not set
2. The value is set but nil
3. The value is set as the correct type

In case 1, the value is not sent to the server at all - this indicates to the server that whatever's already in there for that value should not be touched.

In case 2, the value is sent as null, indicating to the server that whatever's already in there for that value should be cleared.

In case 3, the value is sent as the correct type, indicating to the server that whatever's already in there for the value should be updated.

The double optional basically allows us to do this:

switch outerOptional {
case .some(innerOptional): 
    // Send value of `innerOptional` to server
case .none:
   // Do not send to server at all
}

I realize this isn't totally obvious from a caller standpoint, and I started messing around with a class I was jokingly calling a Throptional to make this a little clearer, but I'm a little wary of adding a new type that people have never heard of when using a double-optional achieves the same goal.

I have at least been working on the tooling repo to have fewer crashes if you're trying to access them before sending, though.

Getting rid of setters:

Yes, for sure. I'm almost certainly going to be taking this as an opportunity to switch to codable, which should allow responses to at the very least use var whatever: Type rather than having to constantly hit the underlying graphQLMap. I'm also considering whether for responses these things should just be lets, and if you need to make modifications, you can do it on your end, but I haven't gotten that far yet 😄

[Robin-Kunde-ck]

@designatednerd Thanks for the double optional explanation. It makes sense for the case of sending data, but it's a bummer that it makes receiving data more complicated. I think this would be another candidate for adding an option for turning this behavior off during code gen.

[designatednerd]

In _theory_, a double-optional should never get generated for anything receiving data, since you're able to specify what fields you want back in your query, whereas with an input object you can leave some things absent.

[Robin-Kunde-ck]

Ok, good to know. It might just be something we're doing then.

[designatednerd]

@Robin-Kunde-ck If you are seeing that, let me know where in a separate ticket plz

[sashaweiss]

Would love the ability to specify whether the generated types should be public or internal! I'm using Apollo in an SDK, and don't necessarily want to expose the GraphQL types to consumers of said SDK.

I bet it wouldn't have to be too granular even to be really useful - like either I'm publicly exposing some GraphQL types (and consequently I might as well expose all of them) or I'm exposing my own data types and converting them to the GraphQL models (and consequently don't need to expose any of them). My use case falls under the latter.

Awesome library so far though - thanks for all your work on this!!

[designatednerd]

@sashaweiss Yeah I think it'd have to be an all or nothing option, but this is definitely on the list (#149 first asked for this....some time ago).

[sashaweiss]

So it was! Great to hear.

[designatednerd]

For anyone interested I'm shoving all the things that I'm hoping to address into this project.

[sashaweiss]

Another idea - separating generated queries by file. Looks like TypeScript/Flow already have an option for this via apollo codegen:generate? Would be nice to have a file per query, each with the one query class in it.

Again, thanks for doing this!

[designatednerd]

Yes, for sure. Right now this is possible if you tell the script to output to a folder (which you have to pre-create) rather than to a single API.swift file, but it's super non-obvious on how to get it working.

[sashaweiss]

Oh fascinating - I saw that option in the help page, but it said it only applied to TS/Flow. If the option already exists, even better! Thanks!

[designatednerd]

Yeah I think at the moment it's semi-unofficial in that passing in the actual parameter doesn't do squat, you have to pass in a folder rather than a file for output. Planning to make that significantly more explicit in the next version

[acarrera94]

+1 for using codables with vars. That will definitely make debugging easier.
And couple of suggestions related to SwiftUI and Combine:

1. Usage of combine, under the hood and it might be nice to have queries return a Combine Publisher instead of a cancellable directly.
2. Codable structs that conform to "Identifiable", when they have an ID would be awesome.

I have accomplished both of these in a current project using extensions.
I would love to help once we get started!

[designatednerd]

Oof, I wish on Combine. We still support back to iOS 9, and Combine won't be available on anything below 13. I'm considering some kind of wrapper in the future but unfortunately that's not in the immediate cards.

Excellent idea on Identifiable, this isn't going to be out until after 5.1 lands so I think that's a perfect use case for it.

[designatednerd]

Added #710 because I have the memory of a goldfish and don't want to forget that one 😄

[acarrera94]

Another option is to look into OpenCombine But maybe an extensions project would work!

[designatednerd]

Definitely something to look into in the future, but I think for step 1 I'm reluctant to rely so completely on an experimental lib for the architecture.

I also harbor hopes and dreams that Combine will come to open-source swift in the future, but I have a feeling that'll be a bit if it happens at all

[designatednerd]

OK awesome suggestions everyone - I'm going to close this particular issue out, you can follow the remainder of the project through the Swift Codegen Github Project - if there's specific functionality you want, please open a new issue and we'll chat on that thread. Thank you all!

[rodrigoelp]

Oof, I wish on Combine. We still support back to iOS 9, and Combine won't be available on anything below 13.

@designatednerd We could still have an api which is specifically designed for iOS 13 and up. That is not a problem for the swift implementation.

Also, it would be great to have the generated file (api.swift) split into multiple files as opposed to one monolithic file. The benefit on having multiple files are: the swift compiler is faster (it will be able to compile in parallel definitions that aren't interdependent), when adding extensions you will be able to include them as TypeName+Extensions.swift which is pretty much standard in the iOS community and it is easier to review/track changes.

[designatednerd]

It is already possible to generate multiple files - please see the Advanced Codegen Tips And Tricks section of our documentation.

There is a long-term plan to build a Combine wrapper library so that users supporting 13 and above can take advantage of it, but there are a considerable number of other fish to fry before this will be possible.