Make persistent cache work with Raycast environment

[thomaspaulmann]

I made a few adjustments to make the persistent cache work with the Raycast JS environment, which is build on top of Node.js.

I did the following:

• Remove the check if window is available in the persistQueryClient function. Raycast doesn't have a window because it renders the component tree natively. Unsure why the check was there but authors can still check in their codebase for the existence of window. This might be a breaking change but the package is still experimental. I think that's acceptable.
• I exported some interfaces to make it easier to construct type-safe an AsyncStorage

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tanstack/react-query/FxNjesgCiXxFnTLA73cAHoWNkaHM
✅ Preview: https://react-query-git-fork-thomaspaulmann-patch-1-tanstack.vercel.app

[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 97aa025:

Sandbox	Source
tannerlinsley/react-query: basic	Configuration
tannerlinsley/react-query: basic-typescript	Configuration

[TkDodo]

Can you rebase this to the Alpha branch? Things have changed quite significantly, and the plugin will become stable in v4.

I think the windows check is there to make it work in SSR, but we have a new approach in v4 with a PersistQueryClientProvider (PR pending) so it might not be needed.

[thomaspaulmann]

Thanks for the quick response @TkDodo. Happy to check out the alpha version and even better if my changes aren't required.

I've realized that the unstable_batchUpdates are dependend on react-dom or react-native. Is there any way to not rely on this?

[thomaspaulmann]

@TkDodo the new PersistQueryClientProvider looks cool! You also check for window in #3248. Any way to get rid of this?

[TkDodo]

You also check for window in #3248. Any way to get rid of this?

haven't tried. we kept it because it was there before. As I said, it might be SSR related.

I've realized that the unstable_batchUpdates are dependend on react-dom or react-native. Is there any way to not rely on this?

where are we using this in the persist plugin?

[thomaspaulmann]

where are we using this in the persist plugin?

It's actually used by react-query itself, not the persist plugin. It's a side effect.

I can think of two solutions:

1. Treat the batching as optional and don't rely that react-dom or react-native is installed. This would require some changes in the lib. AFAIK, the batching is already optional. So probably not a big change.
2. Export the unstable_batchedUpdates from our renderer and make react-query aware of it. Not sure if you would be up for this.

Also, the batching of updates comes out of the box with React 18.

I would love to make react-query work easily with Raycast's API.

@tannerlinsley any thoughts on this?

[TkDodo]

I can think of two solutions:

I think it actually is exported:

https://github.com/tannerlinsley/react-query/blob/1f6ac1164e3ab39aec612c0fef77325a9fafcfb1/src/reactjs/setBatchUpdatesFn.ts#L4

The idea is to call: notifyManager.setBatchNotifyFunction(myBatchFn) where myBatchFn could just be a function that calls the callback if you don't need any batching. There is also a comment here:

https://github.com/tannerlinsley/react-query/blob/47bf076cd8df722623015fb24f8fddc589540af8/src/core/notifyManager.ts#L78-L84

Regarding the window check: I think we can remove it. The PersistQueryClientProvider is merged, and it restores in an effect, which doesn't do anything in SSR. I think the window check was initially necessary because we used window.localstorage directly, but we have since refactored it to accept an arbitrary persister that just has the WebStoragePersister interface, so it's no longer tied to window. Worst case we'd have to revert this :)

Can you please fix the conflicts on this PR, then we can go ahead with this :)

[TkDodo]

and it should go to beta now please :)

[codecov]

Codecov Report

Merging #3415 (e1350f9) into beta (4d753c0) will decrease coverage by 0.00%.
The diff coverage is 94.44%.

❗ Current head e1350f9 differs from pull request most recent head 97aa025. Consider uploading reports for the commit 97aa025 to get more accurate results

@@            Coverage Diff             @@
##             beta    #3415      +/-   ##
==========================================
- Coverage   96.94%   96.94%   -0.01%     
==========================================
  Files          46       46              
  Lines        2391     2386       -5     
  Branches      714      711       -3     
==========================================
- Hits         2318     2313       -5     
  Misses         71       71              
  Partials        2        2              

Impacted Files	Coverage Δ
src/persistQueryClient/persist.ts	95.00% <94.44%> (-0.46%)	⬇️
src/reactjs/Hydrate.tsx	100.00% <0.00%> (ø)
src/reactjs/tests/utils.tsx	97.72% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d753c0...97aa025. Read the comment docs.

[TkDodo]

there is also a section about environment check in the docs that we'd need to remove please:

https://github.com/tannerlinsley/react-query/blob/ba782560a715705942fd9137bfb284ac5f0445d9/docs/src/pages/plugins/persistQueryClient.md#L34-L35

[TkDodo]

It does say avoids build errors 🤔 . Maybe @jonathanstanley can tell us why they added that?

[jonathanstanley]

It does say avoids build errors 🤔 . Maybe @jonathanstanley can tell us why they added that?

indexed db is accessed through the window ... so if window is undefined and you try to access it (such as during node build) it would throw an error. the prior version had it too before i touched it

[TkDodo]

indexed db is accessed through the window ... so if window is undefined and you try to access it (such as during node build) it would throw an error. the prior version had it too before i touched it

yes it was there before, but I think it might be a leftover from a time when the persister accessed window.localstorage. Now, you need to pass the storage to the webstoragepersister, so it's in control of the user, so I guess the check is unneeded?

On the other hand, I stumbled upon a tiny SSR issue: how do you create a persister on the server since window doesn't exist 🤔. You'd need to make up a mock persister or so to use PersistQueryClientProvider. Maybe we should make the persister prop optional to accommodated for that, so that you can pass in undefined during SSR?

[jonathanstanley]

yes it was there before, but I think it might be a leftover from a time when the persister accessed window.localstorage. Now, you need to pass the storage to the webstoragepersister, so it's in control of the user, so I guess the check is unneeded?

I recall my build blowing up during testing when I tried without the check. I can't say with certainty... but I believe the example indexed db persister would throw during build if the check was removed. also, if it is removed, each persister would need to implement its own failsafe. something like a noop for all three calls: persistClient, restoreClient, removeClient. IMO it is cleaner to leave the check in persistQueryClient.

Maybe we should make the persister prop optional to accommodated for that, so that you can pass in undefined during SSR?

Perhaps another option would be to have a allowInSSR flag which skips the window checker when true

[TkDodo]

I recall my build blowing up during testing when I tried without the check.

@jonathanstanley here is a codesandbox, made with the preview build of this PR, using the index db persister, and it seems to work fine:

https://codesandbox.io/s/tannerlinsley-react-query-forked-9m6w7z?file=/src/index.tsx

Perhaps another option would be to have a allowInSSR flag which skips the window checker when true

my concern is not about the window check. It's more like, how do you make this code work in SSR:

 const persister = createWebStoragePersister({
   storage: window.localStorage,
 })

and what could you pass as storage if you don't have window available? Sure, you can just not call persistQueryCient on the server, as it doesn't make sense, but if you use the PersistQueryClientProvider, as advised here, you need to pass a persister, as the prop is mandatory. Note that restoring won't happen on the server because it's done inside a useEffect - so my proposal would be to make the persister prop optional for PersistQueryClientProvider. Otherwise, users will have to use different Providers on the server / client.

[thomaspaulmann]

Sorry for the delay. I think making the persister optional seems like a good idea. On the server, you shouldn't pass one because you won't persist it. Should this be part of this PR? Anything blocking to merge it?

One remaining thing is to deal with the setup of the unstable_batchedUpdates. Right now, react-query expects react-dom to be installed. I'd like to remove this restriction because we don't have react-dom in Raycast. For React Native, you export react-native. Any thought on how to not import any of those during the setup of react-query?

[TkDodo]

Sorry for the delay. I think making the persister optional seems like a good idea. On the server, you shouldn't pass one because you won't persist it. Should this be part of this PR?

yes please, because if we remove the window check, we'll make it very likely to break during SSR. With that change, users have the option to pass in undefined during SSR.

[TkDodo]

One remaining thing is to deal with the setup of the unstable_batchedUpdates. Right now, react-query expects react-dom to be installed. I'd like to remove this restriction because we don't have react-dom in Raycast. For React Native, you export react-native. Any thought on how to not import any of those during the setup of react-query?

would calling notifyManager.setBatchNotifyFunction to something else in your app not solve the issue?

[vercel]

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

Name	Status	Preview	Updated
react-query	✅ Ready (Inspect)	Visit Preview	Apr 26, 2022 at 9:22AM (UTC)

[thomaspaulmann]

Hey @TkDodo,

Just picking this up and I would love to push this over the finish line for the next beta release (do you have a target date?). I made the storage optional as bespoken.

would calling notifyManager.setBatchNotifyFunction to something else in your app not solve the issue?

Unfortunately this isn't enough. Internally, react-query still imports react-dom as a dependency, which Raycast doesn't have, it's a custom React reconciler (similar to React Native). This gives me the following error:

I put together a test project for you to reproduce it here. Is there any way to dynamically import react-dom when available? I see you do something similar for React Native in your build steps. Ideally, the logic would be something like this:

1. If react-dom exists as peer dependency, use it's batch functions.
2. If react-native exists as peer dependency, use it's batch functions.
3. If none exists as peer dependency, use a fallback batch function (noop).

We can also expose a batch function from our reconciler (we have one internally already) if that is helpful. Ideally we can make react-query work without many setup costs. Happy to discuss this further.

[TkDodo]

I see you do something similar for React Native in your build steps

can you point me to where we are doing that for react-native please?

[thomaspaulmann]

can you point me to where we are doing that for react-native please?

Sure. Here is the batch function defined for React Native, notice .native in the filename. And here is the function for React. They are used here which is called from the index.ts file.

Somehow the build system picks then the one or other file. I'm not sure how this is getting resolved.

[TkDodo]

as far as I know, the docs still reference that an environment check for window is being made, see: https://react-query-beta.tanstack.com/plugins/persistQueryClient#environment-checking

can we remove that please

[TkDodo]

Somehow the build system picks then the one or other file. I'm not sure how this is getting resolved.

Thanks @thomaspaulmann . I'm aware of those code pieces, but I am not aware of any bundling on our end doing something specific about files with a .native extension. That's what I was asking :)

but maybe we can discuss this in a follow up issue and get this one merged regardless? prettier is failing, but apart from that, it looks good (see my comments for some small change requests)

[TkDodo]

superseded by:

• fix(persist): remove environment check #3658

[thomaspaulmann]

Oh nice, thanks for the update! Sorry that I didn't get around fixing my bits earlier. Anything that I can help with the remaining dependency on react-dom that I described before?