fix(query): create QueryObserver with initial options

[danielr18]

Related Issues

Fixes #1416

Summary

• Create an observer just once for each options.
• Add a hack for keepPreviousData.
• Refactor to be similar to jotai/urql impl.
  • write function no longer returns a promise.

Check List

• yarn run prettier for formatting code and docs

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
jotai	✅ Ready (Inspect)	Visit Preview	Sep 19, 2022 at 1:39AM (UTC)

[codesandbox-ci]

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 b422598:

Sandbox	Source
React	Configuration
React Typescript	Configuration
React Browserify	Configuration
React Snowpack	Configuration
Next.js	Configuration
Next.js with custom Babel config	Configuration
React with custom Babel config	Configuration

[dai-shi]

Our implementation is pretty complicated. (I hope to re-design for better maintainability.)

Having a test is very nice. Please try keeping observerAtom.

[dai-shi]

We can't define a variable here (as it can be global).
Can we have another solution?

One hacky solution is to use WeakMap<QueryClient, QueryObserver>, if we don't find any other methods.

[dai-shi]

On second look, I think we can probably just have this in observerAtom.

      const options =
        typeof createQuery === 'function' ? createQuery(get) : createQuery

[danielr18]

I think I tried this, but this test failed query no-loading with keepPreviousData

const options = typeof createQuery === 'function' ? createQuery(get) : createQuery

[dai-shi]

Oh, no.. Maybe that was the reason.
I'm still not sure if I understand the problem. So, we need to specify queryKey when we first create an observer?

[danielr18]

For all tests to pass, the observer can only be created once per query.

If createQuery is a function, and it's used in observerAtom, then a new observer would be created if createQuery has a dependency that changes.

I'm new to Jotai, so I don't know how we could have it read the options, without tracking its dependencies, so that only one observer is created.

[dai-shi]

once per query

Does it mean if the queryKey is the same? regardless of the other parts of the query.

[danielr18]

So, we need to specify queryKey when we first create an observer?

Yes

[dai-shi]

I think the problem is that the current atomWithQuery allows changing queryKey dynamically.
We probably shouldn't allow it.
That is going to be a breaking change in its API. If we were to add such a change, we have also other things to change.

Please let us know if you find any solution with dynamic queryKey, then we can merge it meanwhile.

[dai-shi]

fa10081 tried to use only queryKey, which doesn't work..

[danielr18]

fa10081 tried to use only queryKey, which doesn't work..

Would it work if queryKeyAtom is wrapped by a selectAtom where the equalityFn always returns true?
I think that would prevent the observerAtom from recomputing.

[dai-shi]

always returns true?

but, then it doesn't work if queryKey changes?

[danielr18]

always returns true?

but, then it doesn't work if queryKey changes?

It would ensure the observer isn't recreated when the query key changes. The new query key would be set regardless in the queryAtom, when observer.setOptions is called.

[dai-shi]

Oh, so observer.setOptions can override queryKey, but it can't be undefined at the first creation?

wrapped by a selectAtom where the equalityFn always returns true?

Can you try this then? If it works, we'll think about some refactors. (basically, we don't use jotai/utils in jotai/query.)

[danielr18]

Oh, so observer.setOptions can override queryKey, but it can't be undefined at the first creation?

wrapped by a selectAtom where the equalityFn always returns true?

Can you try this then? If it works, we'll think about some refactors. (basically, we don't use jotai/utils in jotai/query.)

I've implemented something similar to selectAtom, and tests are passing.

I removed the use of queryClient.defaultQueryOptions because QueryObserver does it internally.

https://github.com/TanStack/query/blob/63932597db10afc1c7c992acd10e677f489d5fcd/packages/query-core/src/queryObserver.ts#L156

[dai-shi]

I've implemented something similar to selectAtom, and tests are passing.

Nice. Please note that refAtom is a very bad practice and we should fix it in the future. So, it's best not to use it outside selectAtom.

Now, if our requirement is not to re-create observer unless queryClient changes, then probably WeakMap cache is a better option than refAtom. Let me try.

I removed the use of queryClient.defaultQueryOptions because QueryObserver does it internally.

Oh, this is great. Thanks.

[dai-shi]

After working on this, I noticed the problem is not about caching observer across options, but just keeping previous data should make more sense.
I'm not 100% confident and happy with the current implementation, but it's simpler and closer to jotai/urql and would be a good path for future refactoring.
There's a little behavioral change, which is more or less a fix in this context.

Thanks for your investigation and work! Without them, this wouldn't have happened.

[dai-shi]

I wasn't be able to solve this. Maybe I'm missing something. Happy if someone tries it.

[dai-shi]

Ah, maybe it's Strict Effect's double invocation.