Skip to content

rename cacheTime #4678

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

Closed
TkDodo opened this issue Dec 23, 2022 · 7 comments
Closed

rename cacheTime #4678

TkDodo opened this issue Dec 23, 2022 · 7 comments
Labels
question Further information is requested v5
Milestone

Comments

@TkDodo
Copy link
Collaborator

TkDodo commented Dec 23, 2022

context

Almost everyone gets cacheTime wrong (exhibit A). It sounds like "the amount of time that data is cached for", but that is not correct.

cacheTime does nothing as long as a query is still in used. It only kicks in as soon as the query becomes unused. After the time has passed, data will be "garbage collected" to avoid the cache from growing (see also this explanation).

RTK Query has the same feature - their prop is called keepUnusedDataFor. I think this is quite descriptive but also a bit long.

proposal

  • rename cacheTime to gcTime

gc is referring to "garbage collect" time. It's a bit more technical, but also a quite well known abbreviation in computer science.

Also, it is not something that most people will have to customize. The default of 5 minutes is usually fine. A rename will reduce the chance that this property is mixed up with staleTime.

Lastly, if someone doesn't immediately know what gcTime stands for, they will (hopefully) consult the docs. This is a lot better than thinking they know what cacheTime does.

Here is an old discussion on that topic:

@TkDodo TkDodo added the v5 label Dec 23, 2022
@TkDodo TkDodo added this to the v5 milestone Dec 23, 2022
@TkDodo TkDodo added the question Further information is requested label Dec 23, 2022
@incepter
Copy link
Contributor

what about making it totally configurable ?:

{
  keepDataAfterDispose: boolean,
  keepDataAfterDisposeDurationMs: number,
}

I dislike long names, but couldn't think of better words

@TkDodo
Copy link
Collaborator Author

TkDodo commented Jan 3, 2023

We had some discussions internally about renaming staleTime alongside cacheTime so that they both correlate better. The idea was to go:

staleTime -> maxAge
cacheTime -> gcMaxAge

One advantage of this would be that the naming would be in-line with how the new TanStack router names those properties.

I think there are a couple of reasons why renaming staleTime would not be ideal:

  1. I don't think there are lots of cases where people misunderstand staleTime. Data can be fresh or stale, and staleTime defines when it goes stale. We also use stale-while-revalidate to show stale data while we refetch in the background, so this fits well.
  2. We use the naming stale a lot in other places in TanStack Query:
  • the devtools show queries as stale
  • the query filters allow for matching all stale queries by passing stale:true
  • useQuery returns an isStale boolean to indicate that the data is stale
  1. maxAge (the Cache-Control header) is in seconds, while our staleTime is in milliseconds. I think this could lead to confusion for new starters.
  2. compared to the Cache-Control header, if you only specify maxAge, you will not get the cached response after that time. That is not what happens with staleTime: You will always get the cached data, even if stale. To achieve the same thing with the Cache-Control header, you would further have to set the stale-while-revalidate header. This means that naming our staleTime same as maxAge could be seen as confusing because it doesn't do the same thing.

This leads me to the conclusion that we should leave staleTime as-is, and should only consider renaming cacheTime. The two candidates are (with their advantages and drawbacks):

  • inactiveCacheTime
    ✅ reads as: how long inactive queries are going to be cached, which is exactly what it does
    ✅ we already use the term inactive for the devtools and the queryFilters for this case
    ✅ if you type cacheTime (the old name), TypeScript would suggest inactiveCacheTime for you, so it's very discoverable
    ❌ longer name
    ❓ could be misread as "inactiveCache" time, like "the time for the inactive cache" (?). There is no such thing so maybe that isn't a problem

  • gcTime
    ✅ reads as: time until queries will be garbage collected.
    ✅ shorter name
    gc is an abbreviation (even though a well known one)
    ❌ not as discoverable

@trogers1
Copy link

trogers1 commented Jan 5, 2023

FWIW My team has definitely misinterpreted cacheTime due to just assuming they know what it means rather than referring to the docs, particularly when newer devs are learning react-query for the first time. So I think this is a great idea when it comes to teaching/learning react-query.

I would personally recommend removing 'cache' from the name of this particular property altogether to avoid the same aforementioned problem. To that end, my vote would be for gcTime, given the two options. Additionally, gcTime is still discoverable for someone looking for the cacheTime they may remember after they try cache and fail if they decide to search with time:

image

I believe this is something a reasonable person could conceivably do if they had been exposed to the API before and remembered that ___Time was the basic format for these sorts of properties (i.e. staleTime)...

@Mamoanwar97
Copy link

I will vote for inactiveCacheTime and I can work on it when you decide what will be the final name

@DamianOsipiuk
Copy link
Contributor

I would go for gcTime because as you have mentioned it's a well known abbreviation of removing stuff from memory.
inactiveCacheTime seems like still leaving space for interpretation.

@janaagaard75
Copy link

I think the confusion stems from people guessing / assuming how React Query works. For me, the hard part was understanding the concept of how React Query will use stale data to render a view, and then refresh that data in the background, finally updating the view, if the updated data is different. I think it would have helped with some exampled with timelines, depicting the different situations.

@toFrankie
Copy link

👀

dhess added a commit to hackworthltd/primer-app that referenced this issue Apr 10, 2024
This version has some fairly major changes, but most of it is hidden
from us thanks to Orval. We have made the following notable changes,
however:

* `cacheTime` has been renamed to `gcTime`. As far as I can tell,
there are no behavioral changes and this is simply a rename to reduce
confusion. Therefore, this change should be purely cosmetic. (See
TanStack/query#4678,
TanStack/query#1217, and
TanStack/query#4829 for reference.)

* Use object-style parameters to query methods where necessary. These
changes should be purely cosmetic.

* React Query v5 is written in TypeScript, so we can take advantage of
better types. The one place we currently do that is in the `Edit`
component's `useGetProgram` return types. Note that we also use
`isPending` rather than `isLoading`. The docs are a bit confusing on
this point as to whether they're equivalent; see
https://tanstack.com/query/latest/docs/framework/react/guides/migrating-to-v5#status-loading-has-been-changed-to-status-pending-and-isloading-has-been-changed-to-ispending-and-isinitialloading-has-now-been-renamed-to-isloading.
Initially I used `isLoading` when porting to v5, but TypeScript was
not able to narrow the `data` value to non-nil unless I use
`isPending`, so I've gone with `isPending` in the end. As far as I can
tell, it works the same as before.

* v5's dev tools are significantly improved, including the UI, which
lets you choose via a nice drop-down menu where to locate the tool
pop-up. This doesn't play well with our own "dev tools" checkbox UI,
so I've temporarily disabled those `DevOptions`, as the UI for them
will need to be rethought. I think adding another button to the canvas
is probably the way to go, but I'll leave that work for later.

FYI, the migration guide is here, and it informed most of these
changes:

https://tanstack.com/query/latest/docs/framework/react/guides/migrating-to-v5

Signed-off-by: Drew Hess <[email protected]>
dhess added a commit to hackworthltd/primer-app that referenced this issue Apr 10, 2024
This version has some fairly major changes, but most of it is hidden
from us thanks to Orval. We have made the following notable changes,
however:

* `cacheTime` has been renamed to `gcTime`. As far as I can tell,
there are no behavioral changes and this is simply a rename to reduce
confusion. Therefore, this change should be purely cosmetic. (See
TanStack/query#4678,
TanStack/query#1217, and
TanStack/query#4829 for reference.)

* Use object-style parameters to query methods where necessary. These
changes should be purely cosmetic.

* React Query v5 is written in TypeScript, so we can take advantage of
better types. The one place we currently do that is in the `Edit`
component's `useGetProgram` return types. Note that we also use
`isPending` rather than `isLoading`. The docs are a bit confusing on
this point as to whether they're equivalent; see
https://tanstack.com/query/latest/docs/framework/react/guides/migrating-to-v5#status-loading-has-been-changed-to-status-pending-and-isloading-has-been-changed-to-ispending-and-isinitialloading-has-now-been-renamed-to-isloading.
Initially I used `isLoading` when porting to v5, but TypeScript was
not able to narrow the `data` value to non-nil unless I use
`isPending`, so I've gone with `isPending` in the end. As far as I can
tell, it works the same as before.

* v5's dev tools are significantly improved, including the UI, which
lets you choose via a nice drop-down menu where to locate the tool
pop-up. This doesn't play well with our own "dev tools" checkbox UI,
so I've temporarily disabled those `DevOptions`, as the UI for them
will need to be rethought. I think adding another button to the canvas
is probably the way to go, but I'll leave that work for later.

FYI, the migration guide is here, and it informed most of these
changes:

https://tanstack.com/query/latest/docs/framework/react/guides/migrating-to-v5

Signed-off-by: Drew Hess <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested v5
Projects
None yet
Development

No branches or pull requests

7 participants