Improve fragment reuse #2497
Improve fragment reuse #2497
Conversation
🦋 Changeset detectedLatest commit: 1ae55d3 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 |
👷 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. |
|
Marking this as draft for now because I need to write much more tests for this. But it passes tests and was shown to make a big difference on some real world example. |
pcmanus
force-pushed
the
improve-fragment-reuse
branch
2 times, most recently
from
fd11718
to
2d5de87
Compare
|
Added tests and made a few improvements. This is now ready for review. |
pcmanus
force-pushed
the
improve-fragment-reuse
branch
from
2d5de87
to
18ff304
Compare
.changeset/rich-kings-obey.md
Outdated
| Improves reuse of named fragments in subgraph fetches. When a question has named fragments, the code tries to reuse | ||
| those fragment in subgraph fetches is those can apply (so when the fragment is fully queried in a single subgraph fetch). | ||
| However, the existing was only able to reuse those fragment in a small subset of cases. This change makes it much more | ||
| like that _if_ a fragment can be reused, it will be. |
There was a problem hiding this comment.
| like that _if_ a fragment can be reused, it will be. | |
| likely that _if_ a fragment can be reused, it will be. |
internals-js/src/operations.ts
Outdated
| @@ -2475,6 +2652,36 @@ class FragmentSpreadSelection extends FragmentSelection { | |||
| assert(false, 'Unsupported, see `Operation.withAllDeferLabelled`'); | |||
| } | |||
|
|
|||
| minus(that: Selection): undefined { | |||
| assert( | |||
| that instanceof FragmentSpreadSelection && this.namedFragment.name === that.namedFragment.name, | |||
There was a problem hiding this comment.
Noticing that this is a slightly different clause than is used in equals() further down. Is there a reason for that? If not, should this assertion just use equals()?
There was a problem hiding this comment.
Good point, using equals should be just fine here.
internals-js/src/operations.ts
Outdated
| } | ||
|
|
||
| contains(that: Selection): boolean { | ||
| if ((that instanceof FragmentSpreadSelection) |
| } | ||
|
|
||
| contains(that: Selection): boolean { | ||
| return (that instanceof FragmentSelection) |
There was a problem hiding this comment.
That one we want to keep as-is. equals recursively calls equals on the subselection, but here we're recursively calling contains instead.
internals-js/src/operations.ts
Outdated
| contains(that: SelectionSet): boolean { | ||
| if (this._selections.length < that._selections.length) { | ||
| return false; | ||
| private triviallyNestedSelectionsFoKey(parentType: CompositeType, key: string): Selection[] { |
There was a problem hiding this comment.
Here and below
| private triviallyNestedSelectionsFoKey(parentType: CompositeType, key: string): Selection[] { | |
| private triviallyNestedSelectionsForKey(parentType: CompositeType, key: string): Selection[] { |
The existing code was only able to reuse named fragments in a small subset of cases (essentially when the fragment was matched by a full sub-selection). This patch makes the code much more able to reuse fragments by allowing to match sub-selections, handle intersecting fragments, etc...
pcmanus
force-pushed
the
improve-fragment-reuse
branch
from
18ff304
to
1ae55d3
Compare
The code to try to reuse named fragments inside subgraph queries was a bit "crude" and there was plenty of cases where fragments weren't being reused. The main culprit was know and described in this todo, but there is a few other small points addressed by this PR.