Fix method removing empty branches from selection sets #2125
Conversation
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
| @@ -1932,9 +1926,13 @@ class FragmentSpreadSelection extends FragmentSelection { | |||
| export function operationFromDocument( | |||
| schema: Schema, | |||
| document: DocumentNode, | |||
| operationName?: string, | |||
| options?: { | |||
There was a problem hiding this comment.
Note for reviewer: I didn't wanted to add the new validate as a nameless argument because that was starting to get ugly, especially when the operationName was already optional. But at the same time, operationFromDocument is called in a ton of places (particularly tests), almost always with just the 2 first arguments, so switching to all argument being name-based was pretty annoying and would have generated a big diff. Hence why I opted for this compromised of keeping the 2 first main argument nameless and putting everything else in an "options" object.
| } | ||
|
|
||
| const updatedSelectionSet = this.selectionSet.filter(predicate); | ||
| return this.selectionSet === updatedSelectionSet | ||
| const thisWithFilteredSelectionSet = this.selectionSet === updatedSelectionSet |
There was a problem hiding this comment.
Are you sure you want === here? It seems a little strange to me that we'd do that type of equality test immediately after filtering, I sould expect some sort of deep equality check, but I'm not looking into it too deeply.
There was a problem hiding this comment.
Yeah, this code is just an optimisation. The point is to save allocating a new FieldSelection object (or, in the case of SelectionSet.filter, a SelectionSet object) in the case where nothing was filtered. Which is expected to be common, both because we use filter in case where we know filtering rarely kicks in, like in that patch with empty branches which happens but not at all in all cases, but also because the method is recursive and while filtering may happen in some branches of a selection set, it probably still won't happen in most of them).
Anyway, using deep equality doesn't make sense in term of such optimisation since we'd lose more in computing the deep equality check in general than what we'd save in allocations.
| // Note that we essentially expand all fragments as part of this. | ||
| const selectionSet = this.selectionSet; | ||
| const updatedSelectionSet = selectionSet.filter(predicate); | ||
| return updatedSelectionSet === selectionSet | ||
| const thisWithFilteredSelectionSet = updatedSelectionSet === selectionSet |
There was a problem hiding this comment.
Same question here as above.
As the code for @defer build the selection set used for "primary parts", it may start adding "branches" (selections) that end up being empty because everything within that branch is actually deferred (and thus not part of this primary part). So once we've finished building everything, we call a `withoutEmptyBranches` method on the selection set that can have such (invalid for graphQL) empty branches to remove them. Unfortunately, the logic for that method was broken: it was removing empty branches in simple cases, but the recursion was done the wrong way so that if removing an empty "leaf" left its parent empty, this wasn't removed properly. In other words, given `{ a b { c {} } }` the method was returning: `{ a b { } }` instead of the proper: `{ a }` This commit fixes the logic of that method to behave correctly. Fixes apollographql#2123
pcmanus
force-pushed
the
defer-related-issues
branch
from
12300a9 to
6d323c8
Compare
As the code for
@deferbuild the selection set used for "primary parts",it may start adding "branches" (selections) that end up being empty
because everything within that branch is actually deferred (and thus
not part of this primary part). So once we've finished building
everything, we call a
withoutEmptyBranchesmethod on the selection setthat can have such (invalid for graphQL) empty branches to remove
them.
Unfortunately, the logic for that method was broken: it was removing
empty branches in simple cases, but the recursion was done the wrong
way so that if removing an empty "leaf" left its parent empty, this
wasn't removed properly. In other words, given
{ a b { c {} } }the method was returning:
{ a b { } }instead of the proper:
{ a }This commit fixes the logic of that method to behave correctly.
Fixes #2123