[Feature Request] Improved Support for Pull to Refresh about store

[ferinagy]

Thanks for the tips for the helper @yigit (both #124 (comment) and #124 (comment)). I checked it out and found a few drawbacks in these implementations:

1. It only notifies of the subsequent Loading if it was triggered via refresh(), but not via underlying store
2. It only supports requests with refresh == true -> not too hard to add by changing to fun stream(refresh: Boolean): Flow<StoreResponse<Key, Value>
3. It repeats cached value after refresh, as you mentioned -> I managed to filter the subsequent caches out, but the initial code is a bit messy (probably could be improved)
4. It only supports a single key -> it is probably enough for the pull-to-refresh scenario, but it would be nice to also support stream of states per key (which is already supported by store.stream(...))
5. Not entirely sure, but from the looks of it, I would say, that there might be some issues when subscribing to the stream after the loading started, but before the data is delivered.

That being said, such helper class would be helpful, but I would prefer it to be provided by this library as opposed to trying to write it myself in every project.

---

On the other hand, I still think it would be easier if the stream(...) would also deliver subsequent Loading states and we could just filter them out if not interested instead of this dance with a trigger + flatMap + throwing away the stream every time there is a refresh.

I could imagine to extend the StoreResponse to distinguish reloading and error after data was already delivered:

sealed class StoreResponse<out T> {
    object Loading : StoreResponse<Nothing>
    data class Data(val value: T, origin: ResponseOrigin) : StoreResponse<T>
    data class Error(val error: Throwable, origin: ResponseOrigin) : StoreResponse<T>

    data class LoadingWithPreviousData(val previousData: Data<T>) : StoreResponse<T> 
    data class ErrorWithPreviousData(val error: Throwable, origin: ResponseOrigin, val previousData: Data<T>) : StoreResponse<T>
}

The last 2 states would be useful to eg. show list with data with a loading indicator and show list with snackbar error respectively. If the stream(...) would deliver something like this, we could also show list with loading indicator after subscribing when a request is already in flight, as we would be receiving LoadingWithPreviousData.

[ferinagy]

...the caller is already aware of the change to the viewModel and we don't need to be the source of truth for this change. If there's a usecase I'm missing here please let me know.

I think it is useful to show in the UI that data is being updated even if it is not triggered by the user on the very same screen. It can be triggered on the previous screen, or user could do a pull to refresh then exit the screen and then come back. In both of these cases, I would like to tell the user: "Here is some data, but I am working on getting you a fresh set".

In these cases, storing the refresh state in viewmodel tied to a screen would not be enough. We could create a helper that would wrap a store (and outlive a single screen), that would keep the state of loading/error, but like I said, i think it is not so trivial and could get out of sync with the store. On the other hand, the store already has knowledge of when a fetch is happening (as well as errors). It would be IMO much simpler for developer to simply filter/ignore the states in the stream than try to inject them themselves.

If we decide not to extend the stream, then I would at least opt to include such helper class wrapping the stream in the library.

[OrhanTozan]

I think introducing a new API for this is a bandage fix. I think its commonly expected that the .fresh() method itself should trigger the Store emitting a StoreResponse.Loading.

[ferinagy]

BTW if viewmodel is supposed to keep track of the loading state, then I fail to see the point in supporting even the first load as a state in the stream.

[digitalbuddha]

Does look like a nice api. I'd love to hear other's thoughts. Since this is additive maybe we should have a recipe? This would give us more flexibility with not having to support this api forever while still sharing with others

[yigit]

I actually thought we would write a helper class for this, never get around to finalize it.
The thing i was thinking was more like a helper object that encapsulates the behavior.
Something like this:

class UIRequest(
    val key: Key
) {
   private val requests = ConflatedBroadcastChannel<StoreRequest>(StoreRequest.cached(key = key, refresh = true))
   fun refresh() {
      requests.offer(StoreRequest.fresh(key))
   }
  val stream : Flow<StoreResponse<Key, Value> = refreshRequest.asFlow().flatMapLatest {
       store.stream(it)
  }
}

The problem with the implementation above is that it will NOT reflect any further local change if the network request for refresh fails which is not great. But if desired (usually is), it can be handled w/ a little modification in the steam

class UIRequest(
    val key: Key
) {
   private val requests = ConflatedBroadcastChannel<Boolean>(true)
   fun refresh() {
      requests.offer(true)
   }
  val stream : Flow<StoreResponse<Key, Value> = refreshRequest.asFlow().flatMapLatest {
       store.stream(StoreRequest.cached(key, refresh = it))
  }
}

Now this one has the downside of re-emitting the latest value. Right now, if a fresh request's network request fails, we don't emit any further values. We might either make a flag for that (fallbackToDiskOnError) which might actually make sense (thinking out loud here), or we can change that helper to handle that and re-connect a cached read if a Error is dispatched.

Thoughts ?

[ychescale9]

Right now, if a fresh request's network request fails, we don't emit any further values. We might either make a flag for that (fallbackToDiskOnError)

Was running into this exact issue yesterday when trying to implement an extension that supports conditionally using fresh() / cached() request based on a lambda user passes in.

I’d love to have a fallbackToDiskOnError flag for the fresh() request, although I’d also like the UI to be notified of the failed fetch after displaying the fallback data from cache e.g. showing a snackbar. Perhaps in that case it can emit a Data response with cached data, followed by an Error response with origin = Fetcher, then continue to stream changes from sourceOfTruth? Or can we model this with a single Error response with the data field populated with cache fallback? Neither feels intuitive with the current StoreResponse API though...

[yigit]

Your UI will still know if we add fallback as your client will receive:

StoreResponse.Loading(origin = Fetcher...)
StoreResponse.Error(origin = Fetcher...)
// maybe cache, i'm not sure but should probably also include cache, almost definitely :dancer: 
StoreResponse.Data(origin = Persister) 

do you think it is not enough?

[ychescale9]

It could work for "swipe to refresh" when the UI is already displaying data.

For my case though I want to start streaming with a StoreRequest.fresh(fallbackToDiskOnError = true) at the start. Problem with the Loading, Error, Data sequence is when I receive the Error I don't know whether an incoming Data(origin = Persister will be emitted immediately after it so I don't know whether to show a prominent error state (with a retry button in the middle), or display a transient error message (snackbar) and wait for the upcoming data from persister.

In general I'd like to use a separate stream for handling transient events such as the Error(origin = Fetcher) in this case instead of turning it into a permanent "Error" state which will be cached and re-emitted as the latest state after config change. This is why I don't like the Loading, Error, Data sequence as we'll need to keep track of the previous emission to decide what the right current state should be.

[ychescale9]

My current solution is to stream StoreRequest.cached(key, refresh = true) as the main flow, and use suspend fun Store<Key, Output>.fresh(key: Key) to trigger a fetch for swipe to refresh and rely on the multicaster to emit new data through the main flow.

With this approach the InFlight / Error states need to be managed manually in the ViewModel / StateMachine beyond the initial request, but this is not that big of a deal when you already need to do this anyway to merge multiple streams of data sources into UI states.

[aartikov]

@digitalbuddha @NahroTo
I was surprised when discovered that stream() does not emit Loading and Error for subsequent calls of fresh(). Is not it more handy behaviour?

[ferinagy]

I also agree with @NahroTo and @aartikov that I would expect subsequent loading states to be also emitted. Paraphrasing StreamWithoutSourceOfTruthTest, I would expect:

    @Test
    fun streamWithoutMultipleLoads() = testScope.runBlockingTest {
        val fetcher = FakeFetcher(3 to "three-1", 3 to "three-2")

        val pipeline = StoreBuilder.from(fetcher).scope(testScope).build()

        val twoItemsNoRefresh = async {
            pipeline.stream(
                StoreRequest.cached(3, refresh = false)
            ).take(4).toList()
        }

        delay(1_000) // make sure the async block starts first
        pipeline.fresh(3)

        assertThat(twoItemsNoRefresh.await()).containsExactly(
            StoreResponse.Loading(origin = ResponseOrigin.Fetcher),
            StoreResponse.Data(
                value = "three-1",
                origin = ResponseOrigin.Fetcher
            ),
            StoreResponse.Loading(origin = ResponseOrigin.Fetcher), // this is not present currently
            StoreResponse.Data(
                value = "three-2",
                origin = ResponseOrigin.Fetcher
            )
        )
    }

Currently there is no 2nd Loading in the stream.

PS @aartikov: In my testing (on 4.0.0-alpha07), I am only missing Loading state. So if there is an error, with 2 fetch calls, I see a stream of Loading, Exception, Exception

[aartikov]

@ghus-raba
After some experiments I found out, that second Error is missing when a store has SourceOfTruth.
Here is my test https://gist.github.com/aartikov/34f1413c9bb8d3b548730b668dbfbd5d

@digitalbuddha
Is it an expected behaviour?

[yigit]

hmm that does look like a bug but need to debug further why the second error is not showing, it should show up in the stream.

[yigit]

ok I debugged it, what is going on is that it is a one off fetcher so when the first stream receives an error, it does not have any reason to keep listening for more, it falls back to Source of truth.
We enable piggyback on server responses only if there is no source of truth. I'm not sharp enough to remember why we did it that way.
https://github.com/dropbox/Store/blob/main/store/src/main/java/com/dropbox/android/external/store4/impl/FetcherController.kt#L63

I've tried always enabling it which does fix this test and does not fail any other test but i'm not 100% sure why we did it this way so not feeling very comfortable to make the change. (also, you still won't get another loading)

The right solution might be just creating the helper class that'll do this properly (a stream of refresh triggers).

Something like:

val refreshTrigger = MutableStateFlow(0)
val data = refreshTrigger.flatMap {
    store.stream(...)
}
fun refresh() = refreshTrigger.value += 1

[OrhanTozan]

I think @ghus-raba 's solution is nicely intuitive, that's what I found others (including myself) do in the past for repositories. I think it keeps the users away of managing the loading state on their own, and keeping the Store as the source of truth.

Quick note: I would rename Loading to something like InitialLoading.

[eyalgu]

We enable piggyback on server responses only if there is no source of truth. I'm not sharp enough to remember why we did it that way.
IIRC piggypack was added to mimc SoT behavior of pushing down subsequent updates for the non SoT case. we never intended it to be used with SoT originally.

[yigit]

Do dispatch loading, we probably need to do more because it is mostly synthesized as the initial step.
That being said, I'm not fully sure if we want to send loading to existing observers. In the UI, unless triggered by the user, it might be very unintiutive. Granted, developer can always filter the following ones out so may not be that big of a deal either.

@eyalgu wdyt about always enabling piggyback?
One issue is though, right now, an existing stream consider Disk as origin for any followup data change. If we make it piggy back, it may result in sending more data w/ origin->network, or an expectation of it which might be tricky but should be do-able.

[eyalgu]

Taking a step back what is our goal here? to allow delivery of multiple errors to a stream? to allow multiple deliveries of loading/noNewData to a stream? both?

As you said I'm not sure we should be delivering multiple Loading/NoNewData events to collectors. The usefulness of this seems limited, as you would usually only want the UI to show loading state if it is either the first load (which we support), or if the user triggered the action (e.g pull to refresh). in the latter the caller is already aware of the change to the viewModel and we don't need to be the source of truth for this change. If there's a usecase I'm missing here please let me know.

As for multiple Error state - what's the usecase fo delivering multiple errors to a collector? If the idea is to show a toast in the UI then I think this is actually similar to the case above - When the repository/presenter/viewmodel (choose your poison) detects an error it will decide if this is retrieable error, if so it will just call stream again and will be able to get the next error. The only thing that piggybacking can get us here is to be able to react to errors that resulted from an unrelated stream operation and I'm not sure I see what's the usecase here.

Personally, I'm not a fan of enabling piggyback for SoT cases because it can cause more memory pressure than necessary and will make the streams more complex.

[OrhanTozan]

@eyalgu I think you're coupling Store and UI too tightly with each other now.

When I call store.stream().collect { state -> }, the state always reflects the current state of the data. Keeping it this way prevents having to do your own subsequent imperative calls after. I think the code is more declarative and easier to reason about when we keep it: "data goes down (store.stream { data -> }), events go up (store.refresh())".

Coming back at your question:

Taking a step back what is our goal here?

I think our goal should be the following: Ensure store.stream() correctly reflects the current state at all times.

[eyalgu]

@eyalgu I think you're coupling Store and UI too tightly with each other now.

Apologies if it came out that way, I was merely trying to understand the usecase for the proposed change.

When I call store.stream().collect { state -> }, the state always reflects the current state of the data. Keeping it this way prevents having to do your own subsequent imperative calls after. I think the code is more declarative and easier to reason about when we keep it: "data goes down (store.stream { data -> }), events go up (store.refresh())".

Coming back at your question:

Taking a step back what is our goal here?

I think our goal should be the following: Ensure store.stream() correctly reflects the current state at all times.

Can you give an example of the usecase you have in mind. A real life example would be helpful in evaluating the need for this change.

[OrhanTozan]

@eyalgu Here's a usecase:
Let's say I have a Calendar feature in my app. In the Calendar overview all of the Calendar events get shown (ala Planning view of Google Calendar).
CalendarEventStore<Unit, List<CalendarEvent>>.

I want my UI to reflect the current state of the data.
store.stream{}
So, when I'm initially visiting the screen for the first time, I'm probably just seeing an empty screen with a loading spinner in the middle

store.stream { state ->
    when(state) {
        is State.Loading -> {
           // just the spinner is shown
        }
        is State.Data -> {
           // events are shown
        }
    }
}

This is already possible with the current version of Store, right now Store is holding all of the state! But now I want to be able to manually refresh the calendar with a refresh button (Store.fresh(key)) . When I press the refresh button, a inderminate horizontal progress bar shows up at the top of the screen, but in the meanwhile I can still scroll through my existing items.
Now if you look at the code above, refreshing the calendar means that all of the events will dissapear and the spinner will just show..

Having multiple StoreResponses like @ghus-raba 's, this issue will be solved:

    object Loading : StoreResponse<Nothing>
    data class Data(val value: T, origin: ResponseOrigin) : StoreResponse<T>
    data class Error(val error: Throwable, origin: ResponseOrigin) : StoreResponse<T>

    data class LoadingWithPreviousData(val previousData: Data<T>) : StoreResponse<T> 
    data class ErrorWithPreviousData(val error: Throwable, origin: ResponseOrigin, val previousData: Data<T>) : StoreResponse<T>
}

Edit: I just realised that the original subject of this issue is a good usecase by itself for this also: pull to refresh screens

[eyalgu]

I open #230 with a proposal on how to simplify the API.

BTW if viewmodel is supposed to keep track of the loading state, then I fail to see the point in supporting even the first load as a state in the stream.

That's a good question - I think the only reason for the current support for Loading/NoNewData is to know when the fetch you trigged has completed (e.g to show a spinner at the bottom of your screen when showing stale/partial data).

[ferinagy]

Some of the alpha feedback we've gotten is that there are too many states to handle under StoreResponse.

I believe the feedback was actually exactly opposite. 😄

But on a more serious note, I think I like the proposal of #230, at least at first sight. It leaves some state to be kept in viewmodel, but makes the api simpler and provides necessary data to implement the pull to refresh and similar scenarios. 👍

[yigit]

For visibility, here is a quick and dirty implementation of my recommendation to build this on top of the existing stream:

#230 (comment)

To be clear, this allows implementing refresh while keeping data etc but it still won't dispatch loading events from unrelated streams because i'm not a big fan of that change.