-
Notifications
You must be signed in to change notification settings - Fork 2k
ES site list: preloads overview page (and Site object) when link is in view
#107452
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
base: trunk
Are you sure you want to change the base?
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
packages/api-queries/src/site.ts
Outdated
| const site = await fetchSite( siteSlug ); | ||
| queryClient.setQueryData( [ 'site-by-id', site.ID, SITE_FIELDS, SITE_OPTIONS ], site ); | ||
| return site; |
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 feels kinda hacky, but queries don't have onSuccess callbacks (by design).
Another option would be to do this just in the overview page preloading code
| if ( preload ) { |
Then it would only work in the specific overview page case. But would maybe be less hacky?
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.
How if we use the initialData to get the cache from another query?
initialData: () => {
const cachedSites = queryClient.getQueriesData(['site-by-id']);
return cachedSites
.map(([key, data]) => data)
.find(site => site.slug === siteSlug);
},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 read through the initialData docs and this looks like the perfect use case!
I've also made sure to set the initialDataUpdatedAt field. Otherwise, the siteByIdQuery might immediately fetch the site by ID anyway, even when it's not needed. Setting initialDataUpdatedAt means that TanStack will know how long ago the data was fetched and whether it should update it.
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 still don't get it. Why do we need / why are we able to set the initial data of siteBySlugQuery() from siteByIdQuery()?. When we open the Site List, we don't populate siteByIdQuery() at all. So I'm not sure how this initialData logic is helpful here 🤔
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.
When we open the Site List, we don't populate siteByIdQuery() at all. So I'm not sure how this initialData logic is helpful here 🤔
We populate it by preload="viewport". With this setting, we preload the link when it's in the viewport so the siteByIdQuery will be pre-warmed
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 but it will preload the siteBySlugQuery(), not siteByIdQuery(), no? Or could you point me to the code/logic that does that.
I tested by removing the initialData and initialDataUpdatedAt and everything seems to work 🤔
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 this is mainly to ensure we can share the cache if one of them has already preloaded it. If any part of the app needs siteByIdQuery, we won’t need to request it again as long as the cached data hasn’t expired.
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.
Ohhh sorry I get it now. This logic is present in both of the queries 🤦♂️, I missed it.
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 way to think about it is that we are replacing the old optimisation, but we are doing it in quite a different way. The main thing is to ensure the navigation from the site list to the site-specific views is seamless.
Previously it was more about proactively pre-filling the site-by-slug and site-by-id query caches. Now the site-by-slug cache just happens to get pre-filled, but it is more declarative, we're just declaring that we would like to preload the overview link. And as you point out, site-by-id isn't really being pre-fetched at all. We're more lazily sharing the cached data. But the effect is the same—we're making sure the Site object is available to use immediately if we happen to have it locally.
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~236 bytes added 📈 [gzipped]) Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~350 bytes added 📈 [gzipped]) Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. 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. |
e8ff97b to
afc98ad
Compare
| render( <SiteLogs logType={ initialLogType } /> ); | ||
|
|
||
| // Wait for data and TabPanel to render | ||
| await waitFor( () => expect( nock.isDone() ).toBe( true ) ); |
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.
Removing these network checks to get the tests passing. The new initialData means the network call is no longer needed.
But we shouldn't need to wait explicitly for network calls to finish. We're already using findBy* queries, which have a wait built in: https://testing-library.com/docs/dom-testing-library/api-async#findby-queries
Waiting for UI elements that the user would see is better.
| initialDataUpdatedAt: () => { | ||
| const site = getFromCache(); | ||
| if ( site?.ID ) { | ||
| return queryClient.getQueryState( [ 'site-by-id', site.ID, SITE_FIELDS, SITE_OPTIONS ] ) |
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.
Maybe this?
| return queryClient.getQueryState( [ 'site-by-id', site.ID, SITE_FIELDS, SITE_OPTIONS ] ) | |
| return queryClient.getQueryState( siteByIdQuery( site.ID ).queryKey ) |
| // Used to find an existing Site object which is already in the `site-by-id` cache. | ||
| const getFromCache = () => | ||
| queryClient | ||
| .getQueriesData< Site >( { queryKey: [ 'site-by-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.
I think this must be exactly this otherwise it might get stale object whenever we update SITE_FIELDS or SITE_OPTIONS (not sure)
| .getQueriesData< Site >( { queryKey: [ 'site-by-id' ] } ) | |
| .getQueriesData< Site >( siteByIdQuery( site.ID ) ) |
| // Used to find an existing Site object which is already in the `site-by-slug` cache. | ||
| const getFromCache = () => | ||
| queryClient | ||
| .getQueriesData< Site >( { queryKey: [ 'site-by-slug' ] } ) |
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.
Similar comment here.
| initialDataUpdatedAt: () => { | ||
| const site = getFromCache(); | ||
| if ( site?.ID ) { | ||
| return queryClient.getQueryState( [ 'site-by-slug', site.slug, SITE_FIELDS, SITE_OPTIONS ] ) |
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.
Similar comment here.
Fixes DOTMSD-913
Proposed Changes
preload=viewportto the site list link. This will cause theSiteobject to get primed in the query cachesite-by-idandsite-by-slugcaches are primed.Why are these changes being made?
Before elastic search, this optimisation of pre-filling the
site-by-slugcache was done when the/me/sitesendpoint was loaded.wp-calypso/client/dashboard/sites/index.tsx
Lines 243 to 247 in c685488
This worked because we knew the
/me/sitesand/sites/%sresponse objects had the same format. But with ES we know the site list is now backed by site objects that don't match theSitedata type.Siteis still the definitive type for representing a site in the MSD, so we should make sure the site list loads it.We only preload when the link is in "viewport" because the user could have a large page size. Preloading all the
Siteobjects wouldn't be that bad if the user keeps the page size small.CC @arthur791004 since I know you've been thinking about this optimisation.
Testing Instructions
/sites?flags=dashboard/v2/es-site-listREACT_QUERY_OFFLINE_CACHElocal storagePre-merge Checklist