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

SSR: queries are cached on server #70

Closed
cherniavskii opened this issue Dec 9, 2019 · 14 comments
Closed

SSR: queries are cached on server #70

cherniavskii opened this issue Dec 9, 2019 · 14 comments

Comments

@cherniavskii
Copy link
Contributor

I've started to use react-query with next.js SSR using initialData option.
It works pretty good, but looks like queries are cached in server js bundle and are used across all requests.

Here's a minimal example reproducing the issue: https://codesandbox.io/s/react-query-ssr-cache-rkfsh

And a GIF which demonstrates the issue:
Screen Recording 2019-12-09 at 15 55 15

Note, that user index is incremented every time, but server always renders 1.
Here's what happens:

  • index is 1, there's no queries stored yet
  • user requests the page, data query is created and initialData is set
  • user refreshes the page
  • new initialData is passed to useQuery, but there already is a data query, so cached data is user on server render
  • page is rendered on client, since there's no queries in client bundle, new initialData is used

I've also checked if swr suffer from the same issue, but it works just fine - see https://codesandbox.io/s/swr-ssr-cache-qrmgn

Do you have any idea how it can be handled on react-query side?
As a workaround, I'm calling clearCachedQueries in _document.getInitialProps, which clears cached queries on server on every request

@cherniavskii cherniavskii changed the title SSR: initialData is cached SSR: queries are cached on server Dec 9, 2019
@tannerlinsley
Copy link
Collaborator

So it sounds to me like you are asking for a way to disable caching when react-query is used on the server, is that right? Outside of that, React Query is doing what it was designed to do, albeit, in the client. So let me ask, are there situations where you would want to use the cached value from useQuery on the server?

@cherniavskii
Copy link
Contributor Author

@tannerlinsley

you are asking for a way to disable caching when react-query is used on the server, is that right?

Not sure about that. I think caching on server isn't an issue as long as that cache is not shared between page requests.

But what wonders me the most is that swr doesn't suffer from that issue, although conceptually its caching mechanism works very similarly (correct me if I'm wrong).

I briefly looked through swr source code, but didn't find any code specific to this issue.

@tannerlinsley
Copy link
Collaborator

Hmmm. One guess is that in SWR, initialData may somehow be overriding the cached data. IMO, it shouldn't.

The other idea is that maybe initialData is somehow updating the query cache?

Regardless of what SWR does, we can figure out a way to fix this. Some of my thoughts keep going back to the idea that this use case is a bit contrived. I would really like to know how you're approaching this in a real example to better know what course of action to take.

@cherniavskii
Copy link
Contributor Author

Huh, looked at swr code once again and I think I've found the key difference.

In swr initialData isn't saved to cache, but used as a fallback when returning results - see https://github.com/zeit/swr/blob/05779e6c892e9dac9007faa37044c3aa5fb8cd69/src/use-swr.ts#L563

In react-query initialData is stored in cache.
So in swr cache is never modified on server.

So the fix would be to not store initialData in cache, but simply use it as a fallback

I will willingly work on PR, but source code in the repo is outdated (I've checked initialData implementation in production build from NPM).

@cherniavskii
Copy link
Contributor Author

I would really like to know how you're approaching this in a real example to better know what course of action to take.

Yes, I intentionally made that example to be as simple as possible.

I'm mostly worried about security in this case.

I think the real-life use case would be SSRing user data:

  • logged-in user enters dashboard page (rendered on a server)
  • html rendered on server contains data of the first user whose dashboard was rendered after server has started

@cherniavskii
Copy link
Contributor Author

initialData may somehow be overriding the cached data.

Yes, that's exactly what I've discovered.

IMO, it shouldn't.

Can you elaborate on this?

@tannerlinsley
Copy link
Collaborator

Yes, security should come first. Sharing cache data between users is a top concern for apps, but not technically something React-Query can control. Something I'm asking myself is this, if you weren't doing server-side rendering, and just requesting data, wouldn't you be sending some type of identifier or token to get user-specific data, then validating that token for access to it?

I bring this up because for our discussions sake, you could technically add a userId variable to the query and you would no longer be sharing cache results. In fact, for multi-user apps, it's almost expected that if the app doesn't clear the query cache when user context changes (log-in/log-out), then userId should be represented as a variable in almost every query, so as to result in a different query hash for different users. That would technically be the only way to store multi-user data in the same cache without security concerns.

To clarify on your other findings, I believe that allowing initialData seed an empty cache instead of only acting as a fallback is actually an awesome and expressive feature of the API.

So after talking this through I think the best course of action is that if you want to cache data in the server context, you should either (1) try to use a query id or variable that will allow multi-users to exist in the cache at the same time (2) clear the cache when user context changes

More about (2) from above, the more I talk through it in my head, the more I don't feel like caching query results in the server is a good idea at all. It makes me think that there should simply be a way to opt out of using the cache at all, or simply force react-query to not cache on the server. I feel like it will only lead to major headaches and like you have illustrated here, security problems.

Thoughts?

@cherniavskii
Copy link
Contributor Author

wouldn't you be sending some type of identifier or token to get user-specific data, then validating that token for access to it?

Yes, this is valid point - user data are protected on the backend side.

then userId should be represented as a variable in almost every query

I don't think it's obvious for developers. If user id isn't required by API endpoint (it's a common case from my experience), I don't think anyone would bother to provide one as query variable.

I believe that allowing initialData seed an empty cache instead of only acting as a fallback is actually an awesome and expressive feature of the API.

I agree, that prefilling cache with initialData is more elegant solution, but are there benefits from user's/developer's perspective?

the more I don't feel like caching query results in the server is a good idea at all

Yes, I agree. Caching query result on server doesn't make any sense to me now.
What can we do to disable it?

@tannerlinsley
Copy link
Collaborator

I think a good question here is "should we disable caching by defualt in server-side scenarios?"

I think the answer is yes. I feel like the only feature people should be using during SSR is initialData, which doesn't technically rely on persisting anything to the cache, it's merely there for rendering an initial state on the server. After that, the cache state is relatively useless until it gets to the client.

@cherniavskii
Copy link
Contributor Author

should we disable caching by defualt in server-side scenarios?

Yes, I agree.

BTW, looks like using initialData as a fallback has drawbacks like this vercel/swr#178

@tannerlinsley
Copy link
Collaborator

😂 Yeah, I don't think React Query has that issue. Once initialData is ingested into the query, the null data state isn't possible (unless initialData is null, I guess). Even during pagination, the data state is never null or reset (not sure why SWR would be doing that regardless).

@tannerlinsley
Copy link
Collaborator

Version 0.3.22 is now available with caching disabled when typeof window === 'undefined'.

@cherniavskii
Copy link
Contributor Author

@tannerlinsley Cool, thanks a lot!
I'll check this out

@cherniavskii
Copy link
Contributor Author

@tannerlinsley works like a charm, thanks again!

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

No branches or pull requests

2 participants