fix(query): subscribe instead of fetch if initialData #586
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pmndrs/jotai/45V3uyoqf5pbyiV4LG2wkzEz3iqv |
aulneau
force-pushed
the
fix/query-no-need-to-fetch
branch
from
4d92961 to
e8310d0
Compare
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit ab36eaa:
|
src/query/atomWithQuery.ts
Outdated
| .fetchOptimistic(defaultedOptions) | ||
| .then(listener) | ||
| .catch((error) => listener({ error })) | ||
| if (!defaultedOptions.initialData) { |
There was a problem hiding this comment.
More precisely, you want to check if getInitialData() returns non-falsy value. Like, options.initialData = () => null. We may drop the support of it. Just needs to make it consistent.
There was a problem hiding this comment.
Sure, that is a good catch. Do you think this would be sufficient?
| if (!defaultedOptions.initialData) { | |
| if (!getInitialData()) { |
There was a problem hiding this comment.
I think we should call getInitialData() just once in our code.
| @@ -301,5 +302,7 @@ it('query with initialData test', async () => { | |||
|
|
|||
| // NOTE: the atom never suspends | |||
| await findByText('count: 0') | |||
| expect(mockFetch).toHaveBeenCalledTimes(0) | |||
There was a problem hiding this comment.
@dai-shi does this work for you with regards to testing? I can't think immediately a way to create a failing test
There was a problem hiding this comment.
Do you mean this doesn't fail before the fix?
There was a problem hiding this comment.
ah i see what you mean, yes it does fail:
| observer | ||
| .fetchOptimistic(defaultedOptions) | ||
| .then(listener) | ||
| .catch((error) => listener({ error })) |
There was a problem hiding this comment.
@dai-shi I don't think we need this bit of code either way it seems -- can you tell me more about it?
Because we are subscribing on mount, the subscription kicks off the fetch initially. Seems like we're doing double work, unless I am missing something
There was a problem hiding this comment.
When an atom suspends, the component suspends and it will not be mounted, which means onMount will not be called. Unless we resolve the promise, the component will not be mounted.
While doing some more testing, I noticed that on mount a query would automatically refetch even though I had provided
initialDatato it. I could not stop this behavior -- based off of reading https://react-query.tanstack.com/guides/initial-query-data#_top, it is the case that initialData will be treated as fresh until the staleTime has been met, or based on other user options passed (meaning a fetch should not happen until it become stale, and there have been some refetch options passed)Because we were automatically running the fetchOptimistic method regardless of if initialData was passed, we were causing an unneeded fetch to happen. React query will pick up and take over and do all the fetching if needed when there is initialData. We are already subscribing on mount, so there is no need to fetch during the initial get.