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

ApolloClient doesn't use the provided callbackQueue in clearCache. #1900

Closed
iresslerCMM opened this issue Aug 10, 2021 · 2 comments · Fixed by #1904
Closed

ApolloClient doesn't use the provided callbackQueue in clearCache. #1900

iresslerCMM opened this issue Aug 10, 2021 · 2 comments · Fixed by #1904
Assignees

Comments

@iresslerCMM
Copy link
Contributor

iresslerCMM commented Aug 10, 2021

Bug report

The ApolloClient's implementation of clearCache(callbackQueue:completion:) from ApolloClientProtocol does not use the callbackQueue.

It is currently implemented like this:

public func clearCache(callbackQueue: DispatchQueue = .main,
                       completion: ((Result<Void, Error>) -> Void)? = nil) {
  self.store.clearCache(completion: completion)
}

Which drops the callbackQueue without ever using it.

I would expect the implementation to be like this:

public func clearCache(callbackQueue: DispatchQueue = .main,
                       completion: ((Result<Void, Error>) -> Void)? = nil) {
  self.store.clearCache(callbackQueue: callbackQueue, completion: completion)
}

Versions

Please fill in the versions you're currently using:

  • apollo-ios SDK version: 0.45.0 (But wasn't changed in 0.46.0 from what I can see)
  • Xcode version: All
  • Swift version: All
  • Package manager: SPM

Steps to reproduce

Call

ApolloStore.clearCache(callbackQueue:completion:)

with a callbackQueue that is not .main.

Further details

If this is correct and I'm not missing anything I'm happy to submit a PR with my proposed change (or something different if I'm missing a better solution).

@calvincestari
Copy link
Member

@isaacressler - thanks for raising this issue. Your corrected implementation looks correct; if you're happy to submit a PR we can merge it in. I'm trying to figure out how we can get a test in to ensure we've got this covered in the future.

@calvincestari
Copy link
Member

@isaacressler - here's a PR to introduce tests for this. I'll rebase once the fix PR is merged.

@calvincestari calvincestari self-assigned this Aug 11, 2021
@iresslerCMM iresslerCMM changed the title ApolloStore doesn't use the provided callbackQueue in clearCache. ApolloClient doesn't use the provided callbackQueue in clearCache. Aug 11, 2021
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 a pull request may close this issue.

2 participants