D-James-GH / cached_query

Simple caching for flutter apps
MIT License
48 stars 10 forks source link

Docs clarification & missing infinite query fetch option case #46

Open iOSonntag opened 1 week ago

iOSonntag commented 1 week ago

Hi There hope your doing well, I have a tiny misleading docs fix to offer:

In the infinite query you have this part:

/// If the fist page has changed then revalidate all pages. Defaults to false.
final bool revalidateAll;

I thought it means when set to false, the query will still keep the old data in the following pages and just wont refetch them. So i set it to true everywhere because I needed to update all old pages (or even better discard them). Later on I realized that setting this to false is actually what I want, that is fetch first page and discard all other pages.

Also note that you abbandonded the logic for check for first page equality and have the following comment in code:

// Note: Removed re-fetching all pages if the first page has changed unless force revalidate is on.
// this was taking too long and didn't seam worth it.

Maybe some renaming would be great here.

The second thing I noticed is that there is a special case that you might want to cover:

Imagine you store your infinite queries on disk, and imagine you have a highscore list with 10.000 users, and imagine that the first 10 players no longer play this game (first page). Now if you refetch the query with forceRevalidateAll = false then the highscore list will never ever be updated because the first page doesnt change so the old data is just returned. The only way to neglect that behaviour is by setting forceRevalidateAll to true. That has the unintended consiquence, that all pages are beeing refetched all the time and every refetch. If the data is stored on disk that means that a single user might. triger 100 requests every time he wants to refetch the highscore list.

There should be an option that clears the infinite query data EVEN if the first page is equal

D-James-GH commented 1 week ago

@iOSonntag Thanks for the feedback. I agree I think there is some cleaning up and enhancements that can be done around the infinite lists, including making the options more concise.

On the second point: this is a difficult issue, so any ideas are welcome. Here is a discussion about the same thing on react query: https://github.com/TanStack/query/discussions/3576

This issue with your example is; it is usually impossible to fetch individual pages without causing inconsistencies in the list. The most common case that we have used an infinite query for is a news feed type thing where new posts are always added to the top, which is why the first page equality is a part of this package (full refetch could always be done with a pull down action + refetch). Always re-setting to the first page only would mean the user looses scroll location. A max stored pages could help, although that would mean that bi-directional infinite query would need to be added incase the user is half way through a list. A shouldRefetchPage call back could also be useful for even more control, although I am not sure how it would be used without testing it out.

Any thoughts are welcome.

iOSonntag commented 1 week ago

Yeah I get why that is tricky, but at least it would be nice to have the option to skip this test:

if (!forceRevalidateAll &&
    _state.data.isNotNullOrEmpty &&
    pageEquality(firstPage, _state.data![0])) {
  // As the first pages are equal assume data hasn't changed
  _onSuccess?.call(firstPage);
  _setState(
    _state.copyWith(
      status: QueryStatus.success,
      timeCreated: DateTime.now(),
    ),
  );
  _emit();
  return;
}

And not beeing forced to enter this test:

if (forceRevalidateAll || revalidateAll) {
  for (int i = 1; i < _state.length; i++) {
    final arg = _getNextArg(newState);
    if (arg == null) {
      break;
    }
    final res = await _queryFn(arg);
    newState = newState.copyWith(
      data: res != null ? [...?newState.data, res] : newState.data,
      lastPage: res,
    );
  }
}

I know for most configurations that would mean resetting scroll position when beeing turned on. But in some cases its actual no big deal. We almost never outdate queries and only refetch them if the user makes a pull to refresh or if some client action was taken that is known to change the data. I know that is not really a nice solution for the entire problem, maybe let me introduce another idea:

Imagine that the query refetch function takes arguments that allow the caller to notify the query what actually caused the refetch for example:

Future<InfiniteQueryState<T>> fetch({
  String? reason
})
...
Future<InfiniteQueryState<T>> _fetchAll({
  String? reason
}) async {
...

The framework might put something in like INITIAL_FETCH / REFETCH_AFTER_BEING_STALE / FETCHING_NEXT_PAGE or whatever you want to call it and the developer might enter on a pull to refresh action GET_LATEST_DATA or whatever he likes.

This would allow for callbacks that can decide based on the loading source what to do.

enum FetchBehaviour
{
  discardAllPreviousData,
  treatFirstPageEqualityAsNoDataChanged
  refetchAllExistingPages
}
...
InfinitQuery(
  behaviourOnFetch: (String? reason)
  {
    if (reason == 'GET_LATEST_DATA') return FetchBehaviour.discardAllPreviousData;

    return null; // Note: null might stand for library defaults
  }
)

If this is paired with reasonable defaults, we have a couple of advantages. First we could change the behaviour of the fetch based on source/reason. Second we could add this information in the state of the query allowing the developer to decide what to display in each situtation to the user.

If the query state loading is true and it is an initial fetch or a pull to refetch i would like to display no items and a loading indicator, if the state is something that only reloads because it is beeing stale, I would like to display the old items and replace them as soon as the query is not loading anymore.

I know that this is not an easy task, as it means changing some core parts of the fetch algorithm but if done right, it woul not introduce any bearking changes and still get all the benefits from it.

It also still means that if no user action is taken and we want to refetch not just the first page some users might refetch their entire page stack all the time but at least for the users who use pull to refresh their stack gets wiped.