From 5c75a8eb99a4f2f61cb042a01ee57f28caf304c9 Mon Sep 17 00:00:00 2001 From: Kamran Ayub Date: Wed, 8 Apr 2020 13:41:39 -0500 Subject: [PATCH 1/7] Add manual/prefetch cleanup test that fails --- src/tests/cleanup.js | 73 ++++++++++++++++++++++++++++++++++++++ src/tests/cleanup.test.js | 46 ++++++++++++++++++++++++ src/tests/useQuery.test.js | 2 -- 3 files changed, 119 insertions(+), 2 deletions(-) create mode 100644 src/tests/cleanup.js create mode 100644 src/tests/cleanup.test.js diff --git a/src/tests/cleanup.js b/src/tests/cleanup.js new file mode 100644 index 0000000000..ac3476035d --- /dev/null +++ b/src/tests/cleanup.js @@ -0,0 +1,73 @@ +import React from 'react' +import { useQuery, ReactQueryConfigProvider } from '..' + +const queryConfig = { + manual: true, +} + +const fetchUser = () => Promise.resolve({ userId: 1, username: 'Test' }) +const fetchPosts = () => Promise.resolve([{ postId: 1, body: 'Hello' }]) + +const UserProfile = () => { + const { data: user } = useQuery(['user', 1], fetchUser) + + if (user) { + return
User: {user.username}
+ } else { + return
Loading user...
+ } +} + +const PostsByUser = () => { + const { data } = useQuery(['posts', 1], fetchPosts) + + if (data) { + return ( +
+ {data.map(post => ( +
Post: {post.body}
+ ))} +
+ ) + } + + return null +} + +export const Page = ({ onAfterHide }) => { + const [showContent, setShowContent] = React.useState(false) + + return ( + +
+

Hello CodeSandbox

+

+ +

+ + {showContent && ( +
+ + +

+ +

+
+ )} +
+ + ) +} diff --git a/src/tests/cleanup.test.js b/src/tests/cleanup.test.js new file mode 100644 index 0000000000..1909a185be --- /dev/null +++ b/src/tests/cleanup.test.js @@ -0,0 +1,46 @@ +import React from 'react' +import { render, fireEvent } from '@testing-library/react' +import { queryCache } from '..' +import { Page } from './cleanup' + +describe('query cleanup', () => { + afterEach(() => { + queryCache.clear() + }) + + test('should prefetchQuery by force for first query', async () => { + await queryCache.prefetchQuery( + ['user', 1], + () => Promise.resolve({ userId: 1, username: 'Mock User' }), + { force: true } + ) + + const { findByTestId, findByText } = render() + + const showContent = await findByTestId('showContent') + + fireEvent.click(showContent) + + const userProfile = await findByText('User: Mock User') + + expect(userProfile).toBeDefined() + }) + + test('should prefetch by force for second query only', async () => { + await queryCache.prefetchQuery( + ['posts', 1], + () => Promise.resolve([{ postId: 1, body: 'World' }]), + { force: true } + ) + + const { findByTestId, findByText } = render() + + const showContent = await findByTestId('showContent') + + fireEvent.click(showContent) + + const userPosts = await findByText('Post: World') + + expect(userPosts).toBeDefined() + }) +}) diff --git a/src/tests/useQuery.test.js b/src/tests/useQuery.test.js index 2f94c39925..69ebe4eca1 100644 --- a/src/tests/useQuery.test.js +++ b/src/tests/useQuery.test.js @@ -3,7 +3,6 @@ import { act, waitForElement, fireEvent, - cleanup, } from '@testing-library/react' import * as React from 'react' @@ -13,7 +12,6 @@ import { sleep } from './utils' describe('useQuery', () => { afterEach(() => { queryCache.clear() - cleanup() }) // See https://github.com/tannerlinsley/react-query/issues/105 From 3d1aa7ac80a2dac8df8a9a3018687331a75d4bb0 Mon Sep 17 00:00:00 2001 From: Kamran Ayub Date: Wed, 8 Apr 2020 13:53:23 -0500 Subject: [PATCH 2/7] Prefetch both queries in second test --- src/tests/cleanup.test.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/tests/cleanup.test.js b/src/tests/cleanup.test.js index 1909a185be..e4e30b7e87 100644 --- a/src/tests/cleanup.test.js +++ b/src/tests/cleanup.test.js @@ -26,7 +26,12 @@ describe('query cleanup', () => { expect(userProfile).toBeDefined() }) - test('should prefetch by force for second query only', async () => { + test('should prefetch by force for both queries', async () => { + await queryCache.prefetchQuery( + ['user', 1], + () => Promise.resolve({ userId: 1, username: 'Mock User' }), + { force: true } + ) await queryCache.prefetchQuery( ['posts', 1], () => Promise.resolve([{ postId: 1, body: 'World' }]), From aa44aa91b64e0da163100695668e19931652a8ca Mon Sep 17 00:00:00 2001 From: Kamran Ayub Date: Wed, 8 Apr 2020 13:55:37 -0500 Subject: [PATCH 3/7] Add cleanup comment that will make the tests pass --- src/tests/cleanup.test.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/tests/cleanup.test.js b/src/tests/cleanup.test.js index e4e30b7e87..702638b449 100644 --- a/src/tests/cleanup.test.js +++ b/src/tests/cleanup.test.js @@ -1,11 +1,15 @@ import React from 'react' import { render, fireEvent } from '@testing-library/react' +import { cleanup } from '@testing-library/react/pure' import { queryCache } from '..' import { Page } from './cleanup' describe('query cleanup', () => { afterEach(() => { queryCache.clear() + // This will make things pass as it force-flushes the microtask queue twice + // but it's a bandaid over an underlying issue + // await cleanup() }) test('should prefetchQuery by force for first query', async () => { From e30cf94654201786b1929d5adf049809584117b1 Mon Sep 17 00:00:00 2001 From: Kamran Ayub Date: Wed, 8 Apr 2020 14:54:38 -0500 Subject: [PATCH 4/7] Store when the GC was initiated and compare against updatedAt --- src/queryCache.js | 5 ++++- src/tests/cleanup.js | 3 +-- src/tests/cleanup.test.js | 26 +++++--------------------- 3 files changed, 10 insertions(+), 24 deletions(-) diff --git a/src/queryCache.js b/src/queryCache.js index 20a2399887..bda81482d8 100644 --- a/src/queryCache.js +++ b/src/queryCache.js @@ -238,9 +238,12 @@ export function makeQueryCache() { } query.scheduleGarbageCollection = () => { + const scheduledAt = Date.now() query.cacheTimeout = setTimeout( () => { - cache.removeQueries(d => d.queryHash === query.queryHash) + cache.removeQueries( + d => d.state.updatedAt < scheduledAt && d.queryHash === query.queryHash + ) }, typeof query.state.data === 'undefined' && query.state.status !== 'error' diff --git a/src/tests/cleanup.js b/src/tests/cleanup.js index ac3476035d..3a277d2169 100644 --- a/src/tests/cleanup.js +++ b/src/tests/cleanup.js @@ -34,7 +34,7 @@ const PostsByUser = () => { return null } -export const Page = ({ onAfterHide }) => { +export const Page = () => { const [showContent, setShowContent] = React.useState(false) return ( @@ -59,7 +59,6 @@ export const Page = ({ onAfterHide }) => { data-testid="hideContent" onClick={() => { setShowContent(false) - if (onAfterHide) onAfterHide() }} > Close Modal diff --git a/src/tests/cleanup.test.js b/src/tests/cleanup.test.js index 702638b449..f15bf89c56 100644 --- a/src/tests/cleanup.test.js +++ b/src/tests/cleanup.test.js @@ -1,23 +1,15 @@ import React from 'react' import { render, fireEvent } from '@testing-library/react' -import { cleanup } from '@testing-library/react/pure' import { queryCache } from '..' import { Page } from './cleanup' describe('query cleanup', () => { afterEach(() => { queryCache.clear() - // This will make things pass as it force-flushes the microtask queue twice - // but it's a bandaid over an underlying issue - // await cleanup() }) - test('should prefetchQuery by force for first query', async () => { - await queryCache.prefetchQuery( - ['user', 1], - () => Promise.resolve({ userId: 1, username: 'Mock User' }), - { force: true } - ) + test('should set query data on first test', async () => { + queryCache.setQueryData(['user', 1], { userId: 1, username: 'Mock User' }) const { findByTestId, findByText } = render() @@ -30,17 +22,9 @@ describe('query cleanup', () => { expect(userProfile).toBeDefined() }) - test('should prefetch by force for both queries', async () => { - await queryCache.prefetchQuery( - ['user', 1], - () => Promise.resolve({ userId: 1, username: 'Mock User' }), - { force: true } - ) - await queryCache.prefetchQuery( - ['posts', 1], - () => Promise.resolve([{ postId: 1, body: 'World' }]), - { force: true } - ) + test('should not garbage collect new initial data', async () => { + queryCache.setQueryData(['user', 1], { userId: 1, username: 'Mock 2 User' }) + queryCache.setQueryData(['posts', 1], [{ postId: 1, body: 'World' }]) const { findByTestId, findByText } = render() From 5835d22e8359843ebac556bb5125ad446e381176 Mon Sep 17 00:00:00 2001 From: Kamran Ayub Date: Wed, 8 Apr 2020 15:25:07 -0500 Subject: [PATCH 5/7] Replace solution with state marker check and streamline test --- .vscode/settings.json | 21 ++++++++--- src/queryCache.js | 12 +++++- src/tests/cleanup.js | 72 ------------------------------------ src/tests/cleanup.test.js | 39 ------------------- src/tests/queryCache.test.js | 30 +++++++++++++++ 5 files changed, 56 insertions(+), 118 deletions(-) delete mode 100644 src/tests/cleanup.js delete mode 100644 src/tests/cleanup.test.js diff --git a/.vscode/settings.json b/.vscode/settings.json index f3bb781717..a1981000b6 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,8 +1,19 @@ { "workbench.colorCustomizations": { - "titleBar.activeBackground": "#ff4154", // change this color! - "titleBar.inactiveBackground": "#ff4154", // change this color! - "titleBar.activeForeground": "#ffffff", // change this color! - "titleBar.inactiveForeground": "#ffffff" // change this color! - } + "activityBar.background": "#ff7482", + "activityBar.activeBackground": "#ff7482", + "activityBar.activeBorder": "#70ff60", + "activityBar.foreground": "#15202b", + "activityBar.inactiveForeground": "#15202b99", + "activityBarBadge.background": "#70ff60", + "activityBarBadge.foreground": "#15202b", + "titleBar.activeBackground": "#ff4154", + "titleBar.inactiveBackground": "#ff415499", + "titleBar.activeForeground": "#e7e7e7", + "titleBar.inactiveForeground": "#e7e7e799", + "statusBar.background": "#ff4154", + "statusBarItem.hoverBackground": "#ff7482", + "statusBar.foreground": "#e7e7e7" + }, + "peacock.color": "#ff4154" } diff --git a/src/queryCache.js b/src/queryCache.js index bda81482d8..a343ef7282 100644 --- a/src/queryCache.js +++ b/src/queryCache.js @@ -17,6 +17,7 @@ export const queryCache = makeQueryCache() const actionInit = {} const actionFailed = {} const actionMarkStale = {} +const actionMarkGC = {} const actionFetch = {} const actionSuccess = {} const actionError = {} @@ -238,11 +239,11 @@ export function makeQueryCache() { } query.scheduleGarbageCollection = () => { - const scheduledAt = Date.now() + dispatch({ type: actionMarkGC }) query.cacheTimeout = setTimeout( () => { cache.removeQueries( - d => d.state.updatedAt < scheduledAt && d.queryHash === query.queryHash + d => d.state.markedForGarbageCollection && d.queryHash === query.queryHash ) }, typeof query.state.data === 'undefined' && @@ -450,6 +451,7 @@ export function defaultQueryReducer(state, action) { canFetchMore: false, failureCount: 0, isStale: action.isStale, + markedForGarbageCollection: false, data: action.initialData, updatedAt: action.hasInitialData ? Date.now() : 0, } @@ -463,6 +465,12 @@ export function defaultQueryReducer(state, action) { ...state, isStale: true, } + case actionMarkGC: { + return { + ...state, + markedForGarbageCollection: true, + } + } case actionFetch: return { ...state, diff --git a/src/tests/cleanup.js b/src/tests/cleanup.js deleted file mode 100644 index 3a277d2169..0000000000 --- a/src/tests/cleanup.js +++ /dev/null @@ -1,72 +0,0 @@ -import React from 'react' -import { useQuery, ReactQueryConfigProvider } from '..' - -const queryConfig = { - manual: true, -} - -const fetchUser = () => Promise.resolve({ userId: 1, username: 'Test' }) -const fetchPosts = () => Promise.resolve([{ postId: 1, body: 'Hello' }]) - -const UserProfile = () => { - const { data: user } = useQuery(['user', 1], fetchUser) - - if (user) { - return
User: {user.username}
- } else { - return
Loading user...
- } -} - -const PostsByUser = () => { - const { data } = useQuery(['posts', 1], fetchPosts) - - if (data) { - return ( -
- {data.map(post => ( -
Post: {post.body}
- ))} -
- ) - } - - return null -} - -export const Page = () => { - const [showContent, setShowContent] = React.useState(false) - - return ( - -
-

Hello CodeSandbox

-

- -

- - {showContent && ( -
- - -

- -

-
- )} -
- - ) -} diff --git a/src/tests/cleanup.test.js b/src/tests/cleanup.test.js deleted file mode 100644 index f15bf89c56..0000000000 --- a/src/tests/cleanup.test.js +++ /dev/null @@ -1,39 +0,0 @@ -import React from 'react' -import { render, fireEvent } from '@testing-library/react' -import { queryCache } from '..' -import { Page } from './cleanup' - -describe('query cleanup', () => { - afterEach(() => { - queryCache.clear() - }) - - test('should set query data on first test', async () => { - queryCache.setQueryData(['user', 1], { userId: 1, username: 'Mock User' }) - - const { findByTestId, findByText } = render() - - const showContent = await findByTestId('showContent') - - fireEvent.click(showContent) - - const userProfile = await findByText('User: Mock User') - - expect(userProfile).toBeDefined() - }) - - test('should not garbage collect new initial data', async () => { - queryCache.setQueryData(['user', 1], { userId: 1, username: 'Mock 2 User' }) - queryCache.setQueryData(['posts', 1], [{ postId: 1, body: 'World' }]) - - const { findByTestId, findByText } = render() - - const showContent = await findByTestId('showContent') - - fireEvent.click(showContent) - - const userPosts = await findByText('Post: World') - - expect(userPosts).toBeDefined() - }) -}) diff --git a/src/tests/queryCache.test.js b/src/tests/queryCache.test.js index ab7b7a21a0..ffee2041ad 100644 --- a/src/tests/queryCache.test.js +++ b/src/tests/queryCache.test.js @@ -143,4 +143,34 @@ describe('queryCache', () => { await sleep(50) expect(query.state.isStale).toBe(false) }) + + test('query is garbage collected when unsubscribed to', async () => { + const queryKey = 'key' + const fetchData = () => Promise.resolve('data') + await queryCache.prefetchQuery(queryKey, fetchData, { cacheTime: 0 }) + const query = queryCache.getQuery(queryKey) + expect(query.state.markedForGarbageCollection).toBe(false) + const unsubscribe = query.subscribe(1) + unsubscribe() + expect(query.state.markedForGarbageCollection).toBe(true) + await sleep(1) + expect(queryCache.getQuery(queryKey)).toBeUndefined() + }) + + test('query is not garbage collected unless markedForGarbageCollection is true', async () => { + const queryKey = 'key' + const fetchData = () => Promise.resolve(undefined) + await queryCache.prefetchQuery(queryKey, fetchData, { cacheTime: 0 }) + const query = queryCache.getQuery(queryKey) + expect(query.state.markedForGarbageCollection).toBe(false) + const unsubscribe = query.subscribe(1) + unsubscribe() + expect(query.state.markedForGarbageCollection).toBe(true) + queryCache.clear() + queryCache.setQueryData(queryKey, 'data') + await sleep(1) + const newQuery = queryCache.getQuery(queryKey) + expect(newQuery.state.markedForGarbageCollection).toBe(false) + expect(newQuery.state.data).toBe('data') + }) }) From 6976cb37eda39f530f192ae51e535e0a7c3e7282 Mon Sep 17 00:00:00 2001 From: Kamran Ayub Date: Wed, 8 Apr 2020 15:30:19 -0500 Subject: [PATCH 6/7] Undo setting.json change --- .vscode/settings.json | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index a1981000b6..5b318dc360 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,19 +1,8 @@ { "workbench.colorCustomizations": { - "activityBar.background": "#ff7482", - "activityBar.activeBackground": "#ff7482", - "activityBar.activeBorder": "#70ff60", - "activityBar.foreground": "#15202b", - "activityBar.inactiveForeground": "#15202b99", - "activityBarBadge.background": "#70ff60", - "activityBarBadge.foreground": "#15202b", - "titleBar.activeBackground": "#ff4154", - "titleBar.inactiveBackground": "#ff415499", - "titleBar.activeForeground": "#e7e7e7", - "titleBar.inactiveForeground": "#e7e7e799", - "statusBar.background": "#ff4154", - "statusBarItem.hoverBackground": "#ff7482", - "statusBar.foreground": "#e7e7e7" - }, - "peacock.color": "#ff4154" -} + "titleBar.activeBackground": "#ff4154", // change this color! + "titleBar.inactiveBackground": "#ff4154", // change this color! + "titleBar.activeForeground": "#ffffff", // change this color! + "titleBar.inactiveForeground": "#ffffff" // change this color! + } +} \ No newline at end of file From 7dbb61affb073f9c4a4dbbc01514dc52881ac386 Mon Sep 17 00:00:00 2001 From: Kamran Ayub Date: Wed, 8 Apr 2020 15:30:59 -0500 Subject: [PATCH 7/7] Undo setting.json change --- .vscode/settings.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 5b318dc360..f3bb781717 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -5,4 +5,4 @@ "titleBar.activeForeground": "#ffffff", // change this color! "titleBar.inactiveForeground": "#ffffff" // change this color! } -} \ No newline at end of file +}