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
[Feature Request] Improved Support for Pull to Refresh #124
Comments
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 |
I actually thought we would write a helper class for this, never get around to finalize it.
The problem with the implementation above is that it will NOT reflect any further local change if the network request for
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 ? |
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... |
Your UI will still know if we add fallback as your client will receive:
do you think it is not enough? |
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 In general I'd like to use a separate stream for handling transient events such as the |
My current solution is to stream With this approach the |
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. |
@digitalbuddha @NahroTo |
I also agree with @NahroTo and @aartikov that I would expect subsequent loading states to be also emitted. Paraphrasing @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 PS @aartikov: In my testing (on 4.0.0-alpha07), I am only missing |
@ghus-raba @digitalbuddha |
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. |
ok I debugged it, what is going on is that it is a one off fetcher so when the first 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:
|
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:
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 I could imagine to extend the 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 |
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 |
|
Do dispatch loading, we probably need to do more because it is mostly synthesized as the initial step. @eyalgu wdyt about always enabling piggyback? |
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 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. |
@eyalgu I think you're coupling Store and UI too tightly with each other now. When I call Coming back at your question:
I think our goal should be the following: Ensure |
Apologies if it came out that way, I was merely trying to understand the usecase for the proposed change.
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. |
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. |
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. |
@eyalgu Here's a usecase: I want my UI to reflect the current state of the data.
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 ( Having multiple StoreResponses like @ghus-raba 's, this issue will be solved:
Edit: I just realised that the original subject of this issue is a good usecase by itself for this also: pull to refresh screens |
I open #230 with a proposal on how to simplify the API.
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). |
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. 👍 |
For visibility, here is a quick and dirty implementation of my recommendation to build this on top of the existing stream: 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. |
Is your feature request related to a problem? Please describe.
The documentation states "Another good use case for
fresh()
is when a user wants to pull to refresh." While this is correct, there is a drawback to usingfresh()
for pull to refresh events: We don't get the Loading or Error states from the StoreResponse. This may mean implementing additional logic around showing the user the loading state or error state in the UI when using bothstream()
, for the primary flow of data, andfresh()
for the pull to refresh event.Describe the solution you'd like
I created the following extensions to be used in a pull refresh scenario and have found they work well. Since pull to refresh is common, perhaps these would be good to integrate into Store somehow?
Examples of the solution in use
Below is an example of how
refreshFlow
is used alongside the main flow:And finally an example of how
collectRefreshFlow
is used in a ViewModel alongside the main flow:Additional context
I feel these two extensions provide more power/flexibility in the pull to refresh scenario than simply calling the suspending
fresh()
method. Does it seem like these or something similar could fit into the Store API?The text was updated successfully, but these errors were encountered: