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.

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

Batch link can cancel operations that are in queue or in flight #9248

Merged
merged 11 commits into from
Feb 2, 2022

Conversation

PowerKiKi
Copy link
Contributor

After an operation has been subscribed to, and so queued, it is possible
to unsubscribe from it, and it will be removed from the queue.

Unsubscribing will not impact the debounce, so other operations, if any, will
not be delayed by an unsubscription.

If a batch of operation is already in flight, and all operations are unsubscribed
then the entire XHR will be cancelled. If only some operations are unsubscribed
the XHR will be left untouched.

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

PowerKiKi added a commit to PowerKiKi/apollo-angular that referenced this pull request Dec 28, 2021
This is dependent on apollographql/apollo-client#9248,
so it is not strictly specific to `apollo-angular`, but it seemed important to
ensure that this package does indeed allow to cancel XHR even when batching is
enabled.
PowerKiKi and others added 7 commits January 11, 2022 18:15
After an operation has been subscribed to, and so queued, it is possible
to unsubscribe from it, and it will be removed from the queue.

Unsubscribing will not impact the debounce, so other operations, if any, will
not be delayed by an unsubscription.

If a batch of operation is already in flight, and all operations are unsubscribed
then the entire XHR will be cancelled. If only some operations are unsubscribed
the XHR will be left untouched.
Since BatchHandler is an exported type, it seems appropriate to preserve
its optional fields (with their expected types), then override those
fields (next, error, complete) with non-optional versions in the
QueuedRequest subtype. The field/types of QueuedRequest will end up the
same either way.
Since ECMAScript Set and Map preserve the order of their keys (by order
of first insertion), they can often be used to keep track of queues or
LRU usage chains, while also supporting constant-time deletion of keys
(no need for indexOf/splice).
Since we're already changing the type of this pseudo-private property,
it seems like a good idea also to privatize/rename it for real, so we
can test the hypothesis "nobody is using this field directly" by
trialing these changes in beta releases of Apollo Client v3.6.
@benjamn benjamn changed the base branch from main to release-3.6 January 12, 2022 16:51
@benjamn benjamn added this to the Release 3.6 milestone Jan 12, 2022
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this functionality and adding tests @PowerKiKi!

I started making a few tweaks and got perhaps a little carried away (not everything I changed is in direct response to your code, just general cleanup). That said, please let me know if anything I added doesn't look right to you. Otherwise I'll merge this into release-3.6 and publish another beta release soon (this week) so you can try it out.

src/link/batch/batching.ts Outdated Show resolved Hide resolved
@benjamn benjamn mentioned this pull request Jan 18, 2022
src/link/batch/batching.ts Outdated Show resolved Hide resolved
src/link/batch/batching.ts Outdated Show resolved Hide resolved
Because those are never used inside this project, and they are very
likely to break our indices based callback calls and throw an
`server returned results with length...`.
@PowerKiKi
Copy link
Contributor Author

I did a new series of (small) commits. IMHO it address all things we said until now, and it could be final reviewed/merged.

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Thanks all!

@benjamn benjamn merged commit cefd24c into apollographql:release-3.6 Feb 2, 2022
@PowerKiKi PowerKiKi deleted the batch-cancel branch February 7, 2022 08:54
kamilkisiela pushed a commit to PowerKiKi/apollo-angular that referenced this pull request Feb 16, 2022
This is dependent on apollographql/apollo-client#9248,
so it is not strictly specific to `apollo-angular`, but it seemed important to
ensure that this package does indeed allow to cancel XHR even when batching is
enabled.
PowerKiKi added a commit to PowerKiKi/apollo-angular that referenced this pull request Apr 28, 2022
This is dependent on apollographql/apollo-client#9248,
so it is not strictly specific to `apollo-angular`, but it seemed important to
ensure that this package does indeed allow to cancel XHR even when batching is
enabled.
benjamn added a commit that referenced this pull request Jun 7, 2022
…cel"

This reverts commit cefd24c, reversing
changes made to d98f1de.

I plan to publish this version in the next v3.7.0 beta release, then
immediately reinstate most of this functionality, with a narrower
attempt to solve issue #9773 in a follow-up beta release, to allow folks
to compare the behavior of those two versions.
benjamn added a commit that referenced this pull request Jun 7, 2022
PowerKiKi added a commit to PowerKiKi/apollo-angular that referenced this pull request Sep 6, 2022
This is dependent on apollographql/apollo-client#9248,
so it is not strictly specific to `apollo-angular`, but it seemed important to
ensure that this package does indeed allow to cancel XHR even when batching is
enabled.
PowerKiKi added a commit to PowerKiKi/apollo-angular that referenced this pull request Oct 13, 2022
This is dependent on apollographql/apollo-client#9248,
so it is not strictly specific to `apollo-angular`, but it seemed important to
ensure that this package does indeed allow to cancel XHR even when batching is
enabled.
PowerKiKi added a commit to PowerKiKi/apollo-angular that referenced this pull request Oct 13, 2022
This is dependent on apollographql/apollo-client#9248,
so it is not strictly specific to `apollo-angular`, but it seemed important to
ensure that this package does indeed allow to cancel XHR even when batching is
enabled.
kamilkisiela added a commit to kamilkisiela/apollo-angular that referenced this pull request Oct 24, 2022
This is dependent on apollographql/apollo-client#9248,
so it is not strictly specific to `apollo-angular`, but it seemed important to
ensure that this package does indeed allow to cancel XHR even when batching is
enabled.

Co-authored-by: Kamil Kisiela <kamil.kisiela@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants