-
Notifications
You must be signed in to change notification settings - Fork 365
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
refactor: [M3-7440] - Upgrade to TanStack Query v4 #10236
refactor: [M3-7440] - Upgrade to TanStack Query v4 #10236
Conversation
Coverage Report: ❌ |
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.
Thanks for tackling this upgrade. It looks like there is now a TanStack Query v5 -- would it be worth upgrading to v5 now?
@@ -242,7 +242,7 @@ export const DatabaseSummaryConnectionDetails = (props: Props) => { | |||
<Typography> | |||
<span>password</span> = {password} | |||
</Typography> | |||
{credentialsLoading ? ( |
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.
Is this a related change?
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 like if a query is mounted with enabled: false
, isLoading
is now true
. Previously, it was false
. We'll have to look out for any other places where this braking change causes issues.
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.
Huh that's a curious implementation decision from the library.
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.
Are changes in this file related?
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.
They are an attempt to fix the existing flake caused by this test. I figured it wouldn't hurt.
@@ -90,13 +90,13 @@ const isValidRFC1918IPv4 = (address: string) => { | |||
// 192.168.x.x (192.168/16 prefix) | |||
return ( | |||
// check for valid 10.x IPs | |||
// check for valid 192.x IPs |
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.
Hmm.. this might have been accidental
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 think the pre-commit hooks did this.
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.
Should the comment be moved back to the original location?
Nvm, I just saw your comment on #10207. I'm good with moving to v4 first. |
We won't do v5 just yet because it has even more breaking changes. I think the logical order for us will be
|
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.
Thanks for handling this upgrade! Did some local testing but overall am satisfied with E2E tests passing. The single failure looks unrelated.
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.
Ran tests, reviewed changes and tested the application locally. All good from my vantage point ✅
Thanks for the update, excited to see our query key factory implemented as a follow up! 🎉
Description 📝
query-key-factory
isn't compatible with legacy React Queryreact-query
to@tanstack/react-query
queryKey
s must be arrays (notstring
)Preview 📷
Note
No UI changes expected
How to test 🧪
queryKey
s don't cause functionality regressionsAs an Author I have considered 🤔