Completed analysis of include and skip directives#106
Completed analysis of include and skip directives#106evanmcneely wants to merge 8 commits intodevfrom
Conversation
shalarewicz
left a comment
shalarewicz
left a comment
There was a problem hiding this comment.
Awesome job! This really starts to round out the tooling and provides much more support!
Minor comments on tests mostly.
| // named type is the type from which inner fields should be take | ||
| // If the TypeCondition is omitted, an inline fragment is considered to be of the same type as the enclosing context | ||
| const namedType = typeCondition ? typeCondition.name.value.toLowerCase() : parentName; | ||
| // TODO: Handle directives like @include and @skip |
| complexity += fragComplexity; | ||
| this.depth += fragDepth; | ||
| const directive = node.directives; | ||
| if (directive && this.directiveCheck(directive[0])) { |
There was a problem hiding this comment.
We should also handle cases where there are multiple directives attached to this node rather than just checking the first directive.
There was a problem hiding this comment.
I've added this. What is your take on this check:
if (node.directives && !this.directiveExcludeField([...node.directives])) {
//code
}node.directives is always truthy but TS requires the check in order to access the array value at node.directives.
| @@ -855,7 +855,7 @@ describe('Test getQueryTypeComplexity function', () => { | |||
| }); | |||
|
|
|||
| // TODO: refine complexity analysis to consider directives includes and skip | |||
| id, name | ||
| } | ||
| human(id: 1) @includes(if: $directive) { | ||
| human(id: 1) @skip(if: $directive) { |
There was a problem hiding this comment.
The weight of Human and hero are both 1. Can you add something that differentiates the weights so we know that this is working.
| variables = { directive: false }; | ||
| queryParser = new QueryParser(typeWeights, variables); | ||
| query = `query ($directive: Boolean!){ | ||
| hero(episode: EMPIRE) { |
There was a problem hiding this comment.
Same here. Differentiate the weights of here and human
| variables = { directive: true }; | ||
| queryParser = new QueryParser(typeWeights, variables); | ||
| query = `query ($directive: Boolean!){ | ||
| hero(episode: EMPIRE) { |
| xtest('and other directive are ignored', () => { | ||
| test('and other directives or arguments are ignored', () => { | ||
| query = `query { | ||
| hero(episode: EMPIRE) { |
There was a problem hiding this comment.
Differentiate weights here as well
shalarewicz
left a comment
shalarewicz
left a comment
There was a problem hiding this comment.
Requesting changes per previous comment.
…ation between fields
|
I ended up refactoring things a bit to check all directives in the array.
|
2 participants
Summary
@skipand@includedirectives for more accurate cost analysis for queries using these directivesType of Change
Issues
closes #95
Evidence