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

Allow the removal of multiple optimistics at once #11962

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jackstudd
Copy link

In implementing offline mode in an app, we patched Apollo to be able to delete multiple optimistics at once, so that the chain of Layers are recreated only once. Deleting optimistics one by one would result in a spike in RAM & CPU consumption and would end up with a crash of the app, due to the chain of Layers being recreated entirely for each optimistic deletion.

We use things like persisted queue & persisted cache, to provide offline capabilities to the app. To speed up the syncing process, we also batch requests that comes from the queue, and to speed up even further, we don't notify the original operations observables like src/link/batch/batching.ts does, instead we just remove the optimistics. It has proven to work so far! I would love to have your feedbacks on this approach.

Copy link

netlify bot commented Jul 17, 2024

👷 Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 699ab4c

Copy link

changeset-bot bot commented Jul 17, 2024

🦋 Changeset detected

Latest commit: 699ab4c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Patch

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

@jackstudd jackstudd force-pushed the add-batch-remove-optimistics branch from f5c97db to 699ab4c Compare July 17, 2024 11:58
@alessbell
Copy link
Member

Hi @jackstudd 👋

Thanks for the PR and the background you've shared here. Our team has reviewed this and one question that came up is whether the broadcastWatches call of removeOptimistic was part of the bottleneck you're seeing, or if the goal is purely being able to remove several optimistics while creating only a single Layer? We're considering a few different ways of achieving this and any more information you can share will help here :) thanks!

@jackstudd
Copy link
Author

Hi @alessbell 🙋‍♂️

Thanks for your review.

I've tried with and without the broadcastWatches call, and didn't notice any behavioural/performance differences (I didn't measure the performances tho). The main issue was really in in the removeLayer function of Layer, replaying all the layers and recreating the entire chain at each optimistic removal, whether removing all the layers we want and recreating the chain once hugely improved performances.

The goal is purely to be able to remove multiple optimistics without having to replay all the layers. We could also say that the goal is to remove multiple optimistics without affecting performances!

I can also say that, notifying the original observables like src/link/batch/batching.ts does with many observables to notify ended up with the same performances issues as to removing multiple optimistics, but the cause is most likely the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants