Unclear purpose of StoreResponse.NoNewData

[OrhanTozan]

What does it mean when a Store emits StoreResponse.NoNewData? Documentation says only the following:

/**
     * No new data event dispatched by Store to signal the [Fetcher] returned no data (i.e the
     * returned [kotlinx.coroutines.Flow], when collected, was empty).
     */

How is it possible for a Fetcher not to return data? Taking an example from the readme:

StoreBuilder
    .from(
        fetcher = Fetcher.of { api.fetchSubreddit(it, "10").data.children.map(::toPosts) },
        sourceOfTruth = SourceOfTruth.of(
            reader = db.postDao()::loadPosts,
            writer = db.postDao()::insertPosts,
            delete = db.postDao()::clearFeed,
            deleteAll = db.postDao()::clearAllFeeds
        )
    ).build()

when would this Store return a StoreResponse.NoNewData?

[digitalbuddha]

Good call out, the doc can use some adjusting. Here's the issue that lead to the new event. I'll link it in a comment as well https://github.com/dropbox/Store/issues/185

[OrhanTozan]

I've read that issue, and I understand the problem, but I think the solution of the PR that was merged after is a bit rough. These are the two following points that I think should be given a reconsideration (especially now since we are still in alpha):

First of all, StoreResponse.NoNewData has a variable origin: ResponseOrigin property, even though it always comes from the Fetcher (and thus giving it a property would be redundant and make it confusing for users). Now I assume this decision was made, because of the base StoreResponse class having an abstract val origin: ResponseOrigin, but that is just an inheritance problem that could be addressed.

Then second, most importantly; adding StoreResponse.NoNewData assumes that the Store always has a Flow Fetcher. StoreResponse.NoNewData doesn't ever get emitted with a non flow fetcher, so the response type doesn't make much sense in that context. It will result in undesirable code:

when(storeResponse) {
    ...
    is StoreResponse.NoNewData -> throw IllegalStateException()
}

StoreResponse.NoNewData could be seen as a leaking implementation detail.

[eyalgu]

Thanks for engaging with us on how to improve the API!

First of all, StoreResponse.NoNewData has a variable origin: ResponseOrigin

This is indeed unfortunate, but I wanted to minimized the changes to the API and thus decided to leave this in place. For reference Loading can also only be emitted by the fetcher and also has a origin: ResponseOrigin variable so this change did not introduce a new issue.

StoreResponse.NoNewData could be seen as a leaking implementation detail.

We discussed this issue but decided that NoNewData needs to be part of the API because Loading is part of the API. for users of the more general Flow fetcher API, if their flow returns no items they last message they will receive from store would otherwise be Loading.

With that said we probably need to update the API to clarify when NoNewData can be emitted.

[OrhanTozan]

This is indeed unfortunate, but I wanted to minimized the changes to the API and thus decided to leave this in place. For reference Loading can also only be emitted by the fetcher and also has a origin: ResponseOrigin variable so this change did not introduce a new issue. I understand, but perhaps, since we are still in alpha, we could give this an update. We discussed this issue but decided that NoNewData needs to be part of the API because Loading is part of the API. for users of the more general Flow fetcher API, if their flow returns no items they last message they will receive from store would otherwise be Loading. I agree that there must be a solution for the Flow Fetcher problem, but it is an abstraction that needlessly bloats up the StoreResponse for non flow fetchers: ​when(storeResponse) {     ...     is StoreResponse.NoNewData -> throw IllegalStateException() } Coming out of an alpha with code already resulting like this is unfortunate 😕.

In order to keep the API simple for non flow fetchers but tackle the known issue for Flow fetchers, we could perhaps do one of the following:

1. Since it's getting apparent that Non flow fetchers and flow fetchers are too different to abstract them under a same StoreResponse type, we could introduce some new StoreResponses that each Store defines on compile time ahead (so that it is still type safe.
2. The easier way, define the default data:

val store = StoreBuilder .from(Fetcher.ofFlow { api.getArticles() }.onEmpty { emit(emptyList)) }).build()

[OrhanTozan]

On a seperate note: is there a usecase where a flow from an api is empty? It sounds like something one shouldn't have to begin with and an edge cases that has no purpose with a Store/Repository.

[eyalgu]

see #230 for a proposal to simplify the API

[digitalbuddha]

#230 was closed as it changes the paradigm from events to state. I agree that the current solution is not ideal for the reasons @OrhanTozan mentioned. I think we need an internal StoreResponse type that is only used within the store internals rather that emitted to users. Open to other suggestions. Will assign to myself and take on in few weeks unless someone has a better idea for a fix.