feat(solid-query-persist-client): Solid.js implementation of client persistence

[aadito123]

Implemented the query-persist-client-core for Solid.js.

Also made changes to the base solid-query package so that isRestoring can be respected.

Translated the tests from the React implementation, and they all pass.

Addresses the following: #5463

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
query	⬜️ Ignored (Inspect)	Visit Preview		Aug 29, 2023 1:40pm

[codesandbox-ci]

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 3c0d1c0:

Sandbox	Source
@tanstack/query-example-react-basic-typescript	Configuration
@tanstack/query-example-solid-basic-typescript	Configuration
@tanstack/query-example-svelte-basic	Configuration
@tanstack/query-example-vue-basic	Configuration

[aadito123]

I updated the tests to Vitest, something that I missed. I can't get two tests to pass, but I will work on them tomorrow.

[aadito123]

@TkDodo @ardeora The only test that does not pass is the persistence being maintained upon the QueryClient itself changing. I can't get this to work without changing the QueryClientProvider to take a signal, which may not be ideal since it would change the external API a bit. Essentially it would look like this:

const [client, setClient] = createSignal(new QueryClient());
<QueryClientProvider client={client}>
  {props.children}
</QueryClientProvider>

// calling useQueryClient returns an Accessor
const client = useQueryClient();
client()...

Otherwise, I would appreciate any feedback on the PR. Thanks!

[TkDodo]

awesome work, thank you. please look at the failed formatting, and let's wait for @ardeora to review

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 3c0d1c0. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

🟥 Failed Commands
nx affected --targets=test:eslint,test:lib,test:types,test:build,test:bundle

---

Sent with 💌 from NxCloud.

[ardeora]

@TkDodo @aadito123

Thank you so much for working on this! I am currently in the middle of moving to a new city so I would be unavailable for a review till this Sunday 😬

Hope that's not an issue. If it's something urgent I can take a look at it tomorrow or day after 😅

[aadito123]

@ardeora No worries. Just pushed the formatting fix. Thanks!

[ardeora]

@aadito123 Awesome work! It looks like the query can hydrate data from persisters but it doesn't do a background re-fetch after hydration. Any idea why? I'm taking a closer look to see why that happens!

[aadito123]

@ardeora Odd. I assumed the refetch() in the isRestoring computed would cause the background fetch. Perhaps the computed should also include an explicit refetch call to the query client when hydration is complete. I will try it in a bit once I'm at my computer.

Edit: also noticed that tests that were passing are now failing

[aadito123]

Took me a while to figure out where the refetch wasn't happening but it should be okay now. @ardeora Let me know if it works now.

[aadito123]

@ardeora Seems the only test failing is should be able to persist into multiple clients. I tried to make createBaseQuery reactive to the change in client however, memoising the observer breaks the transition case. I am not entirely sure why, but in the createResource fetcher, the observer signal returns undefined when there is an ongoing transition. Performing an early return on such a case, means that the observer's listener never subscribes, and hence, the query state is not updated.

This is all ignoring the fact that the useQueryClient does not return a signal, and hence, reactive changes cannot be made to a change in the query client. If I make the API change to return a signal from useQueryClient, then the test passes. Let me know, if that would be an acceptable API change.

Edit: Nevermind, I can circumvent the undefined issue with createMemo by directly setting the Observer signal in a createComputed. However, I did use the changed API where useQueryClient returns a signal. Also, if we are okay with this change, I would be happy to write the docs for it as well.

[ardeora]

Oh wow! This is so strange I never noticed before. Apparently, Context.Provider works differently than other JSX values. When you pass in the value to Context.Provider the value itself is never reactive unless you use a Signal or reactive wrapper. Reproduction below:

https://playground.solidjs.com/anonymous/35ce36ae-e070-4cc5-b2cd-0b876e934271

Hmm 🤔 I'm not sure how I feel about useQueryClient returning a Signal because this would be a massive breaking change but this is the only way to maintain reactivity of the QueryClient. @TkDodo any advice if we can introduce breaking changes in v5 beta? Or is it too late now?

Also not sure why would we ever want to switch queryClients during runtime? Is there a specific use case that I'm not understanding?

[aadito123]

Should also note that this would be a breaking change for only the solid-query package of the v5 beta. I could go either way on this matter because I don't personally rely on the query client being reactive, however, this is something that react-query does do, so for the sake of feature parity, it might be worthwhile.

Only use case I've seen for runtime query client changes has been to maintain different caches for clients with different cache settings. For example, client 1 might hold data in the cache for a day, whereas client 2 would hold it for an hour. This way, you can switch options without having to throw away the cache for either.

[ardeora]

@aadito123 I think I have a way to preserve backwards compat and make queryClient reactive without breaking changes. How about we preserve reactivity inside the implementation detail?

https://playground.solidjs.com/anonymous/90507ec4-3060-40b5-8cdd-514d20107fed
Here is an example I'm talking about

This way

const queryClient = useQueryClient()
// queryClient will still be the actual query client

In the above approach the queryClient will still be queryClient object but it wont be reactive. Similar to how

const [count, setCount] = createSignal(20)

const countCopy = count()

countCopy will not be reactive

If you want to get a queryClient that can switch during runtime. You can track it in a reactive scope and it will work like so

const reactiveClient = createMemo(() => useQueryClient())

createEffect(() => {
   console.log('Will run everytime queryClient reference changes')
   console.log('Client Changed:', reactiveClient())
})

Does this make sense? This change can be adopted for this PR too. Let me know what are your thoughts on it!

[aadito123]

@ardeora Makes perfect sense, I should have thought about that. The reactivity is a bit hidden outside the library but I am sure documentation can cover that. I made the change and it still passes the tests.

[aadito123]

So weird. The same tests are failing on the base beta branch too.

[TkDodo]

So weird. The same tests are failing on the base beta branch too.

sadly, some tests are flaky. I think the react-query ones only run because of the change in the devtools-core package. You can ignore it for now

[ardeora]

This is looking great! Thank you so much for working on this 😄 ❤️ Do you have a twitter handle? Would you mind if we give you a shout out on twitter?

[aadito123]

@ardeora My handle is @SwagMaster19 but I'm not all that active. Would be happy with the shout-out, nonetheless. Thanks!