Apollo-android: How to know Apollo client hitting network or cache?

[mandrizzle]

I want to display a loading state in my activity only if we need to hit the network, if it is from cache I only want to show the final UI. Is there any way to achieve this? If not, thoughts on maybe adding a callback method to the Apollo client called something like onLoadingFromNetwork that can be used to throw up a loading state?

[BenSchwab]

This is not supported out-of-the-box currently. However, the current snapshot contains the concept of configurable fetchers. You could create something like the default CacheFirstFetcher which instead of suppressing a cache miss, calls onNext with a response with null data, or something along the lines.

We could consider adding calling onNext on cache misses as a configuration for the CacheFirstFetcher that we ship with. Thoughts @sav007 ?

[sav007]

Why not just add this on OkHttpClient side? just add custom interceptor and if you hit it then you will definitely go to network

[mandrizzle]

If I did add an interceptor to the OkHttpClient, it seems it would be really complex to match a request from the UI with an intercepted request to see if it hit the network or not and then somehow send it down the ApolloCall.Callback class or through Rx.

I was hoping you would all be open to adding a callback to ApolloCall.Callback, which would be optional, to know if a request hit the network or not. I think it would make showing loading states in apps a lot easier and look nicer. It would also avoid the flash of a loading screen on cached responses.

[sav007]

I agree that flashing progress bar for cached responses is bad UX, but you can fix it by introducing debounce for instance.

My main concern with any new callbacks that we bring more complexity to the library and I feel this kinda callback to track network request is pure application specific logic. I have a reasonable question why we add callback only for network requests but what about reading from SQL cache store that could potentially be a long operation?

How about this, we are generating operation id com.apollographql.apollo.api.Operation#operationId what we can do is attach this id as custom header for OkHttp Request so that in interceptor you can easily identify the http operation request?

[digitalbuddha]

What we are describing is the concept LCE (loading content error) containers as written about in this post. The general theory is that you have a stream that first emits is loading and then emits either error or the content. https://tech.instacart.com/lce-modeling-data-loading-in-rxjava-b798ac98d80

We can think of adding similar down the line as this helps with redux like architecture.

Jake Wharton recently gave a talk about this type of reactive state recently https://youtu.be/0IKHxjkgop4

I'd actually like similar functionality as well where apollo first pushes a result that just says loading and then pushed another result of the actual content when loaded.

[sav007]

I like the idea of LCE, but I see couple concerns from example in post.

1. Java doesn't support algebraic types like Kotlin (sealed classes) or Swift (enum). So in example you must deal with flags (loading, hasError) or with type cast (check instanceof to get the idea what this notification is). Both cases not good.

2. Don't like idea of multiple callback calls, for example our com.apollographql.apollo.ApolloCall.Callback#onResponse doesn't tell user that it will be called multiple times (API of framework is not explicit here, user must read JavaDocs to understand that it can be called more then once).
   But I agree that LCE is a great concept for Rx, especially with Rx2 that was redesigned to provide explicit API of streams that can emit one item or several (Single vs Observable)

So if we really want to follow LCE concept we need to think carefully here, to provide good explicit API. Still open question loading is it for network only? what about SQL cache?

[mandrizzle]

I think if we want to favor a nice explicit API, it would make sense to have callbacks for all potential loading states like onLoadingFromNetwork, onLoadingFromSql, etc

Also I don't think checking flags is a terrible api, we already do that when with the Response to check if the response is an error or not

[sav007]

@digitalbuddha @mandrizzle How about what I suggested to attach custom http header with Operation Id and then you can provide OkHttpClient with interceptor that will notify when request is hit network. We will definitely add such header no matter of what. I know it's not perfect solution but at least for now while we think how proper implement this kind of service notification in ApolloCallback.

[mandrizzle]

The issue I see what that approach is somehow I need to connect the events emitted from the interceptor to the UI callbacks which doesn't sound like a straight forward task since the http client and Apollo client are pretty separated...unless I am missing something and overthinking the complexity?

[sav007]

We generating com.apollographql.apollo.api.Operation#operationId that unique per query. This id will be in this custom header. When you construct Apollo client provide custom OkHttpClient with injected interceptor:

private class TrackingInterceptor implements Interceptor {
    @Override public okhttp3.Response intercept(Chain chain) throws IOException {
      Request request = chain.request();
      // broadcast that operation with operationId going to network 
      Response response = chain.proceed(lastHttRequest);
      // broadcast that operation with operationId finished network
      return response;
    }
  }

Smth like this. Does it make sense?

[digitalbuddha]

I'm not sure why you would need to connect the events to the responses. I like the above interceptor idea since all it takes is something like event bus or rxjava to emit the events back to UI. One of the foundations of our architecture is to embrace an interceptor implementation where possible. I'd be happy to expand the above sample to simple producer consumer implementation that would emit to ui with no additional libraries necessary.

[mandrizzle]

So you guys are suggesting something like:

// My Activity
void getData() {
  mOperationId = someOperation.operationId;
  mInterceptor.listen(this);
  mApolloClient.query(someOperation).enqueue( ... );
}

@Override void onLoadingFromNetwork(String operationId) {
   if (mOperationId.equals(operationId)) showLoading();
}

?

[digitalbuddha]

Yup exactly! We can add a similar example to wiki.

[BenSchwab]

This seems to be a fine temporary solution, but I feel like we should consider this more before deciding not to support this natively / adding to a wiki. Requiring another listener that needs to be deregistered, and bifurcating the listeners that are observing events from a request seem less than ideal.

As Ivan mentioned having some sort of service notification system in the apollo callback seems reasonable.

I also don't think we should overlook the solution I proposed via a custom fetch strategy. As we declare that our Response class can have nullable data, when the cache misses you can just emit a response with null data instead of suppressing the miss, as is our current implementation. This information, combined with the #onComplete callback, and fromCache flags, allows a fairly flexible range of loading state inference, all within our current apollo callback.

[digitalbuddha]

Sounds good to me! Both your solution and the additional callbacks sound reasonable. Do other clients support this? Might be good to touch base.

[mandrizzle]

I wish more clients did, I see a lot of apps on android that have the flashing of a loading screen problem because they always show a loading screen before they request data. We can help developers avoid that issue and maybe we can even set the standard!

[sav007]

Then have next question: should we ditch the sync call? As if we go with LCE pattern ( @BenSchwab what you described it's pretty much what @digitalbuddha proposed with LCE) it's not possible to implement it with sync calls. And I see that our call is no more one shot, it can emit multiple events (in case of CACHE and NETWORK fetch options). So behaviour of sync and async already diverged, and to support both is additional pain plus API no more straight forward with different behaviour, like:

@Nonnull @Override
    public InterceptorResponse intercept(@Nonnull InterceptorRequest request, @Nonnull ApolloInterceptorChain chain)
        throws ApolloException {
      throw new IllegalStateException(CacheAndNetworkFetcher.class.getSimpleName() + "Can only be used asynchronously");
    }

[BenSchwab]

That's a good question. I'm a proponent of simplifying to asynchronous only. However, It would be great to still be able to do a CACHE_ONLY call synchronously, as this makes Apollo a more viable alternative to bundling data. This could be implemented in a separate type, similar to PrefetchCall. Though, the multiple call types is already have significant code duplication.

[sav007]

Closing this discussion as we agreed on implementation plan