Refactor removeDirectivesFromDocument to fix AST ordering sensitivities and avoid 1/3 AST traversals
#10560
Conversation
🦋 Changeset detectedLatest commit: 251a128 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
benjamn
added
🐎 performance
🧞♂️ enhancement
🧩 implementation-detail
🧪 has-tests
🥚 backwards-compatible
| function getAllFragmentSpreadsFromSelectionSet( | ||
| selectionSet: SelectionSetNode, | ||
| ): FragmentSpreadNode[] { |
There was a problem hiding this comment.
This helper function is no longer needed because we no longer have to traverse subtrees removed by @client directives to find the fragment spreads they contain. Instead, we simply fail to record fragments spreads within removed subtrees, and later remove any/all unused fragments.
| return getInUseByFragmentName(ancestor.name.value); | ||
| } | ||
| } | ||
| invariant.error(`Could not find operation or fragment`); |
There was a problem hiding this comment.
Should we add a test for the error case?
Edit: it seems it would always be a GraphQL syntax error? In which case I see it wouldn't be necessary.
There was a problem hiding this comment.
I can't think of a reason this error should ever happen (assuming well formed GraphQL input), but I'm a little hesitant to throw here.
benjamn
force-pushed
the
rebase-pr-6322-to-fix-issue-6311
branch
2 times, most recently
from
d5471bf
to
fc20585
Compare
benjamn
force-pushed
the
rebase-pr-6322-to-fix-issue-6311
branch
from
fc20585
to
ced0bd0
Compare
Consider the following schema on the server:
```graphql
interface Character {
appearsIn: [Episode]!
}
type Human implements Character {
appearsIn: [Episode]!
height: Int
}
```
Then the following addition on the client:
```graphql
type Droid implements Character {
appearsIn: [Episode]!
primaryFunction: String
}
```
With the following query:
```graphql
query HeroForEpisode($ep: Episode!) {
hero(episode: $ep) {
... on Human {
height
}
... on Droid @client {
primaryFunction
}
}
}
```
The server will complain that the fragment spread has no type overlap
with parent, as the server does not have knowledge of the `Droid` type.
This commit strips out inline fragments which use a client directive,
allowing them to be resolved via local state.
…ion` for clarity Co-authored-by: Alessia Bellisario <alessia@apollographql.com>
benjamn
force-pushed
the
rebase-pr-6322-to-fix-issue-6311
branch
from
ced0bd0
to
1a2bd3d
Compare
This refactoring of
removeDirectivesFromDocumentbuilds on (rebases) the work begun in PR #6322, fixing both the original issue #6311 and the related issue #10539.At a high level, these changes reduce the number of full-document AST traversals performed by
removeDirectivesFromDocumentfrom 3 to 2. Future work could potentially replace the second traversal with a more targeted manual cleanup pass, to improve performance further.In the first of those two traversals, we gather as much information as we can about the AST, including all the variable references and fragments spreads used by each operation or fragment definition. At the same time, we can safely remove subtrees annotated with removal directives like
@client, which effectively hides those removed subtrees from the information collected, and from future traversals.For fragment definitions modified by these removals, if they are left with only a
__typenamefield, we proactively remove the whole fragment definition along with any...spreadreferences to it. However, if a fragment definition contains only a__typenamefield but is not otherwise modified, that fragment definition remains unmodified in the AST (preserving the spirit of #6311), unless there are no...spreadreferences to it, in which case it must be removed, like any other unused fragment definition.An additional exception is made for documents that contain only fragment definitions, with no operations consuming them: removing these "unused" fragments seems wrong, so the removal of unused and/or
__typename-only fragment definitions is disabled when there are no operations in the document, on the presumption these "unused" fragment definitions must have a use beyond this document (for example, selecting a specific fragment from a document containing several definitions usinguseFragment'soptions.fragmentName).Using the information obtained in the first visit, a second visit brings all fragment spreads and definitions into agreement with each other, existence-wise, so there can be no dangling spreads or unused definitions in the final document. This pass also prunes variable declarations from operations that no longer use some of their declared variables, including usages directly within the operation and in any transitively included fragments.
I've added several more tests to enforce more edge cases around ordering sensitivity and transitive variable/fragment usage. I believe this PR has roughly the same level of backwards compatibility as the previous PR #6322, though of course the changes are more significant now, so we might want to add even more tests.