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.

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

Add a staleTime to queries #1167

Merged
merged 14 commits into from
Oct 20, 2022
Merged

Add a staleTime to queries #1167

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1c4ca56
Add a staleTime to queries
Janpot Oct 17, 2022
bb09d1b
Add devtools
Janpot Oct 17, 2022
4d0b3f4
fix staleTime
Janpot Oct 17, 2022
88a7415
Merge branch 'master' into staleTime
Janpot Oct 18, 2022
41d80cd
Merge branch 'master' into staleTime
Janpot Oct 19, 2022
034acd8
remove options
Janpot Oct 19, 2022
128fa0a
ldskj
Janpot Oct 19, 2022
fba3994
invalidate while editing
Janpot Oct 20, 2022
5498438
reset staleTime
Janpot Oct 20, 2022
cb2ff85
fix type
Janpot Oct 20, 2022
068964c
10s
Janpot Oct 20, 2022
9af81df
fix deploy
Janpot Oct 20, 2022
bb05585
1 min
Janpot Oct 20, 2022
aeaa97c
Merge branch 'master' into staleTime
Janpot Oct 20, 2022
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
1 change: 1 addition & 0 deletions packages/toolpad-app/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
"@sentry/nextjs": "^7.15.0",
"@swc/wasm": "^1.3.6",
"@tanstack/react-query": "^4.10.3",
"@tanstack/react-query-devtools": "^4.12.0",
"@types/cors": "^2.8.12",
"abort-controller": "^3.0.0",
"arg": "^5.0.2",
Expand Down
2 changes: 2 additions & 0 deletions packages/toolpad-app/src/appDom/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,9 @@ export interface QueryNode<Q = any> extends AppDomNodeBase {
readonly query: ConstantAttrValue<Q>;
readonly transform?: ConstantAttrValue<string>;
readonly transformEnabled?: ConstantAttrValue<boolean>;
/** @deprecated Not necessary to be user-facing, we will expose staleTime instead if necessary */
readonly refetchOnWindowFocus?: ConstantAttrValue<boolean>;
/** @deprecated Not necessary to be user-facing, we will expose staleTime instead if necessary */
readonly refetchOnReconnect?: ConstantAttrValue<boolean>;
readonly refetchInterval?: ConstantAttrValue<number>;
readonly cacheTime?: ConstantAttrValue<number>;
Expand Down
20 changes: 18 additions & 2 deletions packages/toolpad-app/src/runtime/ToolpadApp.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,14 @@ import NoSsr from '../components/NoSsr';
import { execDataSourceQuery, useDataQuery, UseDataQueryConfig, UseFetch } from './useDataQuery';
import { useAppContext, AppContextProvider } from './AppContext';
import { CanvasHooksContext, NavigateToPage } from './CanvasHooksContext';
import useBoolean from '../utils/useBoolean';

const ReactQueryDevtoolsProduction = React.lazy(() =>
// eslint-disable-next-line import/extensions
import('@tanstack/react-query-devtools/build/lib/index.prod.js').then((d) => ({
default: d.ReactQueryDevtools,
})),
);

const EMPTY_ARRAY: any[] = [];
const EMPTY_OBJECT: any = {};
Expand All @@ -82,8 +90,6 @@ const INITIAL_FETCH: UseFetch = {
const USE_DATA_QUERY_CONFIG_KEYS: readonly (keyof UseDataQueryConfig)[] = [
'enabled',
'refetchInterval',
'refetchOnReconnect',
'refetchOnWindowFocus',
];

function usePageNavigator(): NavigateToPage {
Expand Down Expand Up @@ -872,6 +878,7 @@ const queryClient = new QueryClient({
defaultOptions: {
queries: {
retry: false,
staleTime: 10 * 60 * 1000,
Copy link
Member

Choose a reason for hiding this comment

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

I think that we would be OK with:

Suggested change
staleTime: 10 * 60 * 1000,
staleTime: 1000,

From what I understand, it would mean that we never query more than once every second, so 3600 requests per hour, below the 5000 limits of GitHub. It's not pretty, but I wouldn't expect anyone to want to refresh more than once per second. But for example in https://master--toolpad.mui.com/_toolpad/app/cl6rqzry10009arlv9sto6qja/pages/ip23ggo I would definitely want to use the refresh button after I label a few issues.

Copy link
Member Author

@Janpot Janpot Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

it would mean that we never query more than once every second

But it's not an unrealistic scenario to have more than one github query on one page. Perhaps we can make it slightly longer, say 10s? I'm also planning to make this configurable at some point (#1093).

Copy link
Member

@oliviertassinari oliviertassinari Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

From my perspective, when using a refresh button, we should feel like the results returned are up to date, reliably. I doubt that a staleTime > 1s can allow this UX.
A second concern is that if we set 10s and we fix the over-fetching problem, then we might be blind to a regression.

What I think would be great is to first solve the over-fetching, and then consider caching but to solve API requests that are slow.

Copy link
Member Author

@Janpot Janpot Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

When you call refetch it should ignore cache and always refetch. So the button should always work, regardless of how long we make the staleTime.

Copy link
Member

@oliviertassinari oliviertassinari Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

When you call refetch it should ignore cache and always refetch.

Oh yeah, this would be even better.

Copy link
Member Author

Choose a reason for hiding this comment

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

@oliviertassinari I took care of the cache busting in the editor. Do you still think we should shorten the staleTime when we have the refetch function?

Copy link
Member

@oliviertassinari oliviertassinari Oct 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@Janpot Per #1163 (comment), I think that we could even remove staleTime if it allows us to not fight with cache invalidation issues, e.g. when editing records. But to be honest, I don't fully understand what this property does so I might not be the best one to answer.

Copy link
Member Author

@Janpot Janpot Oct 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

e.g. when editing records

The current architecture doesn't help with that neither. You'd still have to call refetch after updating the row to make the table up to date. There is no way to do it automatic as the list/delete/update methods are not linked in any way. For that we'd need something like a collection that we can link as a whole to a datagrid (#385)

But to be honest, I don't fully understand what this property does so I might not be the best one to answer.

staleTime sets the time after which data is considered out of date. That doesn't mean react-query will automatically refetch it after this time. It just means that after this time has passed, when certain triggers fire (e.g. window focus) react-query will refetch the query in the background.

Will set it at 0 again. The rest of this PR can stay.

Copy link
Member

@oliviertassinari oliviertassinari Oct 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@Janpot Ok, then agree 10s sounds better than 1s, it could even be 1min 👍

Screenshot 2022-10-20 at 15 29 10

https://tanstack.com/query/v4/docs/guides/important-defaults

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding feature request that could help with invalidation: #1184

},
},
});
Expand Down Expand Up @@ -899,6 +906,12 @@ export default function ToolpadApp({

React.useEffect(() => setResetNodeErrorsKey((key) => key + 1), [dom]);

const { value: showDevtools, toggle: toggleDevtools } = useBoolean(false);

React.useEffect(() => {
(window as any).toggleDevtools = () => toggleDevtools();
}, [toggleDevtools]);

return (
<AppRoot ref={rootRef}>
<NoSsr>
Expand All @@ -918,6 +931,9 @@ export default function ToolpadApp({
<BrowserRouter basename={basename}>
<RenderedPages dom={dom} />
</BrowserRouter>
{showDevtools ? (
<ReactQueryDevtoolsProduction initialIsOpen={false} />
) : null}
</QueryClientProvider>
</AppContextProvider>
</ComponentsContext>
Expand Down
7 changes: 2 additions & 5 deletions packages/toolpad-app/src/runtime/useDataQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export async function execDataSourceQuery({

export type UseDataQueryConfig = Pick<
UseQueryOptions<any, unknown, unknown, any[]>,
'enabled' | 'refetchOnWindowFocus' | 'refetchOnReconnect' | 'refetchInterval'
'enabled' | 'refetchInterval'
>;

export interface UseFetch {
Expand All @@ -64,10 +64,7 @@ export function useDataQuery(
{
enabled = true,
...options
}: Pick<
UseQueryOptions<any, unknown, unknown, any[]>,
'enabled' | 'refetchOnWindowFocus' | 'refetchOnReconnect' | 'refetchInterval'
>,
}: Pick<UseQueryOptions<any, unknown, unknown, any[]>, 'enabled' | 'refetchInterval'>,
): UseFetch {
const { appId, version } = useAppContext();
const { savedNodes } = React.useContext(CanvasHooksContext);
Expand Down
48 changes: 0 additions & 48 deletions packages/toolpad-app/src/toolpad/AppEditor/PageEditor/QueryEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ import {
List,
ListItem,
DialogActions,
Checkbox,
FormControlLabel,
TextField,
InputAdornment,
Divider,
Expand Down Expand Up @@ -261,19 +259,6 @@ function QueryNodeEditorDialog<Q>({
);
}, []);

const handleRefetchOnWindowFocusChange = React.useCallback(
(event: React.ChangeEvent<HTMLInputElement>) => {
setInput((existing) =>
update(existing, {
attributes: update(existing.attributes, {
refetchOnWindowFocus: appDom.createConst(event.target.checked),
}),
}),
);
},
[],
);

const handleEnabledChange = React.useCallback((newValue: BindableAttrValue<boolean> | null) => {
setInput((existing) =>
update(existing, {
Expand All @@ -284,19 +269,6 @@ function QueryNodeEditorDialog<Q>({
);
}, []);

const handleRefetchOnReconnectChange = React.useCallback(
(event: React.ChangeEvent<HTMLInputElement>) => {
setInput((existing) =>
update(existing, {
attributes: update(existing.attributes, {
refetchOnReconnect: appDom.createConst(event.target.checked),
}),
}),
);
},
[],
);

const handleRefetchIntervalChange = React.useCallback(
(event: React.ChangeEvent<HTMLInputElement>) => {
const interval = Number(event.target.value);
Expand Down Expand Up @@ -419,26 +391,6 @@ function QueryNodeEditorDialog<Q>({
onChange={handleEnabledChange}
disabled={mode !== 'query'}
/>
<FormControlLabel
control={
<Checkbox
checked={input.attributes.refetchOnWindowFocus?.value ?? true}
onChange={handleRefetchOnWindowFocusChange}
disabled={mode !== 'query'}
/>
}
label="Refetch on window focus"
/>
<FormControlLabel
control={
<Checkbox
checked={input.attributes.refetchOnReconnect?.value ?? true}
onChange={handleRefetchOnReconnectChange}
disabled={mode !== 'query'}
/>
}
label="Refetch on network reconnect"
/>
<TextField
InputProps={{
startAdornment: <InputAdornment position="start">s</InputAdornment>,
Expand Down
2 changes: 1 addition & 1 deletion packages/toolpad-app/src/utils/useBoolean.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as React from 'react';

export default function useBoolean(initialValue: boolean) {
const [value, setValue] = React.useState(initialValue);
const toggle = React.useCallback(() => setValue((existing) => !!existing), []);
const toggle = React.useCallback(() => setValue((existing) => !existing), []);
const setTrue = React.useCallback(() => setValue(true), []);
const setFalse = React.useCallback(() => setValue(false), []);
return { value, setValue, toggle, setTrue, setFalse };
Expand Down
23 changes: 22 additions & 1 deletion yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2596,11 +2596,27 @@
resolved "https://registry.yarnpkg.com/@swc/wasm/-/wasm-1.3.6.tgz#01bae6087bb2458b23c29a5a8ad914c83ec11618"
integrity sha512-rFygmNDMl25/t2ETAtFjpcw6acQOm/o4sW/GN0fVPFUdNpI2zr2/oCXpyRM71OUPbXvksy9jXrt7yMZGD65+wQ==

"@tanstack/match-sorter-utils@^8.1.1":
version "8.5.14"
resolved "https://registry.yarnpkg.com/@tanstack/match-sorter-utils/-/match-sorter-utils-8.5.14.tgz#12efcd536abe491d09521e0242bc4d51442f8a8a"
integrity sha512-lVNhzTcOJ2bZ4IU+PeCPQ36vowBHvviJb2ZfdRFX5uhy7G0jM8N34zAMbmS5ZmVH8D2B7oU82OWo0e/5ZFzQrw==
dependencies:
remove-accents "0.4.2"

"@tanstack/query-core@4.10.3":
version "4.10.3"
resolved "https://registry.yarnpkg.com/@tanstack/query-core/-/query-core-4.10.3.tgz#a6477bab9ed1ae4561ca0a59ae06f8c615c692b7"
integrity sha512-+ME02sUmBfx3Pjd+0XtEthK8/4rVMD2jcxvnW9DSgFONpKtpjzfRzjY4ykzpDw1QEo2eoPvl7NS8J5mNI199aA==

"@tanstack/react-query-devtools@^4.12.0":
version "4.12.0"
resolved "https://registry.yarnpkg.com/@tanstack/react-query-devtools/-/react-query-devtools-4.12.0.tgz#18f341185f0a5580fc8b2daf8197389108be7131"
integrity sha512-gUV+aKpwgP7Cfp2+vwgu6krXCytncGWjTqFcnzC1l2INOf8dRi8/GEupYF7npxD9ky72FTuxc5+TI/lC8eB0Eg==
dependencies:
"@tanstack/match-sorter-utils" "^8.1.1"
superjson "^1.10.0"
use-sync-external-store "^1.2.0"

"@tanstack/react-query@^4.10.3":
version "4.10.3"
resolved "https://registry.yarnpkg.com/@tanstack/react-query/-/react-query-4.10.3.tgz#294deefa0fb6ada88bc4631d346ef5d5551c5443"
Expand Down Expand Up @@ -10502,6 +10518,11 @@ regexpp@^3.2.0:
resolved "https://registry.yarnpkg.com/regexpp/-/regexpp-3.2.0.tgz#0425a2768d8f23bad70ca4b90461fa2f1213e1b2"
integrity sha512-pq2bWo9mVD43nbts2wGv17XLiNLya+GklZ8kaDLV2Z08gDCsGpnKn9BFMepvWuHCbyVvY7J5o5+BVvoQbmlJLg==

remove-accents@0.4.2:
version "0.4.2"
resolved "https://registry.yarnpkg.com/remove-accents/-/remove-accents-0.4.2.tgz#0a43d3aaae1e80db919e07ae254b285d9e1c7bb5"
integrity sha512-7pXIJqJOq5tFgG1A2Zxti3Ht8jJF337m4sowbuHsW30ZnkQFnDzy9qBNhgzX8ZLW4+UBcXiiR7SwR6pokHsxiA==

require-directory@^2.1.1:
version "2.1.1"
resolved "https://registry.yarnpkg.com/require-directory/-/require-directory-2.1.1.tgz#8c64ad5fd30dab1c976e2344ffe7f792a6a6df42"
Expand Down Expand Up @@ -11247,7 +11268,7 @@ sucrase@^3.20.0, sucrase@^3.28.0:
pirates "^4.0.1"
ts-interface-checker "^0.1.9"

superjson@^1.10.1:
superjson@^1.10.0, superjson@^1.10.1:
version "1.10.1"
resolved "https://registry.yarnpkg.com/superjson/-/superjson-1.10.1.tgz#9c73e9393489dddab89d638694eadcbf4bda2f36"
integrity sha512-7fvPVDHmkTKg6641B9c6vr6Zz5CwPtF9j0XFExeLxJxrMaeLU2sqebY3/yrI3l0K5zJ+H9QA3H+lIYj5ooCOkg==
Expand Down