refactor: Selection set generation for `@defer` #3176

calvincestari · 2023-08-05T19:12:58Z

Introduces IsDeferred expression logic which is used in the selection set template to:

Add Operation-level hasDeferredFragments property
Add deferred property to inline and named fragment selections
Mark deferred fragment accessors as Optional

If there are other tests you think would be beneficial, I'm sure there are, please let me know and get them added.

calvincestari · 2023-08-05T19:14:56Z

Sources/ApolloCodegenLib/Frontend/CompilationResult.swift

@@ -173,6 +181,27 @@ public class CompilationResult: JavaScriptObject {

    lazy var directives: [Directive]? = self["directives"]

+    lazy var isDeferred: IsDeferred = {


This is duplicated for both InlineFragment and FragmentSpread, I'm not sure if there is a better way to do it; they're both just simple JavaScriptObject types.

protocol Deferrable { var directives: [Directive]? { get } } fileprivate extension Deferrable where Self: JavaScriptObject { func getIsDeferred() -> IsDeferred { ... } } public class InlineFragment: Deferrable { ... lazy var isDeferred: IsDeferred = getIsDeferred() }

Wish we could easily namespace this Deferrable protocol in CompilationResult. Until we break CompilationResult into it's own package, we might want to call the protocol CompilationResult_Deferrable

The protocol and extension can have the fileprivate scope which is effectively the same as putting it into it's own package for this particular case.

Sources/ApolloCodegenLib/Frontend/CompilationResult.swift

calvincestari · 2023-08-05T19:19:39Z

Sources/ApolloCodegenLib/IR/IR+InclusionConditions.swift

@@ -54,6 +54,21 @@ extension IR {
      self.conditions = [condition]
    }

+    /// A failable initializer to convert from an array of `CompilationResult.InclusionCondition`.
+    init?(_ conditions: [CompilationResult.InclusionCondition]?) {


@AnthonyMDev - I've added this since we determined that ScopeCondition should match the inclusion conditions as well as the parent type. Problem is that the named fragment passed into the scope condition is from CompilationResult and the two InclusionCondition types can't be compared. This is a simple conversion similar to how we treat conversions for the IR.InclusionCondition.Result type.

I wonder if we could instead have a custom == operator that will compare CompilationResult.InclusionConditions to IR.InclusionConditions. That might be a better solution than an initializer that should never be used?

The problem with including this initializer is that it doesn't have an accurate 1:1 way to map. For @skip with no variable, we would be omitting the entire fragment. The preconditionFailure here seems way too easy to accidentally make occur and cause a crash.

I think that a custom matching function that is fileprivate to the file that is using it is a better approach. It's not an actual ==, so I would make it a function with a more descriptive name.

calvincestari · 2023-08-05T19:22:30Z

Sources/ApolloCodegenLib/IR/ScopeDescriptor.swift

@@ -3,9 +3,6 @@ import OrderedCollections

 extension IR {

-  // TODO: Write tests that two inline fragments with same type and inclusion conditions,


Done in https://github.com/apollographql/apollo-ios/pull/3176/files#diff-75a0a416f524d6139314ef8b30651767a446ba7f6a1dbc0e00a4c64f130251d1R2232.

I'm thinking we should also be adding tests for this behavior to the IR building tests, not just the SelectionSetTemplateTests. Making sure the actual IR is structured properly is important and having those tests makes it a LOT easier to debug and figure out the cause of bugs than just the template tests.

calvincestari · 2023-08-08T01:27:40Z

Sources/ApolloCodegenLib/Templates/RenderingHelpers/OperationTemplateRenderer.swift

+  func DeferredProperties(
+    _ definition: IR.Definition
+  ) -> TemplateString {
+    func isDeferred(_ selectionSet: IR.SelectionSet) -> Bool {


@AnthonyMDev - needing your feedback here whether this is the best way to be determining at an operation-level whether any fragments are deferred; I think it should be since we're only doing direct selections.

I think this code should be valid, but I'm wondering if we could prevent another iteration through the entire selection set here.

What if we added hasDeferredFragments to the RootFieldBuilder.Result. We could keep track of this as a var on the RootFieldBuilder, and while building, if we encounter a deferred fragment, we just set that to true.

AnthonyMDev · 2023-08-08T19:18:02Z

Sources/ApolloCodegenLib/IR/IR+RootFieldBuilder.swift

+    private func scopeCondition(
+      for conditionalSelectionSet: ConditionallyIncludable,
+      in parentTypePath: SelectionSet.TypeInfo,
+      isDeferred: CompilationResult.IsDeferred


We should change the type of this parameter to IR.IsDeferred and do ``IsDeferred(compilationResult: namedFragment.isDeferred)` on line 247.

AnthonyMDev · 2023-08-08T19:21:19Z

Tests/ApolloCodegenTests/CodeGeneration/Templates/SelectionSet/SelectionSetTemplateTests.swift

+        .field("__typename", String.self),
+        .include(if: "filter", [
+          .inlineFragment(AsDogIfFilter.self, deferred: true),
+          .inlineFragment(AsDogIfFilter.self, deferred: .if("other")),


This looks like it's going to generate two separate selection sets both named AsDogIfFilter, which is not going to compile. We're going to need to handle this more carefully.

AnthonyMDev · 2023-08-08T19:27:31Z

Sources/ApolloCodegenLib/IR/ScopeDescriptor.swift

@@ -3,9 +3,6 @@ import OrderedCollections

 extension IR {

-  // TODO: Write tests that two inline fragments with same type and inclusion conditions,


I'm thinking we should also be adding tests for this behavior to the IR building tests, not just the SelectionSetTemplateTests. Making sure the actual IR is structured properly is important and having those tests makes it a LOT easier to debug and figure out the cause of bugs than just the template tests.

Sources/ApolloCodegenLib/Templates/RenderingHelpers/OperationTemplateRenderer.swift

AnthonyMDev · 2023-08-08T19:34:46Z

Sources/ApolloCodegenLib/Templates/RenderingHelpers/OperationTemplateRenderer.swift

+  func DeferredProperties(
+    _ definition: IR.Definition
+  ) -> TemplateString {
+    func isDeferred(_ selectionSet: IR.SelectionSet) -> Bool {


I think this code should be valid, but I'm wondering if we could prevent another iteration through the entire selection set here.

What if we added hasDeferredFragments to the RootFieldBuilder.Result. We could keep track of this as a var on the RootFieldBuilder, and while building, if we encounter a deferred fragment, we just set that to true.

AnthonyMDev · 2023-08-08T22:19:15Z

Sources/ApolloCodegenLib/IR/IR+RootFieldBuilder.swift

+        type: type,
+        conditions: inclusionResult.conditions,
+        isDeferred: IsDeferred(compilationResult: isDeferred)
+      )


These changes also should really be tested in tests on the RootFieldBuilder, not just the SelectionSetTemplateTests.

calvincestari added 12 commits August 4, 2023 13:41

Refactors CompilationResult constants

e2809cb

Exposes CompilationResult defer directive

4f071d7

Renders deferred inline fragments

e63a4da

Refactor for named fragment spread

3ca6c2b

Rearrange tests

945e3b7

Evaluate inclusion conditions for scope condition

d9738e7

Fix conditional deferred property label

24e9294

Add tests

91b6f7e

Fix named fragment accessor generation

9bc1413

Additional conditional type case test

0f9c8a8

Inline documentation

135b2f5

Additional test and cleanup

6238588

calvincestari requested a review from AnthonyMDev August 5, 2023 19:13

calvincestari commented Aug 5, 2023

View reviewed changes

Sources/ApolloCodegenLib/Frontend/CompilationResult.swift Show resolved Hide resolved

calvincestari commented Aug 5, 2023

View reviewed changes

Adds operation deferred property generation

4d217ca

calvincestari commented Aug 8, 2023

View reviewed changes

Refactor CompilationResult IsDeferred property

e4ebcd8

AnthonyMDev reviewed Aug 8, 2023

View reviewed changes

Refactored inclusion condition matching in root field builder

123c3f0

AnthonyMDev reviewed Aug 8, 2023

View reviewed changes

Refactor scope condition builder

f933c1e

calvincestari closed this Aug 10, 2023

calvincestari mentioned this pull request Aug 10, 2023

refactor: Update CompilationResult for deferred #3180

Merged

calvincestari deleted the defer/codegen-templates branch July 30, 2024 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Selection set generation for `@defer` #3176

refactor: Selection set generation for `@defer` #3176

calvincestari commented Aug 5, 2023 •

edited

Loading

calvincestari Aug 5, 2023

AnthonyMDev Aug 8, 2023

calvincestari Aug 8, 2023

calvincestari Aug 5, 2023

calvincestari Aug 5, 2023

AnthonyMDev Aug 8, 2023

calvincestari Aug 5, 2023

AnthonyMDev Aug 8, 2023

calvincestari Aug 8, 2023

AnthonyMDev Aug 8, 2023

AnthonyMDev Aug 8, 2023

AnthonyMDev Aug 8, 2023

AnthonyMDev Aug 8, 2023

AnthonyMDev Aug 8, 2023

AnthonyMDev Aug 8, 2023

		@@ -173,6 +181,27 @@ public class CompilationResult: JavaScriptObject {

		lazy var directives: [Directive]? = self["directives"]

		lazy var isDeferred: IsDeferred = {

		@@ -3,9 +3,6 @@ import OrderedCollections

		extension IR {

		// TODO: Write tests that two inline fragments with same type and inclusion conditions,

refactor: Selection set generation for @defer #3176

refactor: Selection set generation for @defer #3176

Conversation

calvincestari commented Aug 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refactor: Selection set generation for `@defer` #3176

refactor: Selection set generation for `@defer` #3176

calvincestari commented Aug 5, 2023 •

edited

Loading