-
Notifications
You must be signed in to change notification settings - Fork 722
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
refactor: Selection set generation for @defer
#3176
Changes from 13 commits
e2809cb
4f071d7
e63a4da
3ca6c2b
945e3b7
d9738e7
24e9294
91b6f7e
9bc1413
0f9c8a8
135b2f5
6238588
4d217ca
e4ebcd8
123c3f0
f933c1e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,6 +54,21 @@ extension IR { | |
self.conditions = [condition] | ||
} | ||
|
||
/// A failable initializer to convert from an array of `CompilationResult.InclusionCondition`. | ||
init?(_ conditions: [CompilationResult.InclusionCondition]?) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @AnthonyMDev - I've added this since we determined that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we could instead have a custom There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I think that a custom matching function that is |
||
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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should change the type of this parameter to |
||
) -> ScopeCondition? { | ||
let inclusionResult = inclusionResult(for: conditionalSelectionSet.inclusionConditions) | ||
guard inclusionResult != .skipped else { | ||
|
@@ -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) | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These changes also should really be tested in tests on the |
||
} | ||
|
||
private func inclusionResult( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,9 +3,6 @@ import OrderedCollections | |
|
||
extension IR { | ||
|
||
// TODO: Write tests that two inline fragments with same type and inclusion conditions, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
// but different defer conditions don't merge together. | ||
// To be done in issue #3141 | ||
struct ScopeCondition: Hashable, CustomDebugStringConvertible { | ||
let type: GraphQLCompositeType? | ||
let conditions: InclusionConditions? | ||
|
@@ -224,6 +221,10 @@ extension IR { | |
return true | ||
} | ||
|
||
var isDeferred: Bool { | ||
scopePath.contains { $0.isDeferred != false } | ||
} | ||
|
||
static func == (lhs: ScopeDescriptor, rhs: ScopeDescriptor) -> Bool { | ||
lhs.scopePath == rhs.scopePath && | ||
lhs.matchingTypes == rhs.matchingTypes | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,4 +50,40 @@ extension OperationTemplateRenderer { | |
""" | ||
} | ||
|
||
func DeferredProperties( | ||
calvincestari marked this conversation as resolved.
Show resolved
Hide resolved
|
||
_ definition: IR.Definition | ||
) -> TemplateString { | ||
func isDeferred(_ selectionSet: IR.SelectionSet) -> Bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
guard !selectionSet.scope.isDeferred else { return true } | ||
|
||
if let fields = selectionSet.selections.direct?.fields { | ||
for field in fields.values { | ||
if let entityField = field as? IR.EntityField { | ||
guard !isDeferred(entityField.selectionSet) else { return true } | ||
} | ||
} | ||
} | ||
|
||
if let inlineFragments = selectionSet.selections.direct?.inlineFragments { | ||
for fragment in inlineFragments.values { | ||
guard !isDeferred(fragment.selectionSet) else { return true } | ||
} | ||
} | ||
|
||
if let namedFragments = selectionSet.selections.direct?.namedFragments { | ||
for fragment in namedFragments.values { | ||
return (fragment.isDeferred != false) | ||
} | ||
} | ||
|
||
return false | ||
} | ||
|
||
return """ | ||
\(if: isDeferred(definition.rootField.selectionSet), """ | ||
public static let hasDeferredFragments: Bool = true | ||
""") | ||
""" | ||
} | ||
|
||
} |
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 duplicated for both
InlineFragment
andFragmentSpread
, I'm not sure if there is a better way to do it; they're both just simpleJavaScriptObject
types.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.
Wish we could easily namespace this
Deferrable
protocol inCompilationResult
. Until we breakCompilationResult
into it's own package, we might want to call the protocolCompilationResult_Deferrable
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.
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.