-
Notifications
You must be signed in to change notification settings - Fork 248
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
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. |
fd11718
to
2d5de87
Compare
Added tests and made a few improvements. This is now ready for review. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, using equals
should be just fine here.
internals-js/src/operations.ts
Outdated
} | ||
|
||
contains(that: Selection): boolean { | ||
if ((that instanceof FragmentSpreadSelection) |
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.
Use equals() here?
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.
Good point.
} | ||
|
||
contains(that: Selection): boolean { | ||
return (that instanceof FragmentSelection) |
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.
Use equals()?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
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.