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.

Sign up for GitHub

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

Jump to bottom

refactor: Selection set generation for @defer #3176

Closed
wants to merge 16 commits into from
Closed

refactor: Selection set generation for @defer #3176

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
e2809cb
Refactors CompilationResult constants
calvincestari Aug 4, 2023
4f071d7
Exposes CompilationResult defer directive
calvincestari Aug 4, 2023
e63a4da
Renders deferred inline fragments
calvincestari Aug 4, 2023
3ca6c2b
Refactor for named fragment spread
calvincestari Aug 4, 2023
945e3b7
Rearrange tests
calvincestari Aug 4, 2023
d9738e7
Evaluate inclusion conditions for scope condition
calvincestari Aug 5, 2023
24e9294
Fix conditional deferred property label
calvincestari Aug 5, 2023
91b6f7e
Add tests
calvincestari Aug 5, 2023
9bc1413
Fix named fragment accessor generation
calvincestari Aug 5, 2023
0f9c8a8
Additional conditional type case test
calvincestari Aug 5, 2023
135b2f5
Inline documentation
calvincestari Aug 5, 2023
6238588
Additional test and cleanup
calvincestari Aug 5, 2023
4d217ca
Adds operation deferred property generation
calvincestari Aug 8, 2023
e4ebcd8
Refactor CompilationResult IsDeferred property
calvincestari Aug 8, 2023
123c3f0
Refactored inclusion condition matching in root field builder
calvincestari Aug 8, 2023
f933c1e
Refactor scope condition builder
calvincestari Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 67 additions & 4 deletions Sources/ApolloCodegenLib/Frontend/CompilationResult.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,18 @@ import JavaScriptCore

/// The output of the frontend compiler.
public class CompilationResult: JavaScriptObject {
/// String constants used to match JavaScriptObject instances.
private enum Constants {
static let LocalCacheMutationDirectiveName = "apollo_client_ios_localCacheMutation"
static let DeferDirectiveName = "defer"
enum DirectiveNames {
static let LocalCacheMutation = "apollo_client_ios_localCacheMutation"
static let Defer = "defer"
}

enum ArgumentLabels {
static let `If` = "if"
}
}

lazy var rootTypes: RootTypeDefinition = self["rootTypes"]

lazy var referencedTypes: [GraphQLNamedType] = self["referencedTypes"]
Expand Down Expand Up @@ -54,7 +62,7 @@ public class CompilationResult: JavaScriptObject {
}

lazy var isLocalCacheMutation: Bool = {
directives?.contains { $0.name == Constants.LocalCacheMutationDirectiveName } ?? false
directives?.contains { $0.name == Constants.DirectiveNames.LocalCacheMutation } ?? false
}()

lazy var nameWithSuffix: String = {
Expand Down Expand Up @@ -117,7 +125,7 @@ public class CompilationResult: JavaScriptObject {
lazy var directives: [Directive]? = self["directives"]

lazy var isLocalCacheMutation: Bool = {
directives?.contains { $0.name == Constants.LocalCacheMutationDirectiveName } ?? false
directives?.contains { $0.name == Constants.DirectiveNames.LocalCacheMutation } ?? false
}()

public override var debugDescription: String {
Expand Down Expand Up @@ -173,6 +181,27 @@ public class CompilationResult: JavaScriptObject {

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

lazy var isDeferred: IsDeferred = {
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

guard let directive = directives?.first(where: { $0.name == Constants.DirectiveNames.Defer }) else {
return false
}

guard let argument = directive.arguments?.first(where: { $0.name == Constants.ArgumentLabels.If }) else {
return true
}

switch (argument.value) {
case let .boolean(value):
return .value(value)

case let .variable(variable):
return .variable(variable)

default:
preconditionFailure("Incompatible argument value. Expected Boolean or Variable, got \(argument.value).")
}
}()

public override var debugDescription: String {
selectionSet.debugDescription
}
Expand All @@ -197,6 +226,27 @@ public class CompilationResult: JavaScriptObject {

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

lazy var isDeferred: IsDeferred = {
guard let directive = directives?.first(where: { $0.name == Constants.DirectiveNames.Defer }) else {
return false
}

guard let argument = directive.arguments?.first(where: { $0.name == Constants.ArgumentLabels.If }) else {
return true
}

switch (argument.value) {
case let .boolean(value):
return .value(value)

case let .variable(variable):
return .variable(variable)

default:
preconditionFailure("Incompatible argument value. Expected Boolean or Variable, got \(argument.value).")
}
}()

var parentType: GraphQLCompositeType { fragment.type }

public func hash(into hasher: inout Hasher) {
Expand Down Expand Up @@ -412,4 +462,17 @@ public class CompilationResult: JavaScriptObject {
}
}

public enum IsDeferred: ExpressibleByBooleanLiteral {
calvincestari marked this conversation as resolved.
Show resolved Hide resolved
case value(Bool)
case variable(String)

public init(booleanLiteral value: BooleanLiteralType) {
self = .value(value)
}

static func `if`(_ variable: String) -> Self {
.variable(variable)
}
}

}
15 changes: 15 additions & 0 deletions Sources/ApolloCodegenLib/IR/IR+InclusionConditions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,21 @@ extension IR {
self.conditions = [condition]
}

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

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

guard let conditions, !conditions.isEmpty else { return nil }

self.conditions = []
self.conditions.append(contentsOf: conditions.map({ condition in
switch condition {
case let .variable(variable, inverted):
return InclusionCondition(variable, isInverted: inverted)
default:
preconditionFailure("Cannot convert condition without variable, got \(condition)!")
}
}))
}

static func allOf<T: Sequence>(
_ conditions: T
) -> Result where T.Element == InclusionCondition {
Expand Down
29 changes: 27 additions & 2 deletions Sources/ApolloCodegenLib/IR/IR+RootFieldBuilder.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,8 +230,29 @@ extension IR {
}

private func scopeCondition(
for conditionalSelectionSet: ConditionallyIncludable,
for inlineFragment: CompilationResult.InlineFragment,
in parentTypePath: SelectionSet.TypeInfo
) -> ScopeCondition? {
scopeCondition(for: inlineFragment, in: parentTypePath, isDeferred: inlineFragment.isDeferred)
}

private func scopeCondition(
for namedFragment: CompilationResult.FragmentSpread,
in parentTypePath: SelectionSet.TypeInfo
) -> ScopeCondition? {
let matchedParentTypes = (parentTypePath.parentType == namedFragment.parentType)
let matchedInclusionConditions = (parentTypePath.inclusionConditions == InclusionConditions(namedFragment.inclusionConditions))

let scopedIsDeferred = matchedParentTypes && matchedInclusionConditions
? namedFragment.isDeferred : false

return scopeCondition(for: namedFragment, in: parentTypePath, isDeferred: scopedIsDeferred)
}

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

Choose a reason for hiding this comment

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

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

) -> ScopeCondition? {
let inclusionResult = inclusionResult(for: conditionalSelectionSet.inclusionConditions)
guard inclusionResult != .skipped else {
Expand All @@ -241,7 +262,11 @@ extension IR {
let type = parentTypePath.parentType == conditionalSelectionSet.parentType ?
nil : conditionalSelectionSet.parentType

return ScopeCondition(type: type, conditions: inclusionResult.conditions)
return ScopeCondition(
type: type,
conditions: inclusionResult.conditions,
isDeferred: IsDeferred(compilationResult: isDeferred)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

private func inclusionResult(
Expand Down
13 changes: 12 additions & 1 deletion Sources/ApolloCodegenLib/IR/IR.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,22 @@ class IR {
}
}

// TODO: Documentation for this to be completed in issue #3141
/// Represents the `@defer` directive on an inline or named fragment spread. Can be expressed as
/// a Boolean literal or a conditional with a variable.
enum IsDeferred: Hashable, ExpressibleByBooleanLiteral {
case value(Bool)
case `if`(_ variable: String)

init(compilationResult isDeferred: CompilationResult.IsDeferred) {
switch isDeferred {
case let .value(value):
self = .value(value)

case let .variable(variable):
self = .if(variable)
}
}

init(booleanLiteral value: BooleanLiteralType) {
switch value {
case true:
Expand Down
3 changes: 0 additions & 3 deletions Sources/ApolloCodegenLib/IR/ScopeDescriptor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@ import OrderedCollections

extension IR {

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// but different defer conditions don't merge together.
// To be done in issue #3141
struct ScopeCondition: Hashable, CustomDebugStringConvertible {
let type: GraphQLCompositeType?
let conditions: InclusionConditions?
Expand Down
31 changes: 24 additions & 7 deletions Sources/ApolloCodegenLib/Templates/SelectionSetTemplate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ struct SelectionSetTemplate {
_ deprecatedArguments: inout [DeprecatedArgument]?
) -> [TemplateString] {
selections.fields.values.map { FieldSelectionTemplate($0, &deprecatedArguments) } +
selections.inlineFragments.values.map { InlineFragmentSelectionTemplate($0.selectionSet) } +
selections.inlineFragments.values.map { InlineFragmentSelectionTemplate($0) } +
selections.namedFragments.values.map { FragmentSelectionTemplate($0) }
}

Expand Down Expand Up @@ -277,15 +277,15 @@ struct SelectionSetTemplate {
"""
}

private func InlineFragmentSelectionTemplate(_ inlineFragment: IR.SelectionSet) -> TemplateString {
private func InlineFragmentSelectionTemplate(_ inlineFragment: IR.InlineFragmentSpread) -> TemplateString {
"""
.inlineFragment(\(inlineFragment.renderedTypeName).self)
.inlineFragment(\(inlineFragment.selectionSet.renderedTypeName).self\(inlineFragment.isDeferred.rendered))
"""
}

private func FragmentSelectionTemplate(_ fragment: IR.NamedFragmentSpread) -> TemplateString {
private func FragmentSelectionTemplate(_ namedFragment: IR.NamedFragmentSpread) -> TemplateString {
"""
.fragment(\(fragment.definition.name.asFragmentName).self)
.fragment(\(namedFragment.definition.name.asFragmentName).self\(namedFragment.isDeferred.rendered))
"""
}

Expand Down Expand Up @@ -381,8 +381,10 @@ struct SelectionSetTemplate {
let name = fragment.definition.name
let propertyName = name.firstLowercased
let typeName = name.asFragmentName
let isOptional = fragment.inclusionConditions != nil &&
!scope.matches(fragment.inclusionConditions.unsafelyUnwrapped)

let isOptional =
(fragment.inclusionConditions != nil && !scope.matches(fragment.inclusionConditions.unsafelyUnwrapped))
|| fragment.isDeferred != false

return """
\(renderAccessControl())var \(propertyName): \(typeName)\
Expand Down Expand Up @@ -982,3 +984,18 @@ extension IR.SelectionSet.Selections {

}
}

fileprivate extension IR.IsDeferred {
var rendered: String {
switch self {
case .value(true):
return ", deferred: true"

case .value(false):
return ""

case let .if(variable):
return ", deferred: .if(\"\(variable)\")"
}
}
}
Loading