apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.38k stars 2.66k forks source link

Allow the removal of multiple optimistics at once #11962

Open jackstudd opened 4 months ago

jackstudd commented 4 months ago

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.

netlify[bot] commented 4 months ago

Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name Link
Latest commit 699ab4c4b508ad50e6977e33b6f7369692630587
changeset-bot[bot] commented 4 months ago

🦋 Changeset detected

Latest commit: 699ab4c4b508ad50e6977e33b6f7369692630587

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

alessbell commented 4 months ago

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 commented 3 months ago

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.