Skip to content

Fix makeQueryCache default caching behaviour in node #749

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 17, 2020
Merged

Fix makeQueryCache default caching behaviour in node #749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/core/queryCache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,16 @@ import {
import { defaultConfigRef } from './config'
import { makeQuery } from './query'

export const queryCache = makeQueryCache()
export const queryCache = makeQueryCache({ frozen: isServer })
Copy link
Collaborator

@tannerlinsley tannerlinsley Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I had someone trying to use ReactQuery in the CLI the other day. This would make that default to frozen, correct?

Copy link
Collaborator Author

@Ephem Ephem Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the global cache will still be frozen by default in all server environments in this PR since changing that behavior would regress #70 and in my eyes be a breaking change.

I would suggest not only that we change this default behavior in the next major, but actually that we get rid of the global cache entirely (at least on the server side, possibly client too). So instead of making the global cache available for CLI-development, we push everyone to create their own cache.

Creating a queryCache via makeQueryCache is trivial and you can easily just export that as a global cache yourself if that is what you want. What we would gain from this is that developers would explicitly have to create a cache somewhere, making it clear that whatever you fetch into this cache will be shared. This would let us get rid of the frozen/unfrozen concept, streamline the behavior and still make it hard to/avoid accidentally leaking data cross-request.

ADD: If this is something we decide to do, we could add a deprecation-log in the development build to get people prepared for it.

Copy link
Collaborator

@tannerlinsley tannerlinsley Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I totally wish I had avoided the singleton from the start, it's going to be very difficult to get away from in the future. Even IMO, it's very convenient. I'd like to avoid it as long as possible ;)


export const queryCaches = [queryCache]

export function makeQueryCache({ frozen = isServer, defaultConfig } = {}) {
export function makeQueryCache({ frozen = false, defaultConfig } = {}) {
// A frozen cache does not add new queries to the cache
const globalListeners = []

const configRef = defaultConfig
? { current: { ...defaultConfigRef.current, ...defaultConfig }}
? { current: { ...defaultConfigRef.current, ...defaultConfig } }
: defaultConfigRef

const queryCache = {
Expand Down
49 changes: 40 additions & 9 deletions src/react/tests/ssr.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,44 @@ import {
usePaginatedQuery,
ReactQueryCacheProvider,
queryCache,
queryCaches,
makeQueryCache,
useQuery,
} from '../index'
import { sleep } from './utils'

describe('Server Side Rendering', () => {
describe('global cache', () => {
afterEach(() => {
queryCaches.forEach(cache => cache.clear({ notify: false }))
})
// A frozen cache does not cache any data. This is the default
// for the global cache in a node-environment, since it's
// otherwise easy to cache data between separate requests,
// which is a security risk.
//
// See https://github.com/tannerlinsley/react-query/issues/70
it('global cache should be frozen by default', async () => {
const fetchFn = () => Promise.resolve('data')
const data = await queryCache.prefetchQuery('key', fetchFn)

expect(data).toBe('data')
expect(queryCache.getQuery('key')).toBeFalsy()

queryCache.clear({ notify: false })
})

// When consumers of the library create a cache explicitly by
// calling makeQueryCache, they take on the responsibility of
// not using that cache to cache data between requests or do so
// in a safe way.
it('created caches should be unfrozen by default', async () => {
const queryCache = makeQueryCache()
const fetchFn = () => Promise.resolve('data')
const data = await queryCache.prefetchQuery('key', fetchFn)

expect(data).toBe('data')
expect(queryCache.getQuery('key')).toBeTruthy()
})

describe('frozen cache', () => {
it('should not trigger fetch', () => {
const queryCache = makeQueryCache({ frozen: true })
const queryFn = jest.fn()

function Page() {
Expand All @@ -35,14 +60,19 @@ describe('Server Side Rendering', () => {
)
}

const markup = renderToString(<Page />)
const markup = renderToString(
<ReactQueryCacheProvider queryCache={queryCache}>
<Page />
</ReactQueryCacheProvider>
)

expect(markup).toContain('status loading')
expect(queryFn).toHaveBeenCalledTimes(0)
})

// See https://github.com/tannerlinsley/react-query/issues/70
it('should not add initialData to the cache', () => {
const queryCache = makeQueryCache({ frozen: true })

function Page() {
const [page, setPage] = React.useState(1)
const { resolvedData } = usePaginatedQuery(
Expand All @@ -54,10 +84,10 @@ describe('Server Side Rendering', () => {
)

return (
<div>
<ReactQueryCacheProvider queryCache={queryCache}>
<h1 data-testid="title">{resolvedData}</h1>
<button onClick={() => setPage(page + 1)}>next</button>
</div>
</ReactQueryCacheProvider>
)
}

Expand All @@ -67,6 +97,7 @@ describe('Server Side Rendering', () => {
})

it('should not add prefetched data to the cache', async () => {
const queryCache = makeQueryCache({ frozen: true })
const fetchFn = () => Promise.resolve('data')
const data = await queryCache.prefetchQuery('key', fetchFn)

Expand Down