Fix cases where fragment could be reused but weren't #2541
Conversation
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
🦋 Changeset detectedLatest commit: 44a33ae The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
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 |
|
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. |
52056e2 to
851f63b
Compare
internals-js/src/operations.ts
Outdated
| @@ -852,6 +859,7 @@ export class NamedFragmentDefinition extends DirectiveTargetElement<NamedFragmen | |||
|
|
|||
| setSelectionSet(selectionSet: SelectionSet): NamedFragmentDefinition { | |||
| assert(!this._selectionSet, 'Attempting to set the selection set of a fragment definition already built') | |||
| assert(selectionSet.parentType === this.typeCondition, `${selectionSet.parentType} !== ${this.typeCondition}`); | |||
There was a problem hiding this comment.
It would be helpful to explain what programming error this assertion intends to catch or have a test that exercises it (I assume this isn't targeted at a user error) since tests pass with or without it.
There was a problem hiding this comment.
I tried to expand a bit in comments and hope it helps (but no, it isn't targeted at a user error).
internals-js/src/operations.ts
Outdated
| * This methods *assumes* that `this.canApplyAtType(type)` is `true` (and may crash if this is not true), and returns | ||
| * a version so this fragment selection set "optimized" when the "current type" is `type`. |
There was a problem hiding this comment.
I can't quite make sense of this comment, can you correct the grammar?
and returns a version so this fragment selection set "optimized" when the "current type" is
type
There was a problem hiding this comment.
Modified. Hopefully that's clearer now.
| if (schema === this.schema()) { | ||
| return true; | ||
| /** | ||
| * This methods *assumes* that `this.canApplyAtType(type)` is `true` (and may crash if this is not true), and returns |
There was a problem hiding this comment.
I figured this comment meant that callers should assert that this.canApplyType(type) returns true (and handle false appropriately) before calling this function but that isn't the case. Should we be making that assertion here or at the call site, or is it ok to do neither?
There was a problem hiding this comment.
I figured this comment meant that callers should assert that
this.canApplyType(type)returnstrue(...)
That is indeed what the comment meant.
(...) but that isn't the case
It is actually the case, but it is slightly indirect. I've added some comments to indicate where that property is checked and where is it propagated through callers.
But I'll note that this.canApplyAtType is not necessarily always super cheap, which is why I'd rather not assert it.
a670859 to
52057c9
Compare
The main case that was handled properly was the case where a fragment that is on an abstract type is applied somewhere where the "current type" is an object type. In that case, some sub-parts (the one that don't match the "curren type") of the fragment are effectively "dead branches", but the code was still trying to matching those in the result, preventing some reuse of the fragment. Another smaller issue was that we sometimes weren't correctly reusing fragments at the very top of a "selection set". This didn't really matter too much for queries in practice, but this was also impacting some case where fragments where use at the top level of some other fragments, also preventing a few reuse. Lastly, fixing this highlighted the fact that the we were always using fragments from the original query, so against the API schema, even when building subgraph queries, and this was creating issues (the code was trying to compensate for this is a few places, but this was confusing and was not covering all cases correctly). So the patch clean this up too, and rebased fragments on subgraph before trying to use them for optimizing said subgraph fetches, ensuring that the code don't mix element from different schema.
52057c9 to
44a33ae
Compare
The main case that was handled properly was the case where a fragment that is on an abstract type is applied somewhere where the "current type" is an object type. In that case, some sub-parts (the one that don't match the "curren type") of the fragment are effectively "dead branches", but the code was still trying to matching those in the result, preventing some reuse of the fragment.
Another smaller issue was that we sometimes weren't correctly reusing fragments at the very top of a "selection set". This didn't really matter too much for queries in practice, but this was also impacting some case where fragments where use at the top level of some other fragments, also preventing a few reuse.
Lastly, fixing this highlighted the fact that the we were always using fragments from the original query, so against the API schema, even when building subgraph queries, and this was creating issues (the code was trying to compensate for this is a few places, but this was confusing and was not covering all cases correctly). So the patch clean this up too, and rebased fragments on subgraph before trying to use them for optimizing said subgraph fetches, ensuring that the code don't mix element from different schema.