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

[svelte] keepPreviousData not working #4851

Closed
chrislcs opened this issue Jan 22, 2023 · 16 comments
Closed

[svelte] keepPreviousData not working #4851

chrislcs opened this issue Jan 22, 2023 · 16 comments
Assignees

Comments

@chrislcs
Copy link

Describe the bug

The keepPreviousData options does not seem to work in the svelte-query. Even when set to true the data is empty until the new query is done.

I made the query reactive using the documentation here: https://tanstack.com/query/v4/docs/svelte/reactivity

I think it is because the current syntax will rerun the createQuery method and create a new query object each time any used variable changes, instead of subscribing to change within the already created query object, but I do not know enough about the inner workings of query-core and svelte-query to know if this is indeed an issue.

Your minimal, reproducible example

https://codesandbox.io/s/nifty-cloud-pxwjho?file=/App.svelte

Steps to reproduce

  1. Create a reactive query with a queryKey with a variable and keepPreviousData set to true
  2. Render the results to the DOM
  3. Update the variable
  4. All results are removed immediately removed from the DOM
  5. And rendered again when the new query result is available

Expected behavior

As a user, I expected the previous data be visible in the DOM during fetching of the updated data

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

  • OS: Windows
  • Browsers: Chrome 109.0, Firefox 109.0

TanStack Query version

4.22.3

TypeScript version

4.9.4

Additional context

No response

@lachlancollins
Copy link
Member

Interesting - I think solid-query gets around this by defining the inline options object as reactive, while in svelte you need to define the whole line as reactive.

I wonder if defining the options before calling createQuery fixes this issue? So it would look like $: options = {...}, then createQuery(options). I'll try this in the morning :)

@lachlancollins lachlancollins self-assigned this Jan 25, 2023
@chrislcs
Copy link
Author

I tried that and it didn't work for me. I had a deeper look into this and, among other things, checked how tanstack/table handles it and they seem to use a writable store for the entire options object. I think this would also be a good idea for svelte-query. It would look something like this:

const options = writable({
  queryKey: ['test', [1]],
  queryFn: async ({ queryKey }) => {
    await sleep(100)
    return queryKey[1]
  },
  keepPreviousData: true,
})
const query = createQuery(options)

Within svelte files you can then simply update the options like so:

$options.queryKey[1] = [1, 2]

And outside of svelte files:

options.update((o) => ({ ...o, queryKey: ['test', [1, 2]] }))

If you do not need the reactivity you can still use a regular options object.

I made a PoC with a test that was failing before and working with these changes. I do not know query-core well though, so I'm not sure all changes are completely correct. You can check this here: main...chrislcs:query:main

@lachlancollins
Copy link
Member

lachlancollins commented Jan 25, 2023

This looks great :) The only bit that I'm a bit concerned about is forcing the types on parseQueryArgs, as this would also need to be done for the other functions that use createBaseQuery and have more complex types. However in v5, there will be no passQueryArgs, so maybe this could be a v5 feature - do you want to submit a PR into the v5 branch?

EDIT: I've also just checked this never worked with the old package either.

@chrislcs
Copy link
Author

chrislcs commented Jan 25, 2023

Yeah forcing the types isn't pretty, was just meant as a PoC. With the current implementation of parseQueryArgs:

  if (!isQueryKey(arg1)) {
    return arg1 as TOptions
  }

  if (typeof arg2 === 'function') {
    return { ...arg3, queryKey: arg1, queryFn: arg2 } as TOptions
  }

  return { ...arg2, queryKey: arg1 } as TOptions

it shouldn't cause any problem since called like createQuery(writableOptions) will return on the first condition, createQuery('test', writableOptions) will pass the first and second condition correctly (since a writable is an object, not a function) and createQuery('test', () => 1, writableOptions) will correctly return on the second condition. It'd be fairly easy to copy over this utility function to svelte-query and make a typesafe version of it.

EDIT: Scratch that. Now that I thought about it more, it wouldn't work if the writeableOptions are not the first argument. We'd need to update the writable with the other arguments if it isn't passed as the first argument. Still not too difficult, but more substantial. Worth it to do that or just go for v5?

I can make a PR tomorrow 👍

@lachlancollins
Copy link
Member

EDIT: Scratch that. Now that I thought about it more, it wouldn't work if the writeableOptions are not the first argument. We'd need to update the writable with the other arguments if it isn't passed as the first argument. Still not too difficult, but more substantial. Worth it to do that or just go for v5?

I think probably just go for v5, thanks for figuring this out! Also I doubt this would fix the problem since createQuery still gets re-run, but keepPreviousData will be merged into placeholderData in v5: #4715

@chrislcs
Copy link
Author

chrislcs commented Feb 2, 2023

Sorry for the sudden lack of activity from my side, I caught the flu and was unable to work on this for a while. I hope to make the PR soon.

