-
-
Notifications
You must be signed in to change notification settings - Fork 2
Completed analysis of include and skip directives #106
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
base: dev
Are you sure you want to change the base?
Conversation
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.
Awesome job! This really starts to round out the tooling and provides much more support!
Minor comments on tests mostly.
src/analysis/QueryParser.ts
Outdated
// 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 |
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.
Remove this TODO
src/analysis/QueryParser.ts
Outdated
complexity += fragComplexity; | ||
this.depth += fragDepth; | ||
const directive = node.directives; | ||
if (directive && this.directiveCheck(directive[0])) { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
Remove this TODO
@@ -1008,17 +1009,48 @@ describe('Test getQueryTypeComplexity function', () => { | |||
hero(episode: EMPIRE) { | |||
id, name | |||
} | |||
human(id: 1) @includes(if: $directive) { | |||
human(id: 1) @skip(if: $directive) { |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Differentiate the weights of here
and human
variables = { directive: true }; | ||
queryParser = new QueryParser(typeWeights, variables); | ||
query = `query ($directive: Boolean!){ | ||
hero(episode: EMPIRE) { |
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.
here as well
}); | ||
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Differentiate weights here as well
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.
Requesting changes per previous comment.
…ation between fields
Summary
@skip
and@include
directives for more accurate cost analysis for queries using these directivesType of Change
Issues
closes #95
Evidence