-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Svelte 5 API proposal #8852
base: main
Are you sure you want to change the base?
feat: Svelte 5 API proposal #8852
Conversation
docs(svelte-5-adapter): force runes mode for all examples (TanStack#7800) * docs(examples): force runes mode for all examples * Fix simple example mounting * Fix type feat(svelte-5-adapter): require options to be passed as function (TanStack#7804) * fix(svelte-5-adapter): Require options to be passed as function * More fixes * More fixes * Update examples fix(svelte-5-adapter): function for `createMutation` options (TanStack#7805) * fix(svelte-5-adapter): require function for createMutation options * Fix type errors refactor(svelte-5-adapter): simplify createBaseQuery (TanStack#7808) * refactor: simplify createBaseQuery * Don't take snapshot of queryKey chore(svelte-5-adapter): tidy-up functions (TanStack#7809) * chore(svelte-5-adapter): tidy-up functions * Fix types * Update svelte Fix import extensions chore: ignore state_snapshot_uncloneable in tests Fix merge
View your CI Pipeline Execution ↗ for commit cb173e2.
☁️ Nx Cloud last updated this comment at |
…-the-longest-name-on-github/tanstack-query into svelte-5-adapter-derived-by
Isn't it a bad thing to have so many $effects? Also how does this work for export scenarios like this where all of my queries are exported https://github.com/exceptionless/Exceptionless/blob/main/src/Exceptionless.Web/ClientApp/src/lib/features/projects/api.svelte.ts currently feels weird and I've been trying to rework params... |
Can you expand upon what you mean? There aren't many effects here, right? Effects in and of themselves aren't bad things, they just become bad things if misused. In this case, there's really no way to synchronize state with an external system (
Basically, you'd just wrap those query exports in const { data, isLoading } = $derived.by(getOrganizationProjectsQuery(...)); |
const data = createQuery(()=>{ ... }) //passing function and directly mutate data for optimistic updates, not sure if it is the best practice tho
const { data, isLoading, ...etc } = $derived.by( // if u want to spread it... then you need $derived.by, but i prefer not doing it
data
); const { data, isLoading, ...etc } = $derived.by( // you can't directly mutate data I believe ? in recent version, we can assign stuff to $derived for optimistic updates but mutate $state feels better? subjective
createQuery({ queryKey, queryFn })
); |
As far as optimistic updates go:
|
Doesn't really matter to me but feels like it's way less idiomatic and I don't like seeing framework code. |
I'm curious -- what idioms are you referring to? The alternative API is basically what we had before: const query = createQuery(() => ({ queryKey, queryFn })) Downsides being:
As far as idioms go, you basically need a thunk on the way in (to preserve argument reactivity) or on the way out (to preserve return value reactivity) -- but not both. Svelte is designed to handle thunk returns gracefully with Either way, the optimistic updates thing above applies -- this shouldn't be implemented with |
I find the example from proposal very elegant 👍 Avoiding the need to use functions for the arguments is amazing, and since queries do depend on outside variables (like the query key) it does make perfect sense that you'd need to wrap it in a |
Having $derived.by at every callsite doesn't feel like just writing javascript (idomatic). And I agree I don't like passing functions either (which seems worse) and not intuitive on what should be a function or not. I have not been destructuring at all here because I haven't seen anyone else do it for tanstack nor the docs (unless they've been updated). I'm always concerned about broken references e.g., isSuccess (computed) being updated properly when destructured? I agree with this as well:
On final note, I'm fine with what ever design but for the sake of porting docs / and samples would be great to have the least amount of changes as possible as svelte doesn't get much love from tanstack docs/community updating those (And I get I could contribute there as well, time is very very limited). Thanks again! |
I just want to point out that "every callsite" is not technically correct, though I expect what you mean is probably true. This would absolutely work: const query = createQuery({ queryKey, queryFn }) ...you just can't destructure it, and More practically, though, you definitely don't need it when creating your own queries: function createUserQuery({ id }) {
return createQuery({
queryKey: ['user', id],
queryFn: ({ queryKey }) => fetch(`/api/users/${queryKey[1]}`).then(r => r.json)
})
} ...you would just need it where you actually use the query: const { data, isLoading } = $derived.by(
createUserQuery({ id })
)
To answer the surface-level question, so long as you're using
This has been the beast I've had to struggle with, especially the intuitiveness part. Maybe for some insanely-bleeding-edge, maximum-possible-performance-needed scenarios it makes sense to fully take advantage of fine-grained reactivity by passing individual getters or functions across the hook boundary, but Tanstack Query is not that use case, especially since all of its state is already externally hosted in
Yeah definitely, this won't go live without updated docs and examples. I'm actually working on porting all of the React tests as well, as the current coverage is pretty abysmal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for your work @elliott-with-the-longest-name-on-github ! After a fair bit of thinking, I agree this syntax is the more svelte v5-appropriate choice. @arnolicious 's comment sums it up well:
Avoiding the need to use functions for the arguments is amazing, and since queries do depend on outside variables (like the query key) it does make perfect sense that you'd need to wrap it in a
derived
(derived.by
), like any other derived value.
Giving a big 👍 to keep working on this!
Also I think it would be more appropriate for the PR to target the TanStack:svelte-5-adapter
branch, so we're comparing the two runes implementations.
I'll re-target the branch -- I had it pointing at |
How will using variables in queryKey work without createQuery accepting a callback? let search = $state('')
const query = createQuery(() => ({
queryKey: ['products', search],
queryFn: async () => {
const data = await fetch(`https://dummyjson.com/products/search?q=${search}`).then((r) => r.json())
return data.products
}) But in this version I couldn't make it work |
Since createQuery now returns a thunk ( const query = $derived.by(createQuery({
queryKey: ['products', search],
queryFn: async () => {
const data = await fetch(`https://dummyjson.com/products/search?q=${search}`).then((r) => r.json())
return data.products
})); |
@Thiagolino8 oh I see... Yeah, that's a problem. You can get around it by passing your own thunk to |
This would recreate the query on every state change |
In this case, I feel like the original solution of passing the query options as a thunk might be more ideal. Another alternative I can think of would be to support passing a thunk to Given that the solid adapter already sets a precedent for passing a thunk to |
I agree. The original solution is a much better DX than the derived one. It
also wouldn’t be an unprecedented API from a Tanstack Query perspective as
the Angular variant also requires a thunk.
…On Mon, Mar 31, 2025 at 7:28 PM David ***@***.***> wrote:
In this case, I feel like the original solution of passing the query
options as a thunk might be more ideal.
Another alternative I can think of would be to support passing a thunk to
queryKey and/or values as thunks within the queryKey array. But then
using the values in queryFn feels sort of disjointed.
Given that the solid adapter already sets a precedent for passing a thunk
to createQuery, I feel that might be the superior solution after all.
There is also precedent among Svelte maintainers
<sveltejs/svelte#12800 (comment)>
that this is the way to go for maintaining reactivity.
—
Reply to this email directly, view it on GitHub
<#8852 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AY6KRUCZRJ7FNNC62PQ44S32XHFRFAVCNFSM6AAAAABZWVCG4SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONRXGY2DEOBWGQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
[image: hmnd]*hmnd* left a comment (TanStack/query#8852)
<#8852 (comment)>
In this case, I feel like the original solution of passing the query
options as a thunk might be more ideal.
Another alternative I can think of would be to support passing a thunk to
queryKey and/or values as thunks within the queryKey array. But then
using the values in queryFn feels sort of disjointed.
Given that the solid adapter already sets a precedent for passing a thunk
to createQuery, I feel that might be the superior solution after all.
There is also precedent among Svelte maintainers
<sveltejs/svelte#12800 (comment)>
that this is the way to go for maintaining reactivity.
—
Reply to this email directly, view it on GitHub
<#8852 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AY6KRUCZRJ7FNNC62PQ44S32XHFRFAVCNFSM6AAAAABZWVCG4SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONRXGY2DEOBWGQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Further, I don't understand the rationale for returning a thunk from The main change that I believe needs to be made to the original pr is that |
queryKey is not the only property that needs to be reactive, basically all properties that do not accept functions need to be |
Agreed @Thiagolino8. I think the original svelte 5 pr is the best of the options presented. |
Sorry, I hadn't seen all of these comments! I'm going to pick and choose some quotes to respond to with the hope of having a good "catch all" response, but if I miss something, please point it out -- I'm not intentionally ignoring any points :) Updates since you've last seen the libraryFirst, @Thiagolino8 is right -- const { data, isLoading } = $derived(
createQuery({ queryKey, queryFn })
); When working with a library like this, there are two core problems when using signal-based reactivity: 1: Getting reactivity into the library The library needs to react to changes made to its arguments. In the above example, that means when There are basically two options in Svelte:
The main advantage of refs is that you can be fine-grained with them (so that when one thing changes, everything else isn't invalidated). That advantage does not apply to this library. When one argument changes, all of them have to be reapplied. Basically, changing an argument to
(For those of you who like to character count, it's actually very close):
...except that that's basically what was already happening, you just didn't know it. Now you do, and I didn't even have to tell you! 😉 2: Getting reactivity out of the library The stuff the library returns needs to be reactive, for obvious reasons. In this case, there's really only one solution: Refs. Here are the options:
Thunks initially looked like the best solution because they could be unwrapped by Boxes are just kinda gross. You'd have to do something like this: const query = $derived(createQuery(...))
query.current.data
query.current.isLoading No one wants to do that. There is a cooler option when we're not working with primitive data types. Objects and arrays are, themselves, references, so if we can return an object whose identity never changes, but whose values are updated, we can maintain reactivity! This is basically what the old version of the Svelte 5 adapter was doing, but it had some significant downsides. Basically, the pattern was this: const value = $state(initialValue);
function update(newValue) {
Object.assign(value, newValue);
}
// use update to subscribe The downside here is that Instead, we can do this: const [value, update] = createRawRef(initialValue) ...where /**
* Makes all of the top-level keys of an object into $state.raw fields whose initial values
* are the same as in the original object. Does not mutate the original object. Provides an `update`
* function that _can_ (but does not have to be) be used to replace all of the object's top-level keys
* with the values of the new object, while maintaining the original root object's reference.
*/
export function createRawRef<T extends {} | Array<unknown>>(
init: T,
): [T, (newValue: T) => void] {
const out = (Array.isArray(init) ? [] : {}) as T
function update(newValue: T) {
Object.assign(out, newValue)
}
for (const [key, value] of Object.entries(init)) {
let state = $state.raw(value)
Object.defineProperty(out, key, {
enumerable: true,
get: () => state,
set: (v) => {
state = v
},
})
}
return [out, update]
} Which means the return value of our hooks can now be just the regular object (not a thunk), but it's only TL;DR
@hmnd I think the current iteration should fix this (my tests so far are showing only the minimal required number of updates when checking in |
(Additional caveat, I've only added a bunch of tests for |
@lachlancollins could we possibly get a pr.new url for this pr too? Not sure if tanstack still uses that service, but would be very helpful intstead of linking locally and building myself! (sorry for the ping, I'm just not sure whether it's possible for me to set up something similar from my end) |
Unfortunately it needs to pass CI checks to release the preview, so for now, linking is the best option! EDIT: It seems like the CI is failing due to a broken lockfile, @elliott-with-the-longest-name-on-github could you update it? |
Oh yeah, I had to deal with that too. I've published it from my own fork of the fork for now (but given the lockfile is the only holdup, I'd recommend others just wait for that fix in the original fork): https://pkg.pr.new/hmnd/tanstack-query-svelte5/@tanstack/svelte-query@2 |
@elliott-with-the-longest-name-on-github, Are you sure? |
Hmm yeah I've played with the latest changes a bit and it really still doesn't match the ergonomics of passing a thunk to |
Ahh rats, I think you're right. I had been basing my tests off of the behavior in the it(
'keeps up-to-date with query key changes',
withEffectRoot(async () => {
let search = $state('')
const states: Array<CreateQueryResult<string>> = []
const query = createQuery(
() => ({
queryKey: ['products', search],
queryFn: async () => Promise.resolve(search),
}),
queryClient,
)
$effect(() => {
states.push({ ...query })
})
await vi.waitFor(() => expect(query.data).toBe(''))
search = 'phone'
await vi.waitFor(() => expect(query.data).toBe('phone'))
console.log(
states.map((s) => ({
data: s.data,
status: s.status,
fetchStatus: s.fetchStatus,
})),
)
expect(states.length).toBe(4)
expect(states[0]).toMatchObject({
status: 'pending',
fetchStatus: 'fetching',
data: undefined,
})
expect(states[1]).toMatchObject({
status: 'success',
fetchStatus: 'idle',
data: '',
})
expect(states[2]).toMatchObject({
status: 'success',
fetchStatus: 'fetching',
data: '',
})
expect(states[1]).toMatchObject({
status: 'success',
fetchStatus: 'idle',
data: 'phone',
})
}),
) On both the [
{ data: undefined, status: 'pending', fetchStatus: 'fetching' },
{ data: '', status: 'success', fetchStatus: 'idle' },
{ data: undefined, status: 'pending', fetchStatus: 'fetching' },
{ data: 'phone', status: 'success', fetchStatus: 'idle' }
] Which tbh is quite baffling; in So TL;DR I think it's likely you're right Theo -- we probably should use a thunk for params to preserve the observer instance between changes -- but I need to track down why the functioned version isn't working. |
Figured it out. Thanks for bearing with, all -- I'm struggling to piece together a full understanding of everything from a combination of the React docs, the Svelte docs, and the core code 😆 |
@elliott-with-the-longest-name-on-github really appreciate you taking the initiative on this and being so open and responsive with feedback! |
Hey, we're all just over here trying to figure out the best way to push bits around 🤷♂️ |
Unfortunately we are all in this boat when it comes to tanstack svelte docs / support. Thanks for all of your help. |
Just pushed changes:
|
Question to everyone (including you, @lachlancollins): Does anyone use I ask because:
TL;DR it seems like useless API surface area we could omit from the Svelte library unless we really need to keep it? Edit: I'm not even sure the internal implementation is correct? I think if you changed the value of In Svelte, for example, because the whole hook doesn't rerun anytime something changes, this pretends to work but doesn't really: return !resolvedOptions.notifyOnChangeProps
? observer.trackResult(query)
: query Because it only runs when the hook is set up -- so if $effect(() => {
if (resolvedOptions.notifyOnChangeProps) {
observer.clearTrackedProps();
resolvedOptions.trackedProps.forEach((prop) => observer.trackProp(prop))
} else {
// I'm not even sure it's possible to do this in a valid way, as you'd need to know which props are _currently_ being accessed, even though you never set up the listeners beforehand.
}
}) From a very technical standpoint, we can just pretend this issue doesn't exist -- as long as you don't change the value of |
Can confirm the latest commits work great! I personally don't see a reason for |
For anyone who's technically inclined to care, the "one-level-deep raw state" implementation has evolved! This has turned into a really neat pattern for integrating Svelte with js-only packages. It started as "create a proxy from an initial object that turns any fields assigned to it into /**
* Makes all of the top-level keys of an object into $state.raw fields whose initial values
* are the same as in the original object. Does not mutate the original object. Provides an `update`
* function that _can_ (but does not have to be) be used to replace all of the object's top-level keys
* with the values of the new object, while maintaining the original root object's reference.
*/
export function createRawRef<T extends {} | Array<unknown>>(
init: T,
): [Readonly<T>, (newValue: T) => void] {
const refObj = (Array.isArray(init) ? [] : {}) as T
const out = new Proxy(refObj, {
set(target, prop, value, receiver) {
if (prop in target) {
return Reflect.set(target, prop, value, receiver)
}
let state = $state.raw(value)
Object.defineProperty(target, prop, {
configurable: true,
enumerable: true,
get: () => {
// If this is a lazy value, we need to call it.
// We can't do something like typeof state === 'function'
// because the value could actually be a function that we don't want to call.
return isBranded(state) ? state() : state
},
set: (v) => {
state = v
},
})
return true
},
})
function update(newValue: T) {
const existingKeys = Object.keys(out)
const newKeys = Object.keys(newValue)
const keysToRemove = existingKeys.filter((key) => !newKeys.includes(key))
for (const key of keysToRemove) {
// @ts-expect-error
delete out[key]
}
for (const key of newKeys) {
// @ts-expect-error
// This craziness is required because Tanstack Query defines getters for all of the keys on the object.
// These getters track property access, so if we access all of them here, we'll end up tracking everything.
// So we wrap the property access in a special function that we can identify later to lazily access the value.
// (See above)
out[key] = brand(() => newValue[key])
}
}
// we can't pass `init` directly into the proxy because it'll never set the state fields
// (because (prop in target) will always be true)
update(init)
return [out, update]
}
const lazyBrand = Symbol('LazyValue')
type Branded<T> = T & { [lazyBrand]: true }
function brand<T extends () => unknown>(fn: T): Branded<T> {
// @ts-expect-error
fn[lazyBrand] = true
return fn as Branded<T>
}
function isBranded<T>(fn: T): fn is Branded<T> {
return Boolean((fn as Branded<T>)[lazyBrand])
} |
'should be able to set different stale times for a query', | ||
async () => { | ||
/** | ||
* TODO: There's a super weird bug with this test, and I think it's caused by a race condition in query-core. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lachlancollins here's the mystery of the day, for which I have no solution. The only way to make it pass that I can find is to add a tight loop in query-core
that delays execution of the rest of updateResult
-- so I'm pretty confident this is a race condition -- I just can't for the life of me figure out where. I'm suspicious it's in query-core
but I can't prove it...
This is a proposal built on the excellent work of @lachlancollins. I've had a few conversations with Rich and the Svelte team about what the best user API is for libraries like this, and we think we've come up with a good solution. Basically, it's this:
There are several benefits to this:
FunctionedParams
requireduseQuery
The main drawback is that, if the arguments change,
createQuery
has to be rerun, meaning the observer is recreated. Given that previously all of the observer's arguments had to be reapplied any time any one of them changed anyway, and the rate of change of arguments is typically slow (queryKey
isn't going to be updating hundreds of times per second, just... every once in a while), this tradeoff seems very worth it -- in fact, I haven't been able to observe any significant difference in performance.TODO:
Ignore everything under
examples
andintegrations
for now, I'm working on getting them up to date