fix: support atom scope outside core #589
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pmndrs/jotai/7S9VaUrKtTwJubPCE8tJxHLsHNkj |
|
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 eb1471b:
|
src/query/atomWithQuery.ts
Outdated
| equalityFn: (a: TData, b: TData) => boolean = Object.is, | ||
| getQueryClient: (get: Getter) => QueryClient = (get) => get(queryClientAtom) |
There was a problem hiding this comment.
I know there is a preference to avoid objects being passed, but I think this will result in a lot of atomWithQuery(createQuery, null, (get) => get(myQueryClientAtom)), as I think passing a custom query client will be used more often than an equalityFn. and maybe passing null is ok, too :)
There was a problem hiding this comment.
I see your point and yes to the preference.
I'm not sure how often people use myQueryClientAtom in app code. Unlike jotai/urql which require "URL", the default client in jotai/query would work for most cases?
We still can use <Provider initialValues={[[queryClientAtom, myQueryClient]]}>.
To allow null for equalityFn, we can't use default parameter, right? we can change that.
There was a problem hiding this comment.
We still can use <Provider initialValues={[[queryClientAtom, myQueryClient]]}>.
If we did this though, it would not allow you to modify the scope, right?
I think folks might use the custom query client often, esp if they have an app that is using react-query already as they might have defined defaults or other settings they want to use.
There was a problem hiding this comment.
Yes, for the custom atom scope, we need to use the third argument.
Do you think it's a good idea to swap the second and third arguments? (=breaking change)
There was a problem hiding this comment.
yeah, I do. I'm honestly not sure how much use or importance the equalityFn provides -- i find modifying the query client to only update on certain changes is more effective https://react-query.tanstack.com/guides/migrating-to-react-query-3#the-queryoptionsnotifyonstatuschange-option-has-been-superceded-by-the-new-notifyonchangeprops-and-notifyonchangepropsexclusions-options
There was a problem hiding this comment.
If we can remove equalityFn completely, it seems better to me. Is it possible?
An internal discussion with @aulneau revealed that many of utils/integrations don't support atom scope properly, especially when we create internal atoms in a function.
This PR addresses the issue. Note this affects only the case using
scope, which is mainly for library usage.