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

Remove existing interceptors from ApolloClient.Builder before adding new ones #5858

Merged
merged 7 commits into from
May 2, 2024

Conversation

BoD
Copy link
Contributor

@BoD BoD commented Apr 29, 2024

A follow-up to #5857, for .httpCache(), .autoPersistedQueries() and .httpBatching().

@BoD BoD requested a review from martinbonnin as a code owner April 29, 2024 13:40
Copy link

netlify bot commented Apr 29, 2024

Deploy Preview for apollo-android-docs canceled.

Name Link
🔨 Latest commit 16a726d
🔍 Latest deploy log https://app.netlify.com/sites/apollo-android-docs/deploys/662fa77b8f094000089be4ad

Copy link
Contributor

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

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

This goes one step in the good direction but I think this API is still problematic. The fact that for an example, ApolloClient.Builder.autoPersistedQueries() is adding an interceptor under the hood is problematic. Code like this is a code smell.

@martinbonnin
Copy link
Contributor

I'm thinking the order should always be:

  • NormalizedCacheInterceptor
  • ApqInterceptor
  • RetryInterceptor
  • NetworkInterceptor

?

HTTP cache itself is a bit awkward because it spans 2 different domains (GraphQL + HTTP). From a quick cursory glance, it looks like it can be anywhere and is probably not going to disturb other ApolloInterceptors

@BoD
Copy link
Contributor Author

BoD commented Apr 29, 2024

I'm thinking the order should always be:

  • NormalizedCacheInterceptor
  • ApqInterceptor
  • RetryInterceptor
  • NetworkInterceptor

That sounds right to me 👍

@BoD
Copy link
Contributor Author

BoD commented May 2, 2024

This goes one step in the good direction but I think this API is still problematic.

Logged #5874 about this.

@BoD BoD merged commit e7d70c5 into main May 2, 2024
9 checks passed
@BoD BoD deleted the builder-clear-interceptors branch May 2, 2024 09:00
BoD added a commit to BoD/apollo-kotlin that referenced this pull request Jul 1, 2024
…new ones (apollographql#5858)

* Remove existing ApolloInterceptors when calling ApolloClient.Builder.httpCache()

* Remove existing ApolloInterceptors when calling ApolloClient.Builder.autoPersistedQueries()

* Remove existing HttpInterceptors when calling ApolloClient.Builder.httpCache()

* Remove existing HttpInterceptors when calling ApolloClient.Builder.httpBatching()

* Add a test for httpCache

* Don't crash if no `X-APOLLO-SERVED-DATE` header is present

* Revert CachingHttpInterceptor change
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