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 to clear the cache synchronously #1186

Merged
merged 2 commits into from
May 5, 2020

Conversation

gsabran
Copy link

@gsabran gsabran commented May 4, 2020

I don't really understand why the existing methods have completion blocks while all the work is done synchronously on the caller thread. This gives the impression that the work is being offloaded to another queue which is not the case. I guess the protocol should not be concerned with the existing implementations. That said I've not replaced the existing methods for backward compatibility.

I had a need for synchronous cache clearance to ensure that everything is done right when the user logs out.

@gsabran gsabran force-pushed the gui--clear-cache-sync branch from 30dcd33 to 05db51a Compare May 4, 2020 19:26
@designatednerd
Copy link
Contributor

@gsabran This is probably a couple things:

  1. Something of a vestigial tail from when this returned a Promise
  2. An acknowledgement that not all implementations of NormalizedCache are going to want to have this operation be synchronous. We are using this asynchronously in our BatchedLoadTests, for example, but other client-side implementations may not want this to be sync.

I'm actually kind of surprised to see we're not firing this off to the background for the SQLite implementation, that seems a bit like a better idea, but maybe it's doing that under the hood? I haven't dug deep enough to see quite yet.

@gsabran
Copy link
Author

gsabran commented May 4, 2020

Yes I was surprised as well. I was first synchronously waiting on this call to complete, before realizing that there was no async work done.

@designatednerd
Copy link
Contributor

❌ /Users/distiller/project/Tests/ApolloTests/BatchedLoadTests.swift:6:21: type 'MockBatchedNormalizedCache' does not conform to protocol 'NormalizedCache' - looks like that needs an update.

@gsabran
Copy link
Author

gsabran commented May 4, 2020

Yeah, I'm just confused why that is as I updated this class.

@designatednerd
Copy link
Contributor

Looks like you'll need to try again 🙃

Copy link
Contributor

@designatednerd designatednerd left a comment

Choose a reason for hiding this comment

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

Looks good!

@designatednerd designatednerd merged commit 0bbe3b5 into apollographql:master May 5, 2020
@designatednerd designatednerd added this to the Next Release milestone May 5, 2020
@gsabran
Copy link
Author

gsabran commented May 5, 2020

Thanks!

@@ -32,9 +32,7 @@ public final class InMemoryNormalizedCache: NormalizedCache {

public func clear(callbackQueue: DispatchQueue?,
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should make handler non optional here, otherwise its easy to miss that its asynchronous operation

Copy link
Contributor

Choose a reason for hiding this comment

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

Eh, I can still see a case for people wanting it to fire off in a non-blocking way but not wanting to pass in a handler, I don't think it's worth it.

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.

3 participants