[BUG] Store does not emit Error in the second time if SoT is used

[aartikov]

It has already been mentioned here #124 (comment).
stream() does not emit Error for subsequent calls of fresh(): Test for that

The bug happens only when SourceOfTruth is used. Remove SoT in the test, and it will be passed.

[digitalbuddha]

Yup this is the same problem. Thank you for the test.

As Yigit found in the previous report, If a fetcher is non flowable and emits an error, store stops listening to that fetcher and only emits from source of truth going forward. This has been unexpected for multiple users now.

This is one of the areas we should consider using one of the newer shared flow constructs rather than handling ourselves.

Could you give me an idea of what your use case is? Worst case we offer a configuration to return all network errors in the non flow fetcher case.

[yigit]

Looking at this with a fresh mind, I didn't really expect fresh to be used for its side effects. In fact I thought it would get annoying to receive loading/error etc when the stream you've built didn't trigger it.
Seems like the expectation is the opposite and it is handy for all network events to be dispatched to all collectors.
I think we would be OK to allow piggyback for all but i'll try to look deeper.

[yigit]

for the test case, do we instead want assertion to be :

    val expected = listOf<StoreResponse<String>>(
            StoreResponse.Loading(origin = ResponseOrigin.Fetcher),
            StoreResponse.Error.Message(message = "Error", origin = ResponseOrigin.Fetcher),
            StoreResponse.Loading(origin = ResponseOrigin.Fetcher), // for the second refresh
            StoreResponse.Error.Message(message = "Error", origin = ResponseOrigin.Fetcher)
        )

It seems like it would make things more consistent but i'm also worried that this is what i was trying to avoid in the first place, people assuming loading means no data in fact it could come after more data :/.
Granted, right now we synthesize loading hence not adding it is probably easier but if we add error but not loading, it would possibly make things more inconsistent? On the upside, it might be what really is desired from a use case perspective.

[aartikov]

@digitalbuddha

Could you give me an idea of what your use case is?

I am just experimenting with Store and found out this inconsistency. A use case could be quite common: loading some data with pull to refresh and error handling.

@yigit

In fact I thought it would get annoying to receive loading/error etc when the stream you've built didn't trigger it.

I think it depends on application requirements if it should to show loading for requests not triggered directly (from another screen for example). More over, UI could be decomposed to small components where "refresh button" and "loading spinner" are separate components which play together by interacting with the same store.
So, I think Store should emit all events associated with a given key. And it also should provide a way to filter unwanted events.

[digitalbuddha]

Just merged a fix. Please let me know if any issues