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

Complete flow only after cache writes have completed #5877

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

Conversation

rohandhruva
Copy link
Contributor

This is a proposal PR that changes the ApolloCacheInterceptor flows to wait for the cache writes to complete before completing the flow. This is typically only useful when using writeToCacheAsynchronously(true), so that you can get a signal of when the cache write has completed.

Thinking about this a bit more, it gets a bit tricky when you use execute(): singleSuccessOrException uses flow.toList() which will always wait for flow completion by default. With this change, you effectively lose all benefits of asynchronous caching if you are also using execute().

Let me know your thoughts: maybe the execute() limitation means that we don't go ahead with this unless we find some workaround for toList().

Relates to #5852

Copy link

netlify bot commented May 6, 2024

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

Visit the deploys page to approve it

Name Link
🔨 Latest commit 2087ca1

@BoD
Copy link
Contributor

BoD commented May 6, 2024

Thanks for this @rohandhruva !

Indeed as you noticed, the execute() case is problematic because the change would defeat the purpose of writeToCacheAsynchronously. I thought we could maybe get around that by using isLast, but unfortunately we can’t fully rely on it as there can be false negatives.

Long term, maybe Martin’s proposition of exposing events would help as we could emit an event to signal the end of async cache writes.

Back to your original question, your best bet would be to not use writeToCacheAsynchronously in specific cases - but I guess that’s probably what you currently do already?

@rohandhruva rohandhruva force-pushed the rdhruva/flow_wait_for_cache_write branch from 7bec21e to 2087ca1 Compare May 6, 2024 18:06
@rohandhruva
Copy link
Contributor Author

Back to your original question, your best bet would be to not use writeToCacheAsynchronously in specific cases - but I guess that’s probably what you currently do already?

Yes, that's what we do for now, especially for places that are only able to accept a single response.

However, for UIs that are able to use a flow of responses, we would like the ability to get the response but still wait for the cache write, and then do something else in the flow.onComplete { } event with the guarantee that the cache was written to. That was the primary motivation behind this PR :)

I thought we could maybe get around that by using isLast

That would be nice indeed :( Another option is that we can use take(1) for execute(), but we would need to hold the singleSuccessOrException logic somewhere in the stream of the flow, which does sound tricky.

Let me think about this a bit more, and please let me know if you have any other suggestions I can try!

@rohandhruva rohandhruva marked this pull request as draft May 28, 2024 16:58
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