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.

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

Merged
merged 2 commits into from Apr 26, 2023

Conversation

BobaFetters
Copy link
Member

-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 selection generation for selection sets
-Added isRootFieldType calculation for GraphQLCompositeType on operations and fragments to the init of the CompilationResult object

-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
@netlify
Copy link

netlify bot commented Apr 26, 2023

Deploy Preview for apollo-ios-docs canceled.

Name Link
🔨 Latest commit 0eaede2
🔍 Latest deploy log https://app.netlify.com/sites/apollo-ios-docs/deploys/644988eb8d1d5d00080a25bf

@@ -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()
Copy link
Member Author

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.

Copy link
Contributor

@AnthonyMDev AnthonyMDev left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

Copy link
Member

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)
Copy link
Contributor

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.

@BobaFetters BobaFetters marked this pull request as draft April 26, 2023 17:13
@BobaFetters
Copy link
Member Author

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
Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

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

👨‍🍳 🤌💋

@BobaFetters BobaFetters marked this pull request as ready for review April 26, 2023 20:46
@BobaFetters BobaFetters merged commit ffc95cb into main Apr 26, 2023
15 of 16 checks passed
@BobaFetters BobaFetters deleted the fix/custom-root-type-generation branch April 26, 2023 20:46
Copy link
Member

@calvincestari calvincestari left a 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/
Copy link
Member

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.", { });
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

Nicely done!

@calvincestari calvincestari mentioned this pull request May 1, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants