-
Notifications
You must be signed in to change notification settings - Fork 55
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
[1/x] Refactor root field builder for deferred fragments #74
[1/x] Refactor root field builder for deferred fragments #74
Conversation
@@ -331,7 +331,7 @@ class SelectionSetTemplateTests: XCTestCase { | |||
|
|||
let expected = """ | |||
public static var __parentType: ApolloAPI.ParentType { TestSchema.Objects.Nested } | |||
|
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 know why the diff for this file keeps looking so messed up. All I did here was to add throw XCTSkip()
so that the PR can pass CI. The skips will be removed in the third PR which actually updates the selection set template.
5fd6bde
to
149ffe6
Compare
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.
LGTM!
apollo-ios-codegen/Sources/GraphQLCompiler/CompilationResult.swift
Outdated
Show resolved
Hide resolved
apollo-ios-codegen/Sources/GraphQLCompiler/CompilationResult.swift
Outdated
Show resolved
Hide resolved
Tests/ApolloCodegenTests/CodeGenIR/IRRootFieldBuilderTests.swift
Outdated
Show resolved
Hide resolved
parentType: Object_Cat, | ||
directSelections: [ | ||
.field("species", type: .string()), | ||
] |
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 think we need to add some tests here for deeply other merging behaviours. For example:
Deferred Fragment on same type
I don't see any tests for when the deferred fragment is on the same type.
query TestOperation {
allAnimals {
__typename
id
... on Animal @defer(label: "root") {
species
}
}
}
In this case, we the deferred fragment is on Animal. We are only having to write ... on Animal
because you can't do ... @defer(label)
on its own. (I don't think we can? If that's allowed, this is still a situation we need to handle, we would actually want a test for both of these syntaxes then.)
Here, we aren't creating an AsAnimal
, the deferred fragment could just live directly in the AllAnimal
object.
Merging across matching types
query TestOperation {
allAnimals {
__typename
id
... on Dog @defer(label: "root") {
species
}
... on Dog {
humanName
}
}
}
Ensure that species
is not merged into the non deferred inline fragment, but the deferred fragment is merged inside of the AsDog
.
query TestOperation {
allAnimals {
__typename
id
... on Dog @defer(label: "root") {
species
}
... on Pet {
humanName
}
}
}
Ensure that the AsDog is still created with the merged field humanName
and the deferred fragment inside it.
Merging Deferred Fragments
I don't see any test for behavior where the merged fragment is a merged selection, only direct. Consider this:
query TestOperation {
allAnimals {
__typename
id
... on Pet @defer(label: "root") {
humanName
}
... on Dog {
species
}
}
}
Because Dog
implements the Pet
interface, the AsPet
should have a direct selection of it's deferred Root
fragment. But the AsDog
should also have this deferred fragment, as purely a pointer to the deferred Root
fragment in the AsPet
You would want:
expect(allAnimals_AsPet).to(shallowlyMatch(
SelectionSetMatcher(
parentType: Interface_Pet,
directSelections: [
.deferred(Interface_Pet, label: "root"),
],
mergedSelections: [
],
mergedSources: [
]
)
))
expect(allAnimals_AsDog).to(shallowlyMatch(
SelectionSetMatcher(
parentType: Object_Dog,
directSelections: [
.field("species", type: .nonNull(.scalar(Scalar_String))),
],
mergedSelections: [
.deferred(Interface_Pet, label: "root"),
],
mergedSources: [
try .mock(allAnimals_AsPet),
]
)
))
expect(allAnimals_AsPet_Deferred_AsRoot).to(shallowlyMatch(
SelectionSetMatcher(
parentType: Interface_Pet,
directSelections: [
.field("humanName", type: .nonNull(.scalar(Scalar_String))),
],
mergedSelections: [
],
mergedSources: [
]
)
))
Because deferred fragments are isolated, you can actually just always reference the type of the generated fragment from the merged source, so the (psuedo) generated code for the AsDog
looks something like:
struct AsDog {
...
struct Fragments {
@Deferred var root: AsPet.Root
}
}
There are probably a number of other test cases similar to this. A couple I know we'd need are:
- If the type doesn't match the interface, don't merge.
- Replacing interface with a union
Merging across deeply nested siblings (cousins?)
The same concept for merging these applies across deeply nested sibling merging. Rather than looking at the type of the direct sibling for a match, you're actually looking for the same field in a matching selection set.
query TestOperation {
allAnimals {
__typename
id
... on Dog {
bestFriend {
humanName
}
species
}
... on Pet {
bestFriend {
... on Animal @defer(label: "root") {
species
}
}
}
}
}
Here, we would want the deferred Root
fragment to be a direct selection of AllAnimal.AsPet.BestFriend
, but the AllAnimal.AsDog.BestFriend
should still also have a merged selection of the deferred Root
fragment. (Again pointing to the merged fragment from the AsPet.BestFriend
)
Same as before we need tests for this with unions as well. We may even want to test with a 3rd level of nesting to ensure the algorithm is adequately complex for all cases.
There are probably other things we should be testing too that don't come to my mind immediately, but that's why we will be shipping this in preview state and getting users to submit reports for other edge cases we've missed!
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.
And everything I've outlined here for inline fragments is likely translatable to NamedFragments as well, so we'll need to ensure the same behavior works properly there!
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.
Additionally, I don't see any tests for NamedFragments
with deferred fields inside of them. If we aren't deferring the named fragment itself, then it's fields will be merged in. So we need to ensure that we don't merge in the named fragments internal deferred fragments!
Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift
Show resolved
Hide resolved
Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift
Show resolved
Hide resolved
Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift
Show resolved
Hide resolved
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 is looking really good! The implementation of it seems to work as intended and I'm happy with the naming changes and clean up.
I just see a lot of missing test cases still. It's crazy how many complex edge cases we have to account for with this stuff.
(I'm now realizing we haven't even gotten into tests @defer
with @include/@skip
!)
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 is looking really good! The implementation of it seems to work as intended and I'm happy with the naming changes and clean up.
I just see a lot of missing test cases still. It's crazy how many complex edge cases we have to account for with this stuff.
(I'm now realizing we haven't even gotten into tests @defer
with @include/@skip
!)
@AnthonyMDev I believe this PR is ready for another review, it's now got:
What it does not have though is additional logic and tests for merging deferred fragments and merging across deeply nested siblings; I've broken that requirement out into apollographql/apollo-ios#3261 since it's convenience rather than a requirement for a preview release. Thanks for the thorough review earlier, I think we've got much better code here as a result. 🎉 |
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 is awesome! Great job @calvincestari! I know it was a difficult journey to get here. Excited to move forward!
.field("species", type: .string()), | ||
], | ||
mergedSelections: [ | ||
.field("id", type: .string()), |
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 think eventually, this should also include the name
field from the AsPet
selection set, but I recall us deciding that there we're some technical challenges to making that happen right now and we were going to come back and revisit that in the future, is that right?
Do we have an issue to refer to for fixing that later? We should put a comment in these tests that refer to that issue for the future fixes.
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.
Correct, I think that will end up getting done in apollographql/apollo-ios#3261.
@@ -164,7 +160,7 @@ public class CompilationResult: JavaScriptObject { | |||
|
|||
lazy var directives: [Directive]? = self["directives"] | |||
|
|||
public lazy var isDeferred: IsDeferred = getIsDeferred() | |||
public lazy var deferCondition: DeferCondition? = getDeferCondition() |
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.
When I merge my changes for 1.7.0 into main
this will need to change a little. For concurrency, we need to make all the JavaScript types immutable, so no lazy
vars. I'll make the change to update this to calculate and store the value during initialization.
So this is fine for this PR.
f499efa4 chore: Disable `@defer` support (#122) 70fd68cd Merge branch 'main' into feature/defer d6c49617 Defer Selection Set Template - Basic (W/clean up) (#118) f5dc1758 merge: `feature/defer` update to `main` (#116) 2c5463b0 refactor: [1/x] Refactor root field builder for deferred fragments (#74) 25d44b83 Merge branch 'main' into feature/defer 1d8424d3 Merge branch 'main' into feature/defer a8c31ca3 Merge branch 'main' into feature/defer d1a3c0ab feature: Operation definition template supports the `@defer` directive (#31) 6a01891b Merge branch 'project-breakup' into feature/defer 11cf5828 Merge branch 'project-breakup' into feature/defer 723b8648 refactor: Don't require `label` argument for deferred named fragments (#33) 9041eee0 feature: Require `label` argument on `@defer` directive (#28) 01b81541 Migrating defer branch to new project structure git-subtree-dir: apollo-ios-codegen git-subtree-split: f499efa4ee99c7eb86e5f2e6d0b630756c48f0de
This PR for apollographql/apollo-ios#3141 refactors the root field builder to build deferred inline and nested fragments into the IR.
IsDeferred
enum into a struct namedDeferCondition
that is an Optional when used; this allows us to usenil
to signal "no defer condition" andsome
to signal "a defer condition" exists. It now also holds the label.ScopeDescriptor
toScopeCondition
which allows us to fully differentiate it in the scope path.