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

Add ApolloStore.close() and NormalizedCache.close() #6299

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

Conversation

martinbonnin
Copy link
Contributor

@martinbonnin martinbonnin commented Dec 3, 2024

@martinbonnin martinbonnin requested a review from BoD as a code owner December 3, 2024 12:40
@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Dec 3, 2024

✅ Docs Preview Ready

No new or changed pages found.

Comment on lines +151 to +156
/**
* Closes resources associated with the cache if any.
* This function must not call `nextCache.close()`, this is done by the caller.
*/
override fun close() {
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with SqlNormalizedCache.close() and not SqlNormalizedCacheFactory.close() because I think it makes more sense? Ultimately, SqlNormalizedCacheFactory may be turned into a plain Kotlin function like we have () -> OkHttpClient for lazy initialization, we could have () -> NormalizedCache, this is the same pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's more intuitive than a closeable factory 👍

Now if a driver is passed, or a factory is re-used, apolloStore.close() must be called with caution. Should we call that out, maybe in the KDoc of the SqlNormalizedCacheFactory(driver: SqlDriver) constructor?

Comment on lines +152 to +153
* Closes resources associated with the cache if any.
* This function must not call `nextCache.close()`, this is done by the caller.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's very easy to forget calling nextCache.close() so I went with this but no strong opinion there.

Copy link
Contributor

@BoD BoD left a comment

Choose a reason for hiding this comment

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

👍

@BoD
Copy link
Contributor

BoD commented Dec 13, 2024

Related: #3564

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