Skip to content
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

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

elliott-with-the-longest-name-on-github
Copy link

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:

const { data, isLoading, ...etc } = $derived(
  createQuery({ queryKey, queryFn })
);

There are several benefits to this:

  • The result can be destructured without losing reactivity
  • It works the way you'd probably expect it to, where any of the input arguments changing updates the query -- no FunctionedParams required
  • It's probably the most-similar API to React's useQuery
  • Super simple implementation

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:

  • Align the package with the standards of the rest of the repo (I've been screwing with the Vite config to set up testing, so I'm sure there's some stuff I haven't fixed yet)
  • Port the React tests (I'm confident I could do this in a day or two, just give me the chance 😉)
  • Gather feedback from the GitHub discussion

Ignore everything under examples and integrations for now, I'm working on getting them up to date

zhihengGet and others added 4 commits February 16, 2025 23:27
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
Copy link

nx-cloud bot commented Mar 25, 2025

View your CI Pipeline Execution ↗ for commit cb173e2.

Command Status Duration Result
nx affected --targets=test:sherif,test:knip,tes... ❌ Failed 4m 19s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 2m 4s View ↗

☁️ Nx Cloud last updated this comment at 2025-04-02 22:00:36 UTC

autofix-ci bot and others added 3 commits March 25, 2025 07:01
…-the-longest-name-on-github/tanstack-query into svelte-5-adapter-derived-by
@niemyjski
Copy link

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...

@elliott-with-the-longest-name-on-github
Copy link
Author

Isn't it a bad thing to have so many $effects?

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 (@tanstack/query-core) without using them.

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...

Basically, you'd just wrap those query exports in $derived.by at the call site and you wouldn't need to pass a function to them for parameters anymore.

const { data, isLoading } = $derived.by(getOrganizationProjectsQuery(...));

@zhihengGet
Copy link

zhihengGet commented Mar 26, 2025

  1. to me, passing function into createQuery feels more natural, solid query also follows the same pattern i believe
  2. easier optimistic updates with $state instead of $derived.by, we can simply mutate the query result .... I know we can assign value to $derived in latest svelte for optimistic update but $state feels easier
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 })
);

@elliott-with-the-longest-name-on-github
  1. easier optimistic updates with $state instead of $derived.by, we can simply mutate the query result .... I know we can assign value to $derived in latest svelte for optimistic update but $state feels easier
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:

  • The current $derived.by design is more efficient than the old one, because the data property being returned is effectively $state.raw -- i.e. not deeply proxified. This means that you can update it by assigning to it at the top level:
    data = { foo: 'bar', ...data } // will work
    data.foo = 'bar' // will not work, data is not a deep proxy
    So maybe it's slightly less convenient in that regard? However, I want to point out, you definitely should not use this strategy for optimistic updates. It is not covered by semver and probably never will be. Your app will probably break unexpectedly at some point because of some patch version that subtly changes how the values are returned, proxied, updated -- whatever. If you need to do optimistic updating, you should use setQueryData, or, if you still want to feel fancy, create your own derived:
    const { data } = $derived.by(createQuery(...))
    const dataIWantToOptimisticallyUpdate = $derived(data.foo.bar)
    
    // later
    dataIWantToOptimisticallyUpdate = 'baz'

@niemyjski
Copy link

Doesn't really matter to me but feels like it's way less idiomatic and I don't like seeing framework code.

@elliott-with-the-longest-name-on-github

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:

  • Not all arguments are reactive (if you're passing in a custom queryClient and that client changes, it's impossible to make createQuery pick it up)
  • You lose destructuring

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 $derived.by -- and the core team is spiritually against passing functions as arguments if at all possible -- but if the community at large really does prefer the other way around, well, we could do that 🤷‍♂️

Either way, the optimistic updates thing above applies -- this shouldn't be implemented with data as $state. It's needlessly inefficient and is bug-prone from the library side.

@arnolicious
Copy link

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 derived (derived.by), like any other derived value.

@niemyjski
Copy link

niemyjski commented Mar 28, 2025

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:

  • Not all arguments are reactive (if you're passing in a custom queryClient and that client changes, it's impossible to make createQuery pick it up)
  • You lose destructuring

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 $derived.by -- and the core team is spiritually against passing functions as arguments if at all possible -- but if the community at large really does prefer the other way around, well, we could do that 🤷‍♂️

Either way, the optimistic updates thing above applies -- this shouldn't be implemented with data as $state. It's needlessly inefficient and is bug-prone from the library side.

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:

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.

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!

@elliott-with-the-longest-name-on-github
Copy link
Author

Having $derived.by at every callsite doesn't feel like just writing javascript (idomatic).

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 queryKey and queryFn can't be updated to new values. So it's a very constrained usecase.

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 })
)

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?

