separate shouldLoadOnMount and shouldFetchOnMount

Describe the bug

note:
work in progress. im working on a fix, will PR later
so no repro code here

edit:
this may be a race condition
between my persister.restoreClient and QueryObserver.onSubscribe.
all queries should wait for restoreClient

use case:
im fetching immutable data: files from github.
these files are content-addressed via commit hash or blob hash,
so building a cache should be trivial

goal:
fetch every file only once

how:
persistence via indexeddb
query options:

    {
      cacheTime: Infinity,
      staleTime: Infinity,
      refetchOnWindowFocus: false,
      refetchOnMount: false,
      refetchOnReconnect: false,

      // any previous data will be kept when fetching new data because the query key changed.
      //keepPreviousData: true, // ?
    }

but this is not enough to prevent TanStack/query from refetching

executeFetch is still called in QueryObserver.onSubscribe

node_modules/@tanstack/query-core/build/lib/queryObserver.esm.js

class QueryObserver extends Subscribable {
// ...
  onSubscribe() {
    if (this.listeners.length === 1) {
      this.currentQuery.addObserver(this);

      if (shouldFetchOnMount(this.currentQuery, this.options)) {
// FIXME QueryObserver.onSubscribe should not refetch, but use cache
        this.executeFetch();
      }

      this.updateTimers();
    }
  }

when i remove the line this.executeFetch(); it works as expected = dont refetch, use cache

problem: shouldFetchOnMount calls shouldLoadOnMount
but "fetch" and "load" are different actions

function shouldFetchOnMount(query, options) {
  return shouldLoadOnMount(query, options) || query.state.dataUpdatedAt > 0 && shouldFetchOn(query, options, options.refetchOnMount);
}

when i simply remove shouldLoadOnMount(query, options) ||
then my interface is broken, the data is not rendered

related #71 (comment)

Your minimal, reproducible example

http://localhost/

Steps to reproduce

note:
work in progress. im working on a fix, will PR later
so no repro code here

Expected behavior

see above

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

grep /@tanstack pnpm-lock.yaml 

  /@tanstack/query-core/4.13.0:
  /@tanstack/query-persist-client-core/4.13.0:
  /@tanstack/react-query-persist-client/4.13.0:
  /@tanstack/solid-query/4.13.0_solid-js@1.6.0:

react-query version

4.13.0

TypeScript version

No response

Additional context

No response

edit:
this may be a race condition
between my persister.restoreClient and QueryObserver.onSubscribe.
all queries should wait for restoreClient

probably yes ... closing for #3198 (comment)

all queries should wait for restoreClient

they do if you use the PersistQueryClientProvider

see my demo app prosemirror-inplace-editing-demo based on solid.js framework

im wrapping my app in PersistQueryClientProvider
which is defined in tanstack-solid-query-persist-client.jsx
using the same logic as similar components, see "based on"

by fetching from the github api
i get an exact number of the actual fetches in the x-ratelimit-used header

i need patches to

pause the queryClient before restoreClient, so that all queries wait until the persistent cache is loaded. without patching, QueryObserver.onSubscribe calls QueryObserver.executeFetch too early
fix @tanstack+solid-query error DOMException: Failed to execute 'put' on 'IDBObjectStore': [object Array] could not be cloned. because Proxy objects in queryKey cannot be serialized for indexeddb

imho these fix bugs in tanstack, but maybe im doing something wrong...?

pause the queryClient before restoreClient, so that all queries wait until the persistent cache is loaded

this is what the Provider is doing via the isRestoring value that is passed with the IsRestoringProvider:

query/packages/react-query-persist-client/src/PersistQueryClientProvider.tsx

Line 50 in 7741a2f

<IsRestoringProvider value={isRestoring}>{children}</IsRestoringProvider>

Observers are not subscribed while the client is still restoring, so no fetches should go out. We have tests for that :)

thanks for the hint.
i tried to translate this to @tanstack/solid-query but ...

the first fetch works, then result is in cache.
on the next page reload, after persister.restoreClient, it fails to load result from cache

i still dont see how QueryObserver is reading data from the cache
i.e. from queryClient.clientState.queries ... enough for today

demo

edit:
clientState is used in persistQueryClientRestore

trace

query/packages/query-persist-client-core/src/persist.ts

Line 78 in 7741a2f

hydrate(queryClient, persistedClient.clientState, hydrateOptions)

then hydrate restores queries into the queryCache

query/packages/query-core/src/hydration.ts

Line 124 in 7741a2f

const queryCache = client.getQueryCache()

case 1: query is not in queryCache -> add

query/packages/query-core/src/hydration.ts

Lines 153 to 154 in 7741a2f

    
           // Restore query 
        
           queryCache.build(

queryCache.build will notify the query observers (this works for me)

query/packages/query-core/src/queryCache.ts

Lines 118 to 119 in 7741a2f

    
           this.notify({ 
        
             type: 'added',

case 2: query is in queryCache -> update

query/packages/query-core/src/hydration.ts

Line 148 in 7741a2f

query.setState(dehydratedQuery.state)

query/packages/query-core/src/query.ts

Line 216 in 7741a2f

this.dispatch({ type: 'setState', state, setStateOptions })

Query.dispatch will notify observers and cache

query/packages/query-core/src/query.ts

Lines 574 to 578 in 7741a2f

    
           this.observers.forEach((observer) => { 
        
             observer.onQueryUpdate(action) 
        
           }) 
        
           this.cache.notify({ query: this, type: 'updated', action })

observer is created *before* the cache is restored

query/packages/react-query/src/useBaseQuery.ts

Line 75 in 99119a5

new Observer<TQueryFnData, TError, TData, TQueryData, TQueryKey>(

... but observer.subscribe is called *after* the cache is restored

query/packages/react-query/src/useBaseQuery.ts

Lines 86 to 88 in 99119a5

    
           isRestoring 
        
             ? () => undefined 
        
             : observer.subscribe(notifyManager.batchCalls(onStoreChange)),

Navigation Menu

Repository navigation

separate shouldLoadOnMount and shouldFetchOnMount #4368

Describe the bug

Your minimal, reproducible example

Steps to reproduce

Expected behavior

How often does this bug happen?

Screenshots or Videos

Platform

react-query version

TypeScript version

Additional context

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	this.observers.forEach((observer) => {
	observer.onQueryUpdate(action)
	})

	this.cache.notify({ query: this, type: 'updated', action })

	isRestoring
	? () => undefined
	: observer.subscribe(notifyManager.batchCalls(onStoreChange)),

separate shouldLoadOnMount and shouldFetchOnMount #4368

Description

Describe the bug

Your minimal, reproducible example

Steps to reproduce

Expected behavior

How often does this bug happen?

Screenshots or Videos

Platform

react-query version

TypeScript version

Additional context

Activity

milahu commented on Oct 24, 2022

TkDodo commented on Oct 24, 2022

milahu commented on Oct 24, 2022

TkDodo commented on Oct 24, 2022

milahu commented on Oct 24, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions