-
Notifications
You must be signed in to change notification settings - Fork 3
Remove @lukemorales/query-key-factory, unblock react-query v5 upgrade
#2040
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
Conversation
| list: (params: ArticleListRequest) => [...articleKeys.listRoot(), params], | ||
| detailRoot: () => [...articleKeys.root, "detail"], | ||
| detail: (id: number) => [...articleKeys.detailRoot(), id], | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The structure above (and in other queries files) is based on https://tkdodo.eu/blog/effective-react-query-keys#use-query-key-factories (blog by one of the maintainers).
Still a "key factory", but a little less automated than it was with @lukemorales/query-key-factory.
The main downside is that, in contrast to @lukemorales/query-key-factory, we are now responsible for ensuring the keys are unique.
| ({ | ||
| queryKey: articleKeys.list(params), | ||
| queryFn: () => articlesApi.articlesList(params).then((res) => res.data), | ||
| }) satisfies QueryOptions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't updated to v5 in this PR, but once we updated to v5 (which should be easy now that @lukemorales/query-key-factory is gone), all these satisfies QueryOptions should be replaced with https://tanstack.com/query/latest/docs/framework/react/reference/queryOptions
@lukemorales/query-key-factory, unblock react-query v5 upgrade
jonkafton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and is actually a cleaner API that we had previously (loses ._ctx and ._def).
I'd consider an alternative along the lines of below, though I appreciate it may be better to alway call a function for the keys regardless of whether it accepts params.
const ROOT_KEY: RootKey = "articles"
const articleKeys = {
root: [ROOT_KEY],
listRoot: [ROOT_KEY, "list"],
list: (params: ArticleListRequest) => [ROOT_KEY, "list", params],
detailRoot: [ROOT_KEY, "detail"],
detail: (id: number) => [ROOT_KEY, "detail", id],
};
I'm not massively seeing the value of a key factory layer inside of the query hooks, which could directly create their keys and call the APIs. Convenient and a safer way for retrieving keys for clearing cache across files perhaps, though perhaps when we upgrade react-query we can find a pattern that better encapsulates this logic without exporting keys.
All works great.
What are the relevant tickets?
Prerequisite for #1940
Description (What does it do?)
This PR removes
@lukemorales/query-key-factory, which we had been using to create query configurations for@tanstack/react-query. However:@tanstack/react-querywas released about 1.5 years ago and has a number of improvements. A long open issi@lukemorales/query-key-factorydoes not support v5, and the open issue Support for TanStack Query v5 lukemorales/query-key-factory#73 to add support shows no progress. A PR to add support was closed. I am not optimistic support will be added any time soon.query-key-factorymore or less works OK with v5 despite official support. However, it does lead to some TS issues that are very hard to resolve.How can this be tested?
The site should function exactly as before.
What I have checked is:
/dashboard/my-lists/, create / delete / edit userlist. The listing page should update appropriately.