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.

Sign up for GitHub

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

Jump to bottom

Swift Codegen Upgrades #1441

Merged
merged 19 commits into from
Aug 5, 2019
Merged

Swift Codegen Upgrades #1441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
43269cb
Add `CaseIterable` conformance to swift Enums
designatednerd Jul 26, 2019
b66e256
Don't use `as!` when the thing being unwrapped is a double-optional
designatednerd Jul 26, 2019
0839e46
Use multi-line string literals for operations and fragments
designatednerd Jul 26, 2019
27215b0
update changelog
designatednerd Jul 26, 2019
2c210f5
fix swift tests failing on CI
designatednerd Jul 26, 2019
d0fb42b
back out multiline codeblock changes
designatednerd Jul 29, 2019
2ada433
add comments with original graphql statement to operation definitions
designatednerd Jul 29, 2019
c4986fe
add comments with graphql statements to fragment definitions
designatednerd Jul 29, 2019
b4def67
update operation definition in the general tests
designatednerd Jul 29, 2019
db9112d
trim out additional whitespace
designatednerd Jul 29, 2019
72c8e6c
update changelog
designatednerd Jul 29, 2019
3f4a6a0
don't add newlines back in when trimming whitespace
designatednerd Jul 30, 2019
216fe2e
add comments to added test cases
designatednerd Jul 31, 2019
58cf555
do not attempt to strip whitespace from things which include multi-li…
designatednerd Jul 31, 2019
17de607
fix alignment sillies
designatednerd Jul 31, 2019
4f01da2
slightly javascriptier code
designatednerd Aug 1, 2019
e26e9c4
yep, guess that needs the return after all too
designatednerd Aug 1, 2019
724e81e
remove the brackets and you can also remove the return!
designatednerd Aug 2, 2019
f07badd
nil-coalesce to `none` when pulling optional out of GraphQL map
designatednerd Aug 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@
- `apollo-codegen-swift`
- Fix issue where type names were not being properly escaped [iOS 193](https://github.com/apollographql/apollo-ios/issues/193)
- Fix overcorrection on removing redundant modifiers [#1449](https://github.com/apollographql/apollo-tooling/issues/1449)
- Added `CaseIterable` conformance so all known cases can be easily iterated.
- Added comment to `operationDefinition` to show the original query
- Stripped excess whitespace out of `operationDefinition`
- Removed force-unwrap when the thing being unwrapped is a double optional
- `apollo-codegen-typescript`
- <First `apollo-codegen-typescript` related entry goes here>
- `apollo-env`
Expand Down
122 changes: 104 additions & 18 deletions packages/apollo-codegen-swift/src/__tests__/__snapshots__/codeGeneration.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,18 @@

exports[`Swift code generation #classDeclarationForOperation() should correctly escape a mutli-line string literal 1`] = `
"public final class CreateReviewMutation: GraphQLMutation {
/// mutation CreateReview($episode: Episode) {
/// createReview(episode: $episode, review: {stars: 5, commentary: \\"\\"\\"
/// Wow!
/// I thought
/// This movie ROCKED!
/// \\"\\"\\"}) {
/// stars
/// commentary
/// }
/// }
public let operationDefinition =
\\"mutation CreateReview($episode: Episode) {\\\\n createReview(episode: $episode, review: {stars: 5, commentary: \\\\\\"\\\\\\"\\\\\\"\\\\n Wow!\\\\n \\\\n This movie ROCKED!\\\\n \\\\\\"\\\\\\"\\\\\\"}) {\\\\n stars\\\\n commentary\\\\n }\\\\n}\\"
\\"mutation CreateReview($episode: Episode) {\\\\n createReview(episode: $episode, review: {stars: 5, commentary: \\\\\\"\\\\\\"\\\\\\"\\\\n Wow!\\\\n I thought\\\\n This movie ROCKED!\\\\n \\\\\\"\\\\\\"\\\\\\"}) {\\\\n stars\\\\n commentary\\\\n }\\\\n}\\"

public let operationName = \\"CreateReview\\"

Expand All @@ -21,7 +31,7 @@ exports[`Swift code generation #classDeclarationForOperation() should correctly
public static let possibleTypes = [\\"Mutation\\"]

public static let selections: [GraphQLSelection] = [
GraphQLField(\\"createReview\\", arguments: [\\"episode\\": GraphQLVariable(\\"episode\\"), \\"review\\": [\\"stars\\": 5, \\"commentary\\": \\"Wow!\\\\n\\\\n This movie ROCKED!\\"]], type: .object(CreateReview.selections)),
GraphQLField(\\"createReview\\", arguments: [\\"episode\\": GraphQLVariable(\\"episode\\"), \\"review\\": [\\"stars\\": 5, \\"commentary\\": \\"Wow!\\\\n I thought\\\\n This movie ROCKED!\\"]], type: .object(CreateReview.selections)),
]

public private(set) var resultMap: ResultMap
Expand Down Expand Up @@ -87,8 +97,18 @@ exports[`Swift code generation #classDeclarationForOperation() should correctly

exports[`Swift code generation #classDeclarationForOperation() should correctly escape a mutli-line string literal with backslashes 1`] = `
"public final class CreateReviewMutation: GraphQLMutation {
/// mutation CreateReview($episode: Episode) {
/// createReview(episode: $episode, review: {stars: 5, commentary: \\"\\"\\"
/// Wow!
/// I thought
/// This movie \\\\ ROCKED!
/// \\"\\"\\"}) {
/// stars
/// commentary
/// }
/// }
public let operationDefinition =
\\"mutation CreateReview($episode: Episode) {\\\\n createReview(episode: $episode, review: {stars: 5, commentary: \\\\\\"\\\\\\"\\\\\\"\\\\n Wow!\\\\n \\\\n This movie \\\\ ROCKED!\\\\n \\\\\\"\\\\\\"\\\\\\"}) {\\\\n stars\\\\n commentary\\\\n }\\\\n}\\"
\\"mutation CreateReview($episode: Episode) {\\\\n createReview(episode: $episode, review: {stars: 5, commentary: \\\\\\"\\\\\\"\\\\\\"\\\\n Wow!\\\\n I thought\\\\n This movie \\\\ ROCKED!\\\\n \\\\\\"\\\\\\"\\\\\\"}) {\\\\n stars\\\\n commentary\\\\n }\\\\n}\\"

public let operationName = \\"CreateReview\\"

Expand All @@ -106,7 +126,7 @@ exports[`Swift code generation #classDeclarationForOperation() should correctly
public static let possibleTypes = [\\"Mutation\\"]

public static let selections: [GraphQLSelection] = [
GraphQLField(\\"createReview\\", arguments: [\\"episode\\": GraphQLVariable(\\"episode\\"), \\"review\\": [\\"stars\\": 5, \\"commentary\\": \\"Wow!\\\\n\\\\n This movie \\\\\\\\ ROCKED!\\"]], type: .object(CreateReview.selections)),
GraphQLField(\\"createReview\\", arguments: [\\"episode\\": GraphQLVariable(\\"episode\\"), \\"review\\": [\\"stars\\": 5, \\"commentary\\": \\"Wow!\\\\n I thought\\\\n This movie \\\\\\\\ ROCKED!\\"]], type: .object(CreateReview.selections)),
]

public private(set) var resultMap: ResultMap
Expand Down Expand Up @@ -172,8 +192,14 @@ exports[`Swift code generation #classDeclarationForOperation() should correctly

exports[`Swift code generation #classDeclarationForOperation() should generate a class declaration for a mutation with variables 1`] = `
"public final class CreateReviewMutation: GraphQLMutation {
/// mutation CreateReview($episode: Episode) {
/// createReview(episode: $episode, review: {stars: 5, commentary: \\"Wow!\\"}) {
/// stars
/// commentary
/// }
/// }
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

public let operationDefinition =
\\"mutation CreateReview($episode: Episode) {\\\\n createReview(episode: $episode, review: {stars: 5, commentary: \\\\\\"Wow!\\\\\\"}) {\\\\n stars\\\\n commentary\\\\n }\\\\n}\\"
\\"mutation CreateReview($episode: Episode) { createReview(episode: $episode, review: {stars: 5, commentary: \\\\\\"Wow!\\\\\\"}) { stars commentary } }\\"

public let operationName = \\"CreateReview\\"

Expand Down Expand Up @@ -257,8 +283,15 @@ exports[`Swift code generation #classDeclarationForOperation() should generate a

exports[`Swift code generation #classDeclarationForOperation() should generate a class declaration for a query with a fragment spread nested in an inline fragment 1`] = `
"public final class HeroQuery: GraphQLQuery {
/// query Hero {
/// hero {
/// ... on Droid {
/// ...HeroDetails
/// }
/// }
/// }
public let operationDefinition =
\\"query Hero {\\\\n hero {\\\\n ... on Droid {\\\\n ...HeroDetails\\\\n }\\\\n }\\\\n}\\"
\\"query Hero { hero { ... on Droid { ...HeroDetails } } }\\"

public let operationName = \\"Hero\\"

Expand Down Expand Up @@ -389,8 +422,13 @@ exports[`Swift code generation #classDeclarationForOperation() should generate a

exports[`Swift code generation #classDeclarationForOperation() should generate a class declaration for a query with conditional fragment spreads 1`] = `
"public final class HeroQuery: GraphQLQuery {
/// query Hero {
/// hero {
/// ...DroidDetails
/// }
/// }
public let operationDefinition =
\\"query Hero {\\\\n hero {\\\\n ...DroidDetails\\\\n }\\\\n}\\"
\\"query Hero { hero { ...DroidDetails } }\\"

public let operationName = \\"Hero\\"

Expand Down Expand Up @@ -549,8 +587,13 @@ exports[`Swift code generation #classDeclarationForOperation() should generate a

exports[`Swift code generation #classDeclarationForOperation() should generate a class declaration for a query with fragment spreads 1`] = `
"public final class HeroQuery: GraphQLQuery {
/// query Hero {
/// hero {
/// ...HeroDetails
/// }
/// }
public let operationDefinition =
\\"query Hero {\\\\n hero {\\\\n ...HeroDetails\\\\n }\\\\n}\\"
\\"query Hero { hero { ...HeroDetails } }\\"

public let operationName = \\"Hero\\"

Expand Down Expand Up @@ -648,8 +691,13 @@ exports[`Swift code generation #classDeclarationForOperation() should generate a

exports[`Swift code generation #classDeclarationForOperation() should generate a class declaration for a query with variables 1`] = `
"public final class HeroNameQuery: GraphQLQuery {
/// query HeroName($episode: Episode) {
/// hero(episode: $episode) {
/// name
/// }
/// }
public let operationDefinition =
\\"query HeroName($episode: Episode) {\\\\n hero(episode: $episode) {\\\\n name\\\\n }\\\\n}\\"
\\"query HeroName($episode: Episode) { hero(episode: $episode) { name } }\\"

public let operationName = \\"HeroName\\"

Expand Down Expand Up @@ -726,8 +774,13 @@ exports[`Swift code generation #classDeclarationForOperation() should generate a

exports[`Swift code generation #classDeclarationForOperation() should generate a class declaration with an operationIdentifier property when generateOperationIds is specified 1`] = `
"public final class HeroQuery: GraphQLQuery {
/// query Hero {
/// hero {
/// ...HeroDetails
/// }
/// }
public let operationDefinition =
\\"query Hero {\\\\n hero {\\\\n ...HeroDetails\\\\n }\\\\n}\\"
\\"query Hero { hero { ...HeroDetails } }\\"

public let operationName = \\"Hero\\"

Expand Down Expand Up @@ -846,8 +899,12 @@ exports[`Swift code generation #initializerDeclarationForProperties() should gen

exports[`Swift code generation #structDeclarationForFragment() should generate a struct declaration for a fragment that includes a fragment spread 1`] = `
"public struct HeroDetails: GraphQLFragment {
/// fragment HeroDetails on Character {
/// name
/// ...MoreHeroDetails
/// }
public static let fragmentDefinition =
\\"fragment HeroDetails on Character {\\\\n name\\\\n ...MoreHeroDetails\\\\n}\\"
\\"fragment HeroDetails on Character { name ...MoreHeroDetails }\\"

public static let possibleTypes = [\\"Human\\", \\"Droid\\"]

Expand Down Expand Up @@ -920,8 +977,12 @@ exports[`Swift code generation #structDeclarationForFragment() should generate a

exports[`Swift code generation #structDeclarationForFragment() should generate a struct declaration for a fragment with a concrete type condition 1`] = `
"public struct DroidDetails: GraphQLFragment {
/// fragment DroidDetails on Droid {
/// name
/// primaryFunction
/// }
public static let fragmentDefinition =
\\"fragment DroidDetails on Droid {\\\\n name\\\\n primaryFunction\\\\n}\\"
\\"fragment DroidDetails on Droid { name primaryFunction }\\"

public static let possibleTypes = [\\"Droid\\"]

Expand Down Expand Up @@ -964,8 +1025,14 @@ exports[`Swift code generation #structDeclarationForFragment() should generate a

exports[`Swift code generation #structDeclarationForFragment() should generate a struct declaration for a fragment with a subselection 1`] = `
"public struct HeroDetails: GraphQLFragment {
/// fragment HeroDetails on Character {
/// name
/// friends {
/// name
/// }
/// }
public static let fragmentDefinition =
\\"fragment HeroDetails on Character {\\\\n name\\\\n friends {\\\\n name\\\\n }\\\\n}\\"
\\"fragment HeroDetails on Character { name friends { name } }\\"

public static let possibleTypes = [\\"Human\\", \\"Droid\\"]

Expand Down Expand Up @@ -1044,8 +1111,12 @@ exports[`Swift code generation #structDeclarationForFragment() should generate a

exports[`Swift code generation #structDeclarationForFragment() should generate a struct declaration for a fragment with an abstract type condition 1`] = `
"public struct HeroDetails: GraphQLFragment {
/// fragment HeroDetails on Character {
/// name
/// appearsIn
/// }
public static let fragmentDefinition =
\\"fragment HeroDetails on Character {\\\\n name\\\\n appearsIn\\\\n}\\"
\\"fragment HeroDetails on Character { name appearsIn }\\"

public static let possibleTypes = [\\"Human\\", \\"Droid\\"]

Expand Down Expand Up @@ -1649,7 +1720,7 @@ exports[`Swift code generation #structDeclarationForSelectionSet() should genera
`;

exports[`Swift code generation #typeDeclarationForGraphQLType() should escape identifiers in cases of enum declaration for a GraphQLEnumType 1`] = `
"public enum AlbumPrivacies: RawRepresentable, Equatable, Hashable, Apollo.JSONDecodable, Apollo.JSONEncodable {
"public enum AlbumPrivacies: RawRepresentable, Equatable, Hashable, CaseIterable, Apollo.JSONDecodable, Apollo.JSONEncodable {
public typealias RawValue = String
case \`public\`
case \`private\`
Expand Down Expand Up @@ -1680,6 +1751,13 @@ exports[`Swift code generation #typeDeclarationForGraphQLType() should escape id
default: return false
}
}

public static var allCases: [AlbumPrivacies] {
return [
.public,
.private,
]
}
}"
`;

Expand All @@ -1705,7 +1783,7 @@ public struct ReviewInput: GraphQLMapConvertible {
/// Comment about the movie, optional
public var commentary: Swift.Optional<String?> {
get {
return graphQLMap[\\"commentary\\"] as! Swift.Optional<String?>
return graphQLMap[\\"commentary\\"] as? Swift.Optional<String?> ?? .none
}
set {
graphQLMap.updateValue(newValue, forKey: \\"commentary\\")
Expand All @@ -1715,7 +1793,7 @@ public struct ReviewInput: GraphQLMapConvertible {
/// Favorite color, optional
public var favoriteColor: Swift.Optional<ColorInput?> {
get {
return graphQLMap[\\"favorite_color\\"] as! Swift.Optional<ColorInput?>
return graphQLMap[\\"favorite_color\\"] as? Swift.Optional<ColorInput?> ?? .none
}
set {
graphQLMap.updateValue(newValue, forKey: \\"favorite_color\\")
Expand All @@ -1726,7 +1804,7 @@ public struct ReviewInput: GraphQLMapConvertible {

exports[`Swift code generation #typeDeclarationForGraphQLType() should generate an enum declaration for a GraphQLEnumType 1`] = `
"/// The episodes in the Star Wars trilogy
public enum Episode: RawRepresentable, Equatable, Hashable, Apollo.JSONDecodable, Apollo.JSONEncodable {
public enum Episode: RawRepresentable, Equatable, Hashable, CaseIterable, Apollo.JSONDecodable, Apollo.JSONEncodable {
public typealias RawValue = String
/// Star Wars Episode IV: A New Hope, released in 1977.
case newhope
Expand Down Expand Up @@ -1764,5 +1842,13 @@ public enum Episode: RawRepresentable, Equatable, Hashable, Apollo.JSONDecodable
default: return false
}
}

public static var allCases: [Episode] {
return [
.newhope,
.empire,
.jedi,
]
}
}"
`;
4 changes: 2 additions & 2 deletions packages/apollo-codegen-swift/src/__tests__/codeGeneration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ describe("Swift code generation", () => {
createReview(episode: $episode, review: {stars: 5, commentary:
"""
Wow!

I thought
This movie ROCKED!
"""
}) {
Expand All @@ -117,7 +117,7 @@ describe("Swift code generation", () => {
createReview(episode: $episode, review: {stars: 5, commentary:
"""
Wow!

I thought
This movie \\ ROCKED!
"""
}) {
Expand Down
34 changes: 29 additions & 5 deletions packages/apollo-codegen-swift/src/codeGeneration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ export class SwiftAPIGenerator extends SwiftGenerator<CompilerContext> {
},
() => {
if (source) {
this.commentWithoutTrimming(source);
this.printOnNewline("public let operationDefinition =");
this.withIndent(() => {
this.multilineString(source);
Expand Down Expand Up @@ -315,6 +316,7 @@ export class SwiftAPIGenerator extends SwiftGenerator<CompilerContext> {
outputIndividualFiles,
() => {
if (source) {
this.commentWithoutTrimming(source);
this.printOnNewline("public static let fragmentDefinition =");
this.withIndent(() => {
this.multilineString(source);
Expand Down Expand Up @@ -977,7 +979,7 @@ export class SwiftAPIGenerator extends SwiftGenerator<CompilerContext> {
this.printNewlineIfNeeded();
this.comment(description || undefined);
this.printOnNewline(
`public enum ${name}: RawRepresentable, Equatable, Hashable, Apollo.JSONDecodable, Apollo.JSONEncodable`
`public enum ${name}: RawRepresentable, Equatable, Hashable, CaseIterable, Apollo.JSONDecodable, Apollo.JSONEncodable`
);
this.withinBlock(() => {
this.printOnNewline("public typealias RawValue = String");
Expand Down Expand Up @@ -1049,6 +1051,21 @@ export class SwiftAPIGenerator extends SwiftGenerator<CompilerContext> {
this.printOnNewline(`default: return false`);
});
});

this.printNewlineIfNeeded();
this.printOnNewline(`public static var allCases: [${name}]`);
this.withinBlock(() => {
this.printOnNewline(`return [`);
values.forEach(value => {
const enumDotCaseName = escapeIdentifierIfNeeded(
this.helpers.enumDotCaseName(value.name)
);
this.withIndent(() => {
this.printOnNewline(`${enumDotCaseName},`);
});
});
this.printOnNewline(`]`);
});
});
}

Expand Down Expand Up @@ -1120,7 +1137,8 @@ export class SwiftAPIGenerator extends SwiftGenerator<CompilerContext> {
name,
propertyName,
typeName,
description
description,
isOptional
} of properties) {
this.printNewlineIfNeeded();
this.comment(description || undefined);
Expand All @@ -1130,9 +1148,15 @@ export class SwiftAPIGenerator extends SwiftGenerator<CompilerContext> {
this.withinBlock(() => {
this.printOnNewline("get");
this.withinBlock(() => {
this.printOnNewline(
`return graphQLMap["${name}"] as! ${typeName}`
);
if (isOptional) {
this.printOnNewline(
`return graphQLMap["${name}"] as? ${typeName} ?? .none`
);
} else {
this.printOnNewline(
`return graphQLMap["${name}"] as! ${typeName}`
);
}
});
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

this.printOnNewline("set");
this.withinBlock(() => {
Expand Down
Loading