@tmarnet
Copy link

tmarnet commented Mar 14, 2023

How would you make the query reactive to external variables (as opposed to user events such as the on:input from the doc) - like the URL's search params (e.g. $page.url.searchParams in SvelteKit) - while keeping the behaviour of keepPreviousData ?

By following the new reactivity documentation the only thing that works for me is the following, but that doesn't feel very intuitive:

  const ids = $page.url.searchParams.getAll(name)

  const queryOptions = writable({
    queryKey: ['reactive', ...ids],
    queryFn() {
      return fetchWithIds(ids)
    },
    placeholderData: keepPreviousData,
  })

  const query = createQuery(queryOptions)

  $: {
    const nextIds = $page.url.searchParams.getAll(name)

    $queryOptions = {
      queryKey: ['reactive', ...nextIds],
      queryFn() {
        return fetchWithIds(nextIds)
      },
      placeholderData: keepPreviousData,
    }
  }

Am I missing something ?

Thanks !

@chrislcs
Copy link
Author

chrislcs commented Mar 14, 2023

You should use the provided queryKey in the query function. This way you do not have to update the query function.

Something like this (quickly wrote this, not tested):

  const ids = $page.url.searchParams.getAll(name)

  const queryOptions = writable({
    queryKey: ['reactive', ids],
    queryFn: ({ queryKey }) => fetchWithIds(queryKey[1]),
    placeholderData: keepPreviousData,
  })

  const query = createQuery(queryOptions)

  $: {
    const nextIds = $page.url.searchParams.getAll(name).map(Number)
    $queryOptions.queryKey[1] = nextIds
  }

@tmarnet
Copy link

tmarnet commented Mar 14, 2023

While this is indeed a bit shorter, it still doesn't feel as right as leveraging reactive statements $: like before - but I guess we don't really have a choice when it comes to using keepPreviousData.

Nonetheless, this now raises the question of typings since we don't benefit from autocompletion in writable() and the queryKey (amongst other things) raises an error:

Binding element 'queryKey' implicitly has an 'any' type. ts(7031)

Any idea of to make it work seamlessly with TS ?

@chrislcs
Copy link
Author

I either do not understand what you mean by it not feeling right, or I do not agree.

Anyhow, you can type the writable options as Writable<CreateQueryOptions>, as in:

  const queryOptions: Writable<CreateQueryOptions> = writable({
    queryKey: ['reactive', ids],
    queryFn: ({ queryKey }) => fetchWithIds(queryKey[1]),
    placeholderData: keepPreviousData,
  })

Or with typescript 4.9 or higher you might be able to do this for narrower types (didn't test it tho):

  const queryOptions = writable({
    queryKey: ['reactive', ids],
    queryFn: ({ queryKey }) => fetchWithIds(queryKey[1]),
    placeholderData: keepPreviousData,
  }) satisfies Writable<CreateQueryOptions>

@ferntheplant
Copy link

ferntheplant commented Jun 8, 2023

I'm running into the same issue on 4.29.11 and v5.0.0-alpha.56 even when I use the writable store for my query options

@ap0nia
Copy link
Contributor

ap0nia commented Jun 8, 2023

It's only available on the alpha version. The main difference is that createBaseQuery always converts the options to a store in order to handle any case.
https://github.com/TanStack/query/blob/alpha/packages/svelte-query/src/createBaseQuery.ts

@ferntheplant
Copy link

Sorry snuck an edit in right before your commented didn't think I'd get a reply so fast 😅

I tried on the alpha release and still am getting the issue with the query state changing to loading when fetching the same query with a new query key. Also now I'm getting a type error that keepPreviousData isn't even an option anymore

@ap0nia
Copy link
Contributor

ap0nia commented Jun 8, 2023

They changed the API for this on the alpha version. You should import the keepPreviousData function provided by the alpha library, and set it as the placeholderData option.

import { createQuery, keepPreviousData } from '@tanstack/svelte-query'

createQuery({
  placeholderData: keepPreviousData
})

Some of the previous replies demonstrated this new usage:
#4851 (comment)

@lachlancollins
Copy link
Member

@tmarnet you can now use derived stores with createQuery in v5 alpha - you can see an example here: https://tanstack.com/query/v5/docs/svelte/reactivity

@HugeLetters
Copy link

import { createQuery, keepPreviousData } from '@tanstack/svelte-query'; 
// "@tanstack/svelte-query": "5.0.0-beta.20"
let post: string;
$: query = createQuery({
	queryKey: [post],
	queryFn: () => getPosts(post),
	placeholderData: keepPreviousData
});

Doesn't work for me still, shows undefined on post updates - is it because query is recreated on every post change? What's the recommended pattern here? Turn post into a writeable and turn query into a const? Seems a bit excessive.

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

No branches or pull requests

6 participants