Swift Codegen Upgrades

[designatednerd]

In this PR:

• Added CaseIterable conformance to enum types so all known cases can be easily iterated.
• Added comments with the original fragment or query definition to operationDefinition and fragmentDefinition
• Trimmed out excess whitespace from the final strings sent to the server for operationDefinition and fragmentDefinition
• Removed force-unwrap when the thing being unwrapped is a double optional

Note to tooling team: When this gets merged it should be released as minor-version alpha so I can make sure everything works well against our API and I haven't missed anything.

Also I am teh suck at typescript so would love any suggestions on how to do stuff better.

• Update CHANGELOG.md with your change (include reference to issue & this PR)
• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass

[lilyball]

Multiline strings would make the raw queries easier to read, but I'd prefer to strip irrelevant whitespace (see #1182) instead, because the raw queries aren't what I'm looking at the source for.

[designatednerd]

@lilyball I actually don't think those are incompatible goals - I'm planning to take a look at stripping that whitespace at the point where it's about to go out to the network next week.

[lilyball]

Once you strip all irrelevant whitespace, that should strip all newlines too, at which point the multiline string only serves to turn one line into 3. The only exception I can think of to stripping newlines is if you put a multiline string literal in your GraphQL directly that includes a newline (looking at the spec right now it uses the exact same """ token to start a multiline string literal).

Speaking of, I hope this PR handles multiline string literals in the GraphQL source. But glancing at the diff right now I'm guessing it doesn't.

[lilyball]

Note that if you simply want to avoid having to escape quotes and backticks, you can use a raw string literal instead of a multiline string literal (you just have to detect if the raw string literal terminator occurs anywhere in the source text, and if so, increase the number of hash marks used for the delimiters).

[lilyball]

Well, I suppose if the source string then includes a newline inside of a multiline string literal you'll need to escape that, using the special form of escapes that raw strings use.

[designatednerd]

@lilyball It's not about avoiding escape quotes and backticks - it's about seeing if the provided GraphQL operation is actually being properly represented properly by the generated code more easily.

Speaking of, I hope this PR handles multiline string literals in the GraphQL source. But glancing at the diff right now I'm guessing it doesn't.

Can you clarify what you mean by this a bit?

[designatednerd]

OK - I'll take a fresh look monday if the multi-line string delimiter in GQL is """ - you're correct that would need special case handling. I can add some tests around that.

[designatednerd]

Personally (and I don't believe I'm alone on this given iOS 179), I find it considerably easier to read everything with a multi-line string than with everything glommed together without whitespace. I agree that getting rid of the whitespace when it hits the network is helpful though, that way you're not sending data you don't need to, and I'll deal with that in a separate PR (partly because I think I may have to do this on the client end rather than in codegen)

[lilyball]

A multiline string is definitely better than what we have today, because what we have today is just really hard to read, but I'm not generally reading the operation definition in the generated Swift source anyway so we don't need to optimize for the prettiest output, just something reasonably legible.

As far as stripping whitespace, there would still be one space between tokens, and I think it's quite reasonable to keep some spacing around {, }, :, and , for legibility's sake, but everything else can go. I'd much rather see

public final class ChatUserInfoByIdQuery: GraphQLQuery {
  public let operationDefinition =
    "query ChatUserInfoByID($userID: ID!, $channelID: ID!) { user(id: $userID) { __typename ...IdentityWithRolesFragment isModerator(channelID: $channelID) } chatRoomBanStatus(userID: $userID, channelID: $channelID) { __typename isPermanent } }"

then see

public final class ChatUserInfoByIdQuery: GraphQLQuery {
  public let operationDefinition = """
query ChatUserInfoByID($userID: ID!, $channelID: ID!) {
  user(id: $userID) {
    __typename
    ...IdentityWithRolesFragment
    isModerator(channelID: $channelID)
  }
  chatRoomBanStatus(userID: $userID, channelID: $channelID) {
    __typename
    isPermanent
  }
}
"""

And the reason is because if I'm actually looking at the generated Swift file, it's because I want to see the Swift API, not because I want to see the original query definition. If I wanted to see that I could just open the .graphql file.

[designatednerd]

@lilyball I think I've addressed your comments, and I've also tried to address #1182.

Basically since the only thing I want the multiline string literal for is to see what's happening, a comment serves this purpose without breaking the crap out of everything else. Then trimming the excess whitespace is also considerably easier.

Let me know if you have questions or concerns - it sounds like you've spent quite a bit more time with the GraphQL spec than I have so far, so if there's anything that I'm not thinking of, with this approach, please let me know!

[lilyball]

To be honest, I very explicitly do not want this. This comment just bloats the file. I notice all your tests are relatively short snippets, but for comparison, I have a single query I'm looking at right now that spans 56 lines, and I'd really not like to have a 56-line comment in the generated source obscuring the actual datatype.

[designatednerd]

There is at least one other person who has explicitly requested multiline string support.

In your opinion, what is better when we're talking about things of similar length: A multiline string that has to be compiled or a multiline comment that doesn't?

[lilyball]

The comment approach is better. Can we make it an option as to whether to have it though?

[lilyball]

This is going to trim meaningful whitespace from multiline strings. As an example from the spec:

mutation {
  sendEmail(message: """
    Hello,
      World!

    Yours,
      GraphQL.
  """)
}

is equivalent to

mutation {
  sendEmail(message: "Hello,\n  World!\n\nYours,\n  GraphQL.")
}

but with this PR it would instead turn into

mutation {
  sendEmail(message: "Hello,\nWorld!\n\nYours,\nGraphQL.")
}

[lilyball]

FWIW, this escaping won't work with multiline strings either, because \ is not special in a multilline string except as part of the token \""".

[designatednerd]

I've opened #1450 on handling of multiline strings in the Swift codegen.

To be honest, I have a longer-term plan to move the generation of codegen to Swift so it's easier for Swift developers to contribute. Since failure to handle multiline strings isn't a regression, I'd prefer to wait to address this issue until I'm working on the updated codegen, since I have a way better handle on Swift's string handling than Typescript's.

[lilyball]

Actually, come to think of it, the old code probably worked fine after all, because string presumably contains the properly-interpreted contents of the multiline string. In that case, this PR will explicitly break multiline strings.

I would strongly encourage you to add a test case that has a multiline string with backslash-escapes in it (since backslash-escapes, except for \""", aren't meaningful in multiline strings), verify its behavior without this PR, and then see what this PR does to it.

[designatednerd]

I snuck a couple additional tests in on #1452 to validate this. I'll try to get this being less cranky in the morning.

[designatednerd]

@lilyball Would be more than happy to take a look at a PR adding the ability to opt out of this feature, as at the moment I need to focus a bit elsewhere.

Otherwise, any further questions here?

[JacksonKearl]

tiny style note:

a => { return b }

can be rewritten to

a => b

[designatednerd]

👍Yeah Swift (for now) you still always have to write a return statement so that's why I did

[designatednerd]

FWIW I had to leave the return in for trim() because otherwise it returns the result of the trim() command (which is void) rather than the actual trimmed string.

[JacksonKearl]

Oh darn. Always a bummer when things break chaining for no reason.

[designatednerd]

I mean there is a reason, just a bit of an annoying one

[JacksonKearl]

Wait a sec, are we thinking of the same trim? Says here that it returns the trimmed string and doesn't modify the original. Confused. https://developer.mozilla.org/en-US/docs/web/javascript/reference/global_objects/string/trim

[JacksonKearl]

Are you using VSCode? It should be able to tell you about return values of stuff right in the editor. Also we have some scripts to let VSCode run the tests for you locally, so you don't have to wait for Circle to get around to your build.

[designatednerd]

Yeah if I don't use a return, VSCode tells me I can't do a replace on void:

[JacksonKearl]

Oh sorry, you need to remove the {} as well. Just line => line.trim(), not line => { line.trim() }. Its a convenience syntax for when the function consists of returning a single expression.

[designatednerd]

AHA. this worked 😃

[JacksonKearl]

var is so 2000-late, the hip thing nowadays is const or let (which is precisely opposite swift's let). ;)

[designatednerd]

which is precisely opposite swift's let). ;)

This is why I drink.

[JacksonKearl]

Can be rewritten:

this.withinBlock(() => 
   this.printOnNewline( isOptional 
       ? `return graphQLMap["${name}"] as? ${typeName}`
       : `return graphQLMap["${name}"] as! ${typeName}`))

or even

this.withinBlock(() => 
   this.printOnNewline(`return graphQLMap["${name}"] as${isOptional ? '?' : '!'} ${typeName}`))

IMO this makes it a bit more immediately clear what the effect of isOptional is, but this is a style thing that could realistically go either way.

[designatednerd]

This is more in line with how this is done throughout the rest of the file - I like the first option but I'm slightly reluctant to touch the other stuff at the moment if I don't have to.

[designatednerd]

🎉OK - let's leave this open until after #1451 gets merged, I'd love to get that one out in a patch release since it's a bugfix, and this one should really go in a minor release and get alpha'd before we release the hounds on it.

[designatednerd]

OK thanks to @abernix for some help testing this against the iOS project - added a nil-coalesce to .none for double-optionals if there's no value to fix a build issue. This is now ready to go out as a minor release!

[lilyball]

I've finally upgraded Apollo tooling and I'm still not happy with the comment. For example I just opened one of my queries and my entire screen is filled with the comment. It's even worse than I thought because the comment includes all of the __typename fields that aren't present in the original query. It's a 67-line comment and it just makes this file harder to work with.

I'm also surprised to see it's a doc comment, except it doesn't use any markdown formatting, which means it looks truly awful when viewed in Xcode's Quick Help. The only reason to use a doc comment is for Quick Help to show it, so I don't understand why it was done like this.

[designatednerd]

@lilyball The reason it includes all the typename is to make sure the query in the comment is exacty what's being sent to the server - and unfortunately that's a requirement to make sure we're staying type-safe end to end.

Good call on using markdown in the doc comment - I'm a little swamped at the moment after coming back from moving, if you have some time I'd be more than happy to take a look at a PR if you have some time.