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

Support Include/Skip in generated selection set initializers #2869

Merged
merged 7 commits into from
Mar 10, 2023

Conversation

AnthonyMDev
Copy link
Contributor

This required a significant change to the way we hold the data for SelectionSets in memory. During GraphQL Execution, we now create the DataDict for each individual entity. This includes their object type and variables.

Now, the variables on each nested entity are used to evaluate if their fields conditions are fulfilled, rather than looking at the global variables for each operation. This allows us to generate initializers for nested entities that override the inclusion condition values.

The same concept also applies to the object type and the implementedInterfaces that are used to determine type conversions.

This means that you cannot just initialize a SelectionSet with raw JSON anymore. The data needs to be transformed. There is now a SelectionSet.init(data: JSONObject, variables: GraphQLOperation.Variables?) throws function that runs a bare bones version of the GraphQLExecutor applying the needed transformations and validating that the data is correct. This means the function is no longer unsafe, as it throws on invalid data.

@AnthonyMDev AnthonyMDev merged commit f1b0c0b into release/1.1 Mar 10, 2023
@AnthonyMDev AnthonyMDev deleted the include-skip-inits branch March 10, 2023 20:03
@@ -212,7 +212,7 @@ public struct ClassroomPetDetails: AnimalKingdomAPI.SelectionSet, Fragment {
/// Parent Type: `Bird`
public struct AsBird: AnimalKingdomAPI.InlineFragment {
public let __data: DataDict
public init(data: DataDict) { __data = data }
public init(_dataDict: DataDict) { __data = _dataDict }
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this underscored param in a public api supposed to be unnamed, and there’s a missing space? Otherwise seems a strange change. Is it to deter people from using it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is to indicate that it's an internal API that needs to be public only because it's used by generated code. You should not be calling it yourself.

In my next PR, I'll be adding a warning to the documentation of this initializer that it should not be called outside of generated code.

@farshid1
Copy link

farshid1 commented Apr 8, 2023

@AnthonyMDev I'm noticing that even though this change has been made to the ApolloAPI module, ApolloCodegenLib hasn't been updated to reflect this change. The argument name passed to SelectionSet's initializer is still data. So the follwoing error is thrown after running the codegen in my codebase: Initializer 'init(data:)' has different argument labels from those required by protocol 'SelectionSet' ('init(_dataDict:)')

@calvincestari
Copy link
Member

@farshid1, I can't seem to find reference to initializers in ApolloCodegenLib that still use the data argument.

Some things that may help track down the issue:

  • If you're using the CLI for code generation please make sure you rebuild it after updating your Apollo iOS dependency; there is a version checker in there which should prevent a version mismatch anyways.
  • Make sure that any automation you have for regenerating the model is actually doing so.

@farshid1
Copy link

You are absolutely correct. Looks like I had installed the package through Xcode once and another time when I migrated my app to use Package.swift. Removed both packages and reinstalled through Package.swift and things are working as they should. Appreciate the pointer @calvincestari!

@calvincestari
Copy link
Member

Glad it's working now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants