-
Notifications
You must be signed in to change notification settings - Fork 2k
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
profileLinks: migrate to @tanstack/react-query
#83968
base: trunk
Are you sure you want to change the base?
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~275 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~2426 bytes removed 📉 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~551 bytes removed 📉 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
8601d64
to
2c59e43
Compare
work in progress /2 working state
6514b6f
to
67f0607
Compare
I think this is generally ready for a first round of reviews. /cc @digitalwaveride |
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.
This PR covers a few migrations with :
- Migrate data fetching and mutations from our legacy data layer and Redux to using @tanstack/react-query. We follow the guideline to place the hooks alongside where they're used and therefore those can all be found in client/me/profile-links/data.
- Migrate any components to TypeScript
- Migrate any class components to function components
Seems like this PR is doing a bit too much. Could we split the PR to one PR per migration at least? I'd say that we could ideally do 1 PR per migration per file even, otherwise, we'll still be doing too much in a single PR. WDYT?
|
||
export type AddProfileLinksPayload = AddProfileLinkPayload[]; | ||
|
||
export type ProfileLinkResponse = ProfileLink[]; |
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.
Some of these types seem arbitrary and could be inlined at no cost. ProfileLink[]
is even shorter than ProfileLinkResponse
.
const [ data ] = args; | ||
|
||
queryClient.setQueryData( | ||
[ 'profile-links' ], |
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 we clearing this cache properly when the user logs out / logs in? Should we key them by user ID?
Proposed Changes
This PR covers a few migrations with
<ProfileLinks />
@tanstack/react-query
. We follow the guideline to place the hooks alongside where they're used and therefore those can all be found inclient/me/profile-links/data
.Previously we had a few different profile links related directories in
client/me
. I rearranged them to all be inclient/me/profile-links
. They're not used anywhere else and for me it makes more sense to have them contained in a single directory.While generally profile links should behave exactly the same we introduce some UX improvements:
busy
states in case a mutation is running (when adding or removing profile links)Testing Instructions
/me
and smoke test the profile links sectionIf you're wondering where those profile links show up after adding them, go to
https://gravatar.com/:wpcom-username
and you should see the list there.Pre-merge Checklist