Skip to content
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.

Sign up for GitHub

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

keep deferred inFlightLinkObservables until the response is finished #12338

Merged
merged 5 commits into from
Feb 6, 2025
Merged

keep deferred inFlightLinkObservables until the response is finished #12338

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e635084
keep deferred `inFlightLinkObservables` until the response is finished
phryneas Feb 4, 2025
795064e
changeset
phryneas Feb 4, 2025
0b6817e
add test
phryneas Feb 6, 2025
26f422e
adjust changeset
phryneas Feb 6, 2025
b201bc0
size limits
phryneas Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
keep deferred inFlightLinkObservables until the response is finished
  • Loading branch information
@phryneas
phryneas committed Feb 6, 2025
commit e635084ecb1270ae3efb5bfca4f663979919b818
8 changes: 6 additions & 2 deletions src/core/QueryManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1182,8 +1182,12 @@ export class QueryManager<TStore> {
]);
observable = entry.observable = concast;

concast.beforeNext(() => {
inFlightLinkObservables.remove(printedServerQuery, varJson);
concast.beforeNext(function cb(method, arg: FetchResult) {
if (method === "next" && "hasNext" in arg && arg.hasNext) {
concast.beforeNext(cb);
} else {
inFlightLinkObservables.remove(printedServerQuery, varJson);
}
Comment on lines +1185 to +1190
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this change right now applies to @defer etc., but not to subscriptions.
Changing that for subscriptions would be nice, but feels a bit more breaking, so I think we should probably do that in 4.0 - WDYT @jerelmiller ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me. I see no reason why two subscriptions kicked off with the same document/variables shouldn't be deduped, especially since it can be disabled per-subscription if need be. Mind making a tracking issue and putting it in the milestone so that we don't forget to update?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#12348 👍

});
}
} else {
Expand Down