To answer the surface-level question, so long as you're using $derived.by, yes, you can destructure and all of the values will remain in sync exactly as you'd expect them to. To explain why on the deeper level: $derived.by is just kinda awesome. Basically what it says is "give me a function, and I'll rerun it whenever any of its dependencies change". createQuery returns a function (() => CreateQueryResult<>), which means any time the dependencies of that function change, it'll rerun and give you the latest value -- which is then destructured and assigned to the variables you've declared.

And I agree I don't like passing functions either (which seems worse) and not intuitive on what should be a function or not.

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 @tanstack/query-core. The simplest mental model is "Hey, when any of the things this current query depends upon change, I need to create a new query" -- which is exactly what $derived does. (And, for what it's worth, it's still really fast 😁)

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).

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.

Copy link
Member

@lachlancollins lachlancollins left a 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.

@elliott-with-the-longest-name-on-github

I'll re-target the branch -- I had it pointing at main because the diffs were super confusing 😆

@Thiagolino8
Copy link

How will using variables in queryKey work without createQuery accepting a callback?
In the version with the callback it was simple:

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

@hmnd
Copy link

hmnd commented Mar 31, 2025

How will using variables in queryKey work without createQuery accepting a callback?
In the version with the callback it was simple:

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 (() => query), you would use it just as you would with the mainline branch and wrap it in $derived.by:

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
Copy link

Thiagolino8 commented Mar 31, 2025

The problem is not how to use the result of createQuery, but how to make it reactive to the properties of the object it receives as a parameter
Even the svelte compiler warns that this way doesn't work
Here I am using the version from the last commit of this PR
Code_LDYezKJIJF

@hmnd
Copy link

hmnd commented Mar 31, 2025

@Thiagolino8 oh I see... Yeah, that's a problem. You can get around it by passing your own thunk to $derived.by, so you'd end up with $derived.by(() => createQuery({...})), but that doesn't feel very intuitive..

@Thiagolino8
Copy link

You can get around it by passing your own thunk to $derived.by, so you'd end up with $derived.by(() => createQuery({...}))

This would recreate the query on every state change
Definitely not the ideal

@hmnd
Copy link

hmnd commented Mar 31, 2025

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 that this is the way to go for maintaining reactivity.

@jthayworth
Copy link

jthayworth commented Mar 31, 2025 via email

@hmnd
Copy link

hmnd commented Mar 31, 2025

Further, I don't understand the rationale for returning a thunk from createQuery. Personally, I prefer not to destructure the query, but even if a user wants to, they can just wrap it in a plain $derived() and keep their reactivity.

The main change that I believe needs to be made to the original pr is that data is currently being updated even when it hasn't changed, resulting in unnecessary re-renders.

@Thiagolino8
Copy link

Another alternative I can think of would be to support passing a thunk to queryKey and/or values as thunks within the queryKey array

queryKey is not the only property that needs to be reactive, basically all properties that do not accept functions need to be
And in addition, changing the format of the options object would make it incompatible with the queryOptions function

@hmnd
Copy link

hmnd commented Apr 1, 2025

Agreed @Thiagolino8. I think the original svelte 5 pr is the best of the options presented.

@elliott-with-the-longest-name-on-github

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 library

First, @Thiagolino8 is right -- $derived.by wasn't the right choice -- I actually had already fixed this by the time this discussion started, I just hadn't seen it. The current version uses plain old $derived:

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 queryKey or queryFn changes. (More exotically, it should also respond to changes in the second argument, queryProvider, but that's a bleeding-edge case.)

There are basically two options in Svelte:

  • Use some sort of ref (a thunk or a getter, typically)
  • Use $derived

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 createQuery (or the other functions) effectively creates a new query -- even if it doesn't seem that way.

$derived behaves more predictably in cases like these. If an argument changes, you get a new query. (Notably, just switching to $derived fixed a couple of edge-case bugs in the original implementation.) It also significantly decreases the internal complexity.

(For those of you who like to character count, it's actually very close):

$derived()
() => ()

$derived also more clearly communicates what's actually happening with this library, as illustrated by @Thiagolino8:

This would recreate the query on every state change
Definitely not the ideal

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

  • A thunk
  • A box (an object with a getter)
  • Something a little cooler (more on that below)

Thunks initially looked like the best solution because they could be unwrapped by $derived.by, but that doesn't really work -- at least not without game-breaking sacrifices in other areas (reactivity of arguments, gross additional syntax having to pass your own thunks/calling the result, etc).

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 value is a fully-recursively-proxied $state object, when it really doesn't need to be. You now can't use structuredClone (and some other APIs) on the result, and all of the deep properties on, for example, query.data are proxied. This is inefficient and silly, and has the potential to cause a lot of updates that don't need to exist.

Instead, we can do this:

const [value, update] = createRawRef(initialValue)

...where createRawRef is:

/**
 * 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 $state.raw at the top-level fields, which allows us to delegate to @tanstack/query-core for everything under that. Better UX and efficiency!

TL;DR

  • $derived communicates what the library is doing better than other options. After exploring about a million options related to the above ^ with the Svelte Core team, we can't find one that's better all-around
  • We don't need to return thunks anymore!

The main change that I believe needs to be made to the original pr is that data is currently being updated even when it hasn't changed, resulting in unnecessary re-renders.

@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 $effects) -- I'd love you to verify if you had any cases where it was overfiring.

@elliott-with-the-longest-name-on-github

(Additional caveat, I've only added a bunch of tests for createQuery right now, so I'm positive there are more bugs in the other hooks. createQuery is going to be the best "let me try this" hook right now.)

@hmnd
Copy link

hmnd commented Apr 2, 2025

@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)

@lachlancollins
Copy link
Member

lachlancollins commented Apr 2, 2025

@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!

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?

@hmnd
Copy link

hmnd commented Apr 2, 2025

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

@Thiagolino8
Copy link

Thiagolino8 commented Apr 2, 2025

This would recreate the query on every state change
Definitely not the ideal

...except that that's basically what was already happening, you just didn't know it.

@elliott-with-the-longest-name-on-github, Are you sure?
See these different behaviors 👇

Latest commit from this PR:
Code_Na1yJg76mI

NPM version:
msedge_FtuvQaWau1

@hmnd
Copy link

hmnd commented Apr 2, 2025

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 createQuery. We need to be able to maintain a reference to the same CreateQueryResult in order to keep the current data live while fetching new data, and there's no way to do that when createQuery is recreated with every change to its options. I think that's the reason both the solid and angular adapters use this paradigm.

@elliott-with-the-longest-name-on-github

@Thiagolino8 / @hmnd

Ahh rats, I think you're right. I had been basing my tests off of the behavior in the svelte-5-adapter branch. This test I just wrote in particular illustrates the issue:

  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 svelte-5-adapter branch and my branch, it fails with the following states:

[
  { 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 svelte-5-adapter createQuery is only run once, but still produces the broken results. I just assumed it was how the library was supposed to work, but taking a step back, that's obviously wrong. And I can't even divine why the svelte-5-adapter branch would be exhibiting this behavior...

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.

@elliott-with-the-longest-name-on-github

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 😆

@hmnd
Copy link

hmnd commented Apr 3, 2025

@elliott-with-the-longest-name-on-github really appreciate you taking the initiative on this and being so open and responsive with feedback!

@elliott-with-the-longest-name-on-github

Hey, we're all just over here trying to figure out the best way to push bits around 🤷‍♂️

@niemyjski
Copy link

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 😆

Unfortunately we are all in this boat when it comes to tanstack svelte docs / support. Thanks for all of your help.

@elliott-with-the-longest-name-on-github

Just pushed changes:

  • We're back to thunks for params, but are still getting the plain object return values (yay!)
  • The overrendering problems should all be fixed due to the previously-described raw-state fix (if you're curious how this has evolved, read below)
  • The tests for @tanstack/svelte-query-persist-client are all fixed

@lachlancollins lachlancollins changed the base branch from main to svelte-5-adapter April 3, 2025 20:45
@lachlancollins lachlancollins changed the base branch from svelte-5-adapter to main April 3, 2025 20:45
@elliott-with-the-longest-name-on-github
Copy link
Author

Question to everyone (including you, @lachlancollins): Does anyone use notifyOnChangeProps?

I ask because:

  • It's kinda useless in Svelte. I guess it makes the subscriber fire less often, but Svelte isn't going to rerender unless one of the properties you're listening to changes...
  • The implementation is annoying. I can make it do its thing, but it doesn't seem worth it.
  • It's... not even used correctly, anywhere that I can tell? For example, it should be settable on any of the queries in createQueries, but we don't actually respect it at all -- not even in React's useQueries, as far as I can tell. (Edit: Found it, but I think it still has the notifyOnChangeProps issues)

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 notifyOnChangeProps from undefined to something else, it would work, but if you changed it from, say, ['data', 'error'] to just ['data'], it would continue to track error because there's no mechanism for removing previously-tracked props from the #trackedProps registry in QueryObserver -- internally or externally.

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 notifyOnChangeProps changes, nothing will happen. What we'd actually have to do is something like:

$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 notifyOnChangeProps it won't matter 😆

@hmnd
Copy link

hmnd commented Apr 4, 2025

Can confirm the latest commits work great! I personally don't see a reason for notifyOnChangeProps, but I'll leave that call to someone more knowledgeable on that :)

@elliott-with-the-longest-name-on-github
Copy link
Author

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 $state.raw fields", and had to evolve to support @tanstack/query-core's complicated "tracked props" stuff. Basically, we have to lazily access the original object's keys so we don't eagerly track all of them, but still turn them into state. It was an interesting problem to solve, but the solution appears to be both pretty performant and pretty seamless.

/**
 * 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.

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants