-
Notifications
You must be signed in to change notification settings - Fork 738
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.
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
SelectionSet Codegen __typename fix - Closes #2955 #2983
Conversation
-Adds data to CompilationResult from JS Frontend that includes the defined root types for query, mutation, and subscription -Uses the new root type data to set the `isRootFieldType` field on `GraphQLCompositeType` which is used when determining __typename field generation for selection sets
✅ Deploy Preview for apollo-ios-docs canceled.
|
@@ -13,6 +15,31 @@ public class CompilationResult: JavaScriptObject { | |||
|
|||
lazy var schemaDocumentation: String? = self["schemaDocumentation"] | |||
|
|||
required init(_ jsValue: JSValue, bridge: JavaScriptBridge) { | |||
super.init(jsValue, bridge: bridge) | |||
processRootTypes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking through the code, and where/how the isRootFieldType
was originally used, without piping the new root type data through the pipeline, or updating the GraphQLCompositeType
objects later in the pipeline, this seems like a good way to ensure all of the GraphQLCompositeType
objects have their isRootFieldType
updated before use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks perfect! Thanks so much @BobaFetters!
output_file="$SCRIPT_DIR/ApolloCodegenFrontendBundle.swift" | ||
$( cd "$SCRIPT_DIR/Javascript" && rollup -c ) | ||
minJS=$(cat "$SCRIPT_DIR/dist/ApolloCodegenFrontend.bundle.js") | ||
printf "%s%s%s" "let ApolloCodegenFrontendBundle: String = #\"" "$minJS" "\"#" > $output_file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done!
""" | ||
|
||
// when | ||
try buildSubjectAndOperation(cocoapodsImportStatements: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be needing the cocoapodsImportStatements: true
. I assume you copied this from another test that happened to be testing part of the codegen template that differs for Cocoapods. There is no harm in this param being set, but I'd remove it, just to keep the scope of the test limited to only what its really testing.
Switched back to draft to fix some tests |
-Updated `RootTypeDefinition` so that `queryType` is required -Moved `processRootTypes` functionality out of `CompilationResult` and into `IR` class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👨🍳 🤌💋
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the awesome work @BobaFetters!
@@ -70,3 +70,6 @@ SwiftScripts/.build-** | |||
# Local generated packages we don't need in the main project | |||
Sources/AnimalKingdomAPI/Generated/Package.swift | |||
Sources/AnimalKingdomAPI/Generated/Package.resolved | |||
|
|||
# Generated js files we don't need committed | |||
Sources/ApolloCodegenLib/Frontend/dist/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
@@ -71,6 +72,17 @@ export function compileToIR( | |||
const fragmentMap = new Map<String, ir.FragmentDefinition>(); | |||
const referencedTypes = new Set<GraphQLNamedType>(); | |||
|
|||
const queryType = schema.getQueryType() as GraphQLNamedType; | |||
if (queryType === undefined) { | |||
throw new GraphQLError("GraphQL Schema must contain a 'query' root type definition.", { }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this we'd ever reach this stage. Since this is a spec violation it'll likely get caught by graphql-js validation before we start compiling the IR.
output_file="$SCRIPT_DIR/ApolloCodegenFrontendBundle.swift" | ||
$( cd "$SCRIPT_DIR/Javascript" && rollup -c ) | ||
minJS=$(cat "$SCRIPT_DIR/dist/ApolloCodegenFrontend.bundle.js") | ||
printf "%s%s%s" "let ApolloCodegenFrontendBundle: String = #\"" "$minJS" "\"#" > $output_file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done!
-Adds data to CompilationResult from JS Frontend that includes the defined root types for query, mutation, and subscription -Uses the new root type data to set the
isRootFieldType
field onGraphQLCompositeType
which is used when determining __typename selection generation for selection sets-Added
isRootFieldType
calculation forGraphQLCompositeType
on operations and fragments to theinit
of theCompilationResult
object