feat(svelte-query): Svelte Query Adapter for TanStack Query#4344
feat(svelte-query): Svelte Query Adapter for TanStack Query#4344drejohnson wants to merge 33 commits intoTanStack:mainfrom
Conversation
|
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 bdfeac4:
|
|
please add the basic example to Line 3 in 4ae9956 |
TkDodo
left a comment
There was a problem hiding this comment.
wow, thanks for this ❤️
as you said, tests are a bit missing. Maybe you could try to add the same test suite that we have for react/solid adapters?
DamianOsipiuk
left a comment
There was a problem hiding this comment.
Looks great 🚀
The only thing i would suggest is to rename basic-typescript to basic example so it could be showcased on the https://tanstack.com/query/v4 as embedded sandbox
Codecov ReportBase: 96.36% // Head: 90.72% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #4344 +/- ##
==========================================
- Coverage 96.36% 90.72% -5.64%
==========================================
Files 45 97 +52
Lines 2281 3719 +1438
Branches 640 911 +271
==========================================
+ Hits 2198 3374 +1176
- Misses 80 326 +246
- Partials 3 19 +16 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
SomaticIT
left a comment
There was a problem hiding this comment.
Hello,
Thank you for your amazing work.
I took a little time to review your work and it looks promising.
I added some comments with my first thoughts.
I will try to take more time to help on the migration.
Thanks,
|
|
||
| export function useQueryClient(): QueryClient { | ||
| let queryClient!: QueryClient | ||
| client.subscribe((val) => { |
There was a problem hiding this comment.
We are creating a subscription each time we call this method.
The subscription is never disposed and the queryClient is never unmount.
It could leed to a big memory leak and unexpected behaviors.
In the legacy svelte-query, this is managed by the QueryClientProvider which is bound to a svelte Component.
This way, the lifetime of the queryClient is managed by svelte directly.
There was a problem hiding this comment.
Thanks for the feedback. My initial approach was using a svelte component but I can't get the build system to package .svelte files. I really like using sveltekit's package script. It makes packaging svelte libraries seamless.
There was a problem hiding this comment.
I think the best approach is indeed using a svelte component.
Another approach, maybe easier, could be to use onMount and onDestroy hooks.
However this means that the useQueryClient will need to be called inside a Svelte component.
There was a problem hiding this comment.
Finally, maybe the best solution would be to use getContext and setContext.
In this case:
- we export a method
initQueryClient(options)which allows to create aQueryClientwith custom options and callsetContextto provide theQueryClientto child components. It is also responsible of the management of theQueryClientlifecycle by registering on theonMountandonDestroyhooks to mount/unmount theQueryClient. - we change
useQueryClient()to usegetContextto retrieve the currentQueryClient
This approach has multiple advantages:
- allow to provide custom options
- allow to have multiple
QueryClientwith different options - avoid having a static global
QueryClient - avoid creating "ghost" subscriptions
There was a problem hiding this comment.
On "ghost subscriptions", I've been trying this PR out in one of our projects and found one design decision that we might need to rethink, unless your suggestion solves this as well:
Here's the scenario:
let myQuery = useQuery({enabled: false, queryFn: => (....)}) — let's say queryFn depends on some $reactive variables, so when useQuery is called all of them are undefined, so we disable it by default or else the query would fail the first time. We actually want the query to run each time one of those vars changes as they are probably bound to a user input.
Unlike in a React component, useQuery will only ever be called once when the component is initialized. We could assigned it reactively e.g. $: myQuery = useQuery(...), which would wait until the dependent variables are initialized and it would appear to work, however, the observer and subscriptions would be recreated each time leading to a memory leak.
One workaround would be to change the API so that the query is initialized separately from actually being called:
let myQuery = useQuery({queryFn: ({variables})=> {...}})
$: myQuery.query({variables: {a: $aVar}})This way the queryFn can receive the inputs it needs when they change, triggering a key change and query.
Another idea: make queryFn a store, so any reactive change would trigger a query.
| client.subscribe((val) => { | ||
| queryClient = val | ||
| queryClient.mount() | ||
| return () => { |
There was a problem hiding this comment.
Returning a function in a svelte subscription does nothing.
There was a problem hiding this comment.
According to the docs it does:
The
.subscribemethod must return anunsubscribefunction. Calling an unsubscribe function must stop its subscription, and its corresponding subscription function must not be called again by the store.
There was a problem hiding this comment.
The doc you are mentioning is about the Store contract, it explains how to manually create a store.
The quote says that the .subscribe method an unsubscribe method. It does not mention the callback that is provided to the .subscribe function.
You can try it easily:
import { writable } from "svelte/store";
const store = writable(1);
const unsubscribe = store.subscribe((val) => {
console.log("store value", val);
return () => console.log("unsubscribe callback");
});
console.log("subscribed");
unsubscribe();
console.log("unsubscribed");
/*
console outputs:
store value 1
subscribed
unsubscribed
*/As you can see the unsubscribe callback is never called.
There was a problem hiding this comment.
Interesting. Thanks for testing that out, I'm quite surprised by that behaviour.
| : $result | ||
| }) | ||
|
|
||
| return { subscribe } |
There was a problem hiding this comment.
In SvelteStack/svelte-query, we also have additional methods: setOptions, updateOptions, setEnabled.
These methods were very useful in realworld scenarios.
Is it possible to implement them?
|
This looks great, thanks for making it happen, I'm very excited to use this ❤️ I'd like to suggest an API change to make it feel more Svelte-like. The prefix of In Svelte, when working with data, most of the operations are either setting/getting, either as state or subscribing/updating a store, donated by the For example, the equivalent of One of my favourite examples of this is Urql. The React API uses What do you think? |
|
Sorry guys, I've been on vacation. I'll update the PR sometime this week to get this moving along |
Thanks, I may have to take you up on your offer |
Thanks! Are you familair with rollup? I can't figure out how to package .svelte components. That and writing tests :D |
Yeah, I agree, I initially thought about doing this. |
|
Good changes, I have some feedbacks:
What do you think? |
| export const setQueryClient = (config?: QueryClientConfig): QueryClient => { | ||
| const client = new QueryClient(config) | ||
| setQueryClientContext(client) | ||
| onMount(() => { |
|
Any update on this? |
|
Hi all, I'm wondering if it would be possible to merge this PR without releasing the package on NPM just yet? It seems like the core functionality is all here (@drejohnson has done an amazing job!), but the final work might benefit from allowing others to contribute their own PRs in this repository. I think it would be awesome to have this released close to the SvelteKit 1.0 release (ETA around 5 days), and I'm sure a lot of people trying it out from a React background would be happy to see TanStack Query available! Based on this thread, it seems the final things that are still to-do are fixing some memory leaks, considering renaming exported functions (e.g. useQuery vs createQuery vs queryStore), adding some missing methods, and more tests. |
|
@lachlancollins we could also make PRs to this PR ? we could merge the PR and not release it, but any further PR that gets merged to main that has a So if you want to get this merged, please contribute to this PR. |
|
Hello! Is it possible to have a list of the specific tasks that are needed to complete for this PR to be merged? I see there are 3 todo items in the description but I would like more information so I can contribute if possible. Thank you! |
|
This PR is stalled a bit, but we'd really like to get it over the finish line. I honestly don't know what is missing to get that done, mainly, the question would be if the author @drejohnson is still around to finish it, or if someone else would pick it up? What we would need is someone to own the svelte-query adapter and who's willing to stick around to maintain it after the first release. We're also working on v5 already and I wouldn't want to merge this to |
Hi @TkDodo, thanks for your reply - when I want to merge my changes into this current PR, do I need to make a new PR into |
|
@lachlancollins depends on what is easier for you. If you think it's starting from scratch, I'll close this PR and you can copy the commits over to a new PR. If you want to make a PR to this PR that's also fine for me. |
|
Im willing to contribute in any way possible as well. Let me know if you want to divide and conquer this @lachlancollins |
|
superseded by #4768 |
This pull request adds Svelte adapter for TanStack Query.
Primitives included in the package are:
A few examples have been included as well as a few initial tests for
useQueryTODO
Acknowledgment
Thanks to: