Skip to content

refactor: [M3-8462] - Upgrade to TanStack Query v5#10804

Merged
bnussman-akamai merged 7 commits intolinode:developfrom
bnussman-akamai:M3-8462-upgrade-to-tanstack-query-v5
Aug 29, 2024
Merged

refactor: [M3-8462] - Upgrade to TanStack Query v5#10804
bnussman-akamai merged 7 commits intolinode:developfrom
bnussman-akamai:M3-8462-upgrade-to-tanstack-query-v5

Conversation

@bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Aug 19, 2024

Description 📝

  • Finishes updating React Query (aka TanStack Query) to latest 📦
  • Fixes deprecations according to this guide

Main Changes 🔁

  • Mutations use isPending instead of isLoading to track if a mutation is executing
  • The option keepPreviousData (an option we use to prevent flickering on paginated endpoints) is no longer a useQuery option. We now use must use placeholderData with the keepPreviousData helper.
  • initialPageParam is now a required option on Infinite Queries

Preview 📷

Screenshot 2024-08-19 at 6 32 32 PM

How to test 🧪

  • Check the diff for anything suspicious 🔍
  • Perform general create/update/delete test for various features of Cloud Manager 🧪
  • Verify all automated testing passes ✅

As an Author I have considered 🤔

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@bnussman-akamai bnussman-akamai added Dependencies Pull requests that update a dependency file React Query Relating to the transition to use React Query Ready for Review labels Aug 19, 2024
@bnussman-akamai bnussman-akamai self-assigned this Aug 19, 2024
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner August 19, 2024 22:32
@bnussman-akamai bnussman-akamai requested review from dwiley-akamai and hana-akamai and removed request for a team August 19, 2024 22:32
Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

Thank you for the effort in doing this 🚀

return page + 1;
},
keepPreviousData: true,
initialPageParam: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use placeholderData here? I'll test this part of the Parent/Child code shortly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we need keepPreviousData / placeholderData on an infinite query, but I will double check this.

headers: options.headers,
params: {
page: pageParam,
page: pageParam as number,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Couldn't we type pageParam as queryFn: ({ pageParam }: { pageParam: number }) =>? I'm kinda interested why it's defined as unknown

Copy link
Member Author

@bnussman-akamai bnussman-akamai Aug 23, 2024

Choose a reason for hiding this comment

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

Yeah, that would also work. I can't really find a way to make pageParam any more type-safe. It seems like with either solution, it can't be strongly typed in the factory or in the query 🫤

}
}, [query.error]);

return query;
Copy link
Contributor

Choose a reason for hiding this comment

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

cc: @venkymano for transparency - just letting you know of the change

@github-actions
Copy link

github-actions bot commented Aug 20, 2024

Coverage Report:
Base Coverage: 86.08%
Current Coverage: 86.04%

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Still a bit more testing and a few more files I want to take a closer look, but it's looking good so far:

CRUD operations for Linodes, Volumes, VPC, Firewalls, Domains, Databases, Kubernetes ✅


// Update the user's grants directly in the cache
this.props.queryClient.setQueriesData<Grants>(
this.props.queryClient.setQueryData<Grants>(
Copy link
Contributor

Choose a reason for hiding this comment

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

From here:

Screenshot 2024-08-21 at 11 42 26 AM

Is the reason for shifting to setQueryData that we're updating a single query and not matching the query key partially?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah exactly. I think setQueriesData was accidentally used here

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Aug 22, 2024
@bnussman-akamai bnussman-akamai merged commit 2e82f16 into linode:develop Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved Multiple approvals and ready to merge! Dependencies Pull requests that update a dependency file React Query Relating to the transition to use React Query

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants