Closed danielbachhuber closed 2 years ago
After my investigation, I've found that the Site Switcher is using 14 more fields. These extra fields add 331ms on average in my case with 94 sites.
The benefit of using useSiteExcerptsQuery
for both is that we could cache the same query for both components so we could show some data instantly from one component to another.
The downside is that for SMP, we end up making multiple requests to update the data. For example, when the user changes the tab. This is not needed with the "redux" approach.
From my point of view, it's not worth the migration to react query for the site switcher. The consequences would be we would load the /sites page slower when the user access directly, which will be very frequent if we make that page the main homepage of WPcom.
On the other side, we can certainly add a logic to avoid this overload on SMP and make requesting two different queries:
Using this last approach, the only downside would be that we add more complexity to the code/navigation.
I'm open to discussion. Here is the data I've used to make my conclusions:
Site Switcher Params
https://public-api.wordpress.com/rest/v1.2/me/sites?http_envelope=1&site_visibility=all&include_domain_only=true&site_activity=active&fields=ID%2CURL%2Ccapabilities%2Cicon%2Cis_multisite%2Cis_private%2Cis_coming_soon%2Cis_vip%2Cjetpack%2Cjetpack_modules%2Cname%2Coptions%2Cplan%2Cproducts%2Csingle_user_site%2Cvisible%2Clang%2Claunch_status%2Csite_migration%2Csite_owner%2Cis_core_site_editor_enabled%2Cis_wpcom_atomic%2Cdescription%2Cuser_interactions&options=admin_url%2Cadvanced_seo_front_page_description%2Cadvanced_seo_title_formats%2Callowed_file_types%2Canchor_podcast%2Ccreated_at%2Cdefault_comment_status%2Cdefault_ping_status%2Cdefault_post_format%2Cdesign_type%2Cediting_toolkit_is_active%2Cframe_nonce%2Cgmt_offset%2Chas_pending_automated_transfer%2Cis_automated_transfer%2Cis_cloud_eligible%2Cis_domain_only%2Cis_mapped_domain%2Cis_redirect%2Cis_wpcom_atomic%2Cis_wpcom_store%2Cis_wpforteams_site%2Cp2_hub_blog_id%2Cjetpack_frame_nonce%2Cjetpack_version%2Cmain_network_site%2Cpage_on_front%2Cpage_for_posts%2Cpodcasting_archive%2Cpodcasting_category_id%2Cpublicize_permanently_disabled%2Cshow_on_front%2Csite_segment%2Csoftware_version%2Ctimezone%2Cupdated_at%2Cupgraded_filetypes_enabled%2Cunmapped_url%2Cverification_services_codes%2Cvideopress_enabled%2Cwoocommerce_is_active%2Cwordads%2Csite_creation_flow%2Cis_difm_lite_in_progress%2Cdifm_lite_site_options%2Csite_intent%2Claunchpad_screen%2Claunchpad_checklist_tasks_statuses
['ID', 'URL', 'capabilities', 'icon', 'is_multisite', 'is_private', 'is_coming_soon', 'is_vip', 'jetpack', 'jetpack_modules', 'name', 'options', 'plan', 'products', 'single_user_site', 'visible', 'lang', 'launch_status', 'site_migration', 'site_owner', 'is_core_site_editor_enabled', 'is_wpcom_atomic', 'description', 'user_interactions']
SMP Params
https://public-api.wordpress.com/rest/v1.2/me/sites?http_envelope=1&site_visibility=all&include_domain_only=true&site_activity=active&fields=ID%2CURL%2Cis_coming_soon%2Cis_private%2Cvisible%2Claunch_status%2Cicon%2Cname%2Coptions%2Cplan%2Cjetpack%2Cis_wpcom_atomic%2Cuser_interactions&options=is_wpforteams_site%2Cupdated_at%2Cis_redirect%2Cunmapped_url%2Cadmin_url
['ID', 'URL', 'is_coming_soon', 'is_private', 'visible', 'launch_status', 'icon', 'name', 'options', 'plan', 'je']
Differences
Fields requested on Site Switcher and not in SMP:
['capabilities', 'is_multisite', 'is_vip', 'jetpack', 'jetpack_modules', 'products', 'single_user_site', 'lang', 'site_migration', 'site_owner', 'is_core_site_editor_enabled', 'is_wpcom_atomic', 'description', 'user_interactions']
Fields requested on SMP and not in Site Switcher:
['je']
I made 20 requests for each url using VSCode thunder client. I have 94 sites.
[1440, 1410, 1390, 1320, 1350, 1410, 1550, 1450, 1380, 1150, 1460, 1420, 1290, 1270, 1410, 1130, 1430, 1440, 1420, 1270]
[958, 1170, 1080, 943, 959, 965, 1180, 899, 995, 1250, 959, 919, 931, 933, 1180, 1140, 1320, 1030, 998, 963]
Thanks for the investigation, @sejas!
I made my research as well, and the results are interesting.
Field | Referenced in |
---|---|
single_user_site |
SiteSelector |
domain |
SiteSelector , Site |
primary |
SiteSelector , Site |
updates (not referenced by the getSite selector, must come from somewhere else) |
SiteIndicator |
title (computed, uses name and slug ) |
SiteSelector , Site |
canUpdateFiles (computed, uses options.is_multi_network and options.file_mod_disabled ) |
SiteIndicator |
options.woocommerce_is_active |
SiteSelector |
options.is_difm_lite_in_progress |
Site |
options.is_domain_only |
Site |
options.p2_hub_blog_id |
Site |
options.is_multi_network |
SiteIndicator |
options.file_mod_disabled |
SiteIndicator |
options.jetpack_connection_active_plugins |
SiteIndicator |
options.is_automated_transfer |
SiteIndicator |
I have opened the SiteSelector
, Site
and SiteIndicator
components looking for site.*
references and the selectors used in the Redux-connect HOC.
With that, I've found the fields that are used in those three components but not included in the site excerpts query (table above).
With the help of @sejas, I've managed to get the original excerpts query URL and compared it with the same URL, but with the additional fields above.
Query | 10 reqs. mean |
---|---|
Original excerpts | 979.8ms |
Excerpts with additional fields | 992.9ms |
About ~1.5% loading time increase.
There's an almost negligible penalty for adding those fields and (with the addition) it still would be around 300ms
faster than the complete sites query.
My main question is: is there any value in doing that kind of work? The site switcher is, almost always, used in conjunction with Calypso. Calypso uses all data request, and the Site Switcher is closed on mount, so the user won't perceive this improvement.
I realize that there is an improvement when opening the site switcher in isolation (i.e. https://wordpress.com/marketing/tools) or Jetpack Cloud, though. So we could make the Site Switcher work in a way that it uses Redux inside Calypso, but uses the excerpts query outside it.
If we're afraid of the excerpts query getting too big, we can allow the user to customize the list of fields passed by adding a hook argument letting them specify more fields as needed.
This does not include the modification in Site
and SiteIndicator
as ideally we should pass the whole site
object, with all necessary fields as a prop, instead of relying on the Redux store for selectors.
It removes the Redux connection in searchSites
HOC as all usages of it are already passing the sites
object anyway. It also maps the sites
prop to visibleSites
in SiteSelector
because that's how this component references the sites
array.
diff --git a/client/components/search-sites/index.js b/client/components/search-sites/index.js
index 1193bcb07f..5ff39ea55b 100644
--- a/client/components/search-sites/index.js
+++ b/client/components/search-sites/index.js
@@ -1,12 +1,6 @@
import { Component } from 'react';
-import { connect } from 'react-redux';
-import getSites from 'calypso/state/selectors/get-sites';
import { searchCollection } from './utils';
-const mapState = ( state ) => ( {
- sites: getSites( state ),
-} );
-
export default function searchSites( WrappedComponent ) {
const componentName = WrappedComponent.displayName || WrappedComponent.name || '';
@@ -33,7 +27,7 @@ export default function searchSites( WrappedComponent ) {
}
}
- const Connected = connect( mapState )( Searcher );
- Connected.displayName = `SearchSites(${ componentName })`;
- return Connected;
+ Searcher.displayName = `SearchSites(${ componentName })`;
+
+ return Searcher;
}
diff --git a/client/components/site-selector/index.jsx b/client/components/site-selector/index.jsx
index 9badb31d9e..fd80f7a780 100644
--- a/client/components/site-selector/index.jsx
+++ b/client/components/site-selector/index.jsx
@@ -15,6 +15,7 @@ import Site from 'calypso/blocks/site';
import SitePlaceholder from 'calypso/blocks/site/placeholder';
import Search from 'calypso/components/search';
import searchSites from 'calypso/components/search-sites';
+import { withSiteExcerpts } from 'calypso/data/sites/use-site-excerpts-query';
import scrollIntoViewport from 'calypso/lib/scroll-into-viewport';
import { addQueryArgs } from 'calypso/lib/url';
import allSitesMenu from 'calypso/my-sites/sidebar/static-data/all-sites-menu';
@@ -22,14 +23,12 @@ import { recordTracksEvent } from 'calypso/state/analytics/actions';
import { getCurrentUser } from 'calypso/state/current-user/selectors';
import { getPreference } from 'calypso/state/preferences/selectors';
import areAllSitesSingleUser from 'calypso/state/selectors/are-all-sites-single-user';
-import getSites from 'calypso/state/selectors/get-sites';
-import getVisibleSites from 'calypso/state/selectors/get-visible-sites';
+import { isSiteVisible } from 'calypso/state/selectors/get-visible-sites';
import hasLoadedSites from 'calypso/state/selectors/has-loaded-sites';
import { getSite, hasAllSitesList } from 'calypso/state/sites/selectors';
import { getSelectedSite } from 'calypso/state/ui/selectors';
import SiteSelectorAddSite from './add-site';
import { getUserSiteCountForPlatform, getUserVisibleSiteCountForPlatform } from './utils';
-
import './style.scss';
const ALL_SITES = 'ALL_SITES';
@@ -610,18 +609,17 @@ const navigateToSite =
}
};
-const mapState = ( state ) => {
+const mapState = ( state, ownProps ) => {
const user = getCurrentUser( state );
return {
hasLoadedSites: hasLoadedSites( state ),
- sites: getSites( state ),
showRecentSites: get( user, 'visible_site_count', 0 ) > 11,
recentSites: getPreference( state, 'recentSites' ),
siteCount: getUserSiteCountForPlatform( user ),
visibleSiteCount: getUserVisibleSiteCountForPlatform( user ),
selectedSite: getSelectedSite( state ),
- visibleSites: getVisibleSites( state ),
+ visibleSites: ownProps.sites.filter( isSiteVisible ),
allSitesSingleUser: areAllSitesSingleUser( state ),
hasAllSitesList: hasAllSitesList( state ),
};
@@ -630,5 +628,6 @@ const mapState = ( state ) => {
export default flow(
localize,
searchSites,
- connect( mapState, { navigateToSite, recordTracksEvent } )
+ connect( mapState, { navigateToSite, recordTracksEvent } ),
+ withSiteExcerpts
)( SiteSelector );
diff --git a/client/data/sites/use-site-excerpts-query.ts b/client/data/sites/use-site-excerpts-query.ts
index 434fe5d4dc..3b47056009 100644
--- a/client/data/sites/use-site-excerpts-query.ts
+++ b/client/data/sites/use-site-excerpts-query.ts
@@ -1,4 +1,6 @@
import config from '@automattic/calypso-config';
+import { createHigherOrderComponent } from '@wordpress/compose';
+import { createElement } from 'react';
import { useQuery } from 'react-query';
import { useStore } from 'react-redux';
import { getJetpackSiteCollisions, getUnmappedUrl } from 'calypso/lib/site/utils';
@@ -39,6 +41,17 @@ export const useSiteExcerptsQuery = () => {
} );
};
+export const withSiteExcerpts = createHigherOrderComponent( ( Component ) => {
+ return ( props ) => {
+ const { data } = useSiteExcerptsQuery();
+
+ return createElement( Component, {
+ ...props,
+ sites: data ?? [],
+ } );
+ };
+}, 'withSiteExcerpts' );
+
// This "null" check also does the type assertion that allows TypeScript to
// make strong guarantees about `t`.
function notNullish< T >( t: T | null | undefined ): t is T {
diff --git a/client/state/selectors/get-visible-sites.js b/client/state/selectors/get-visible-sites.js
index 28cab33858..cc4b6951bf 100644
--- a/client/state/selectors/get-visible-sites.js
+++ b/client/state/selectors/get-visible-sites.js
@@ -2,6 +2,8 @@ import { createSelector } from '@automattic/state-utils';
import getSitesItems from 'calypso/state/selectors/get-sites-items';
import { getSite } from 'calypso/state/sites/selectors';
+export const isSiteVisible = ( site ) => site.visible === true;
+
/**
* Get all visible sites
*
@@ -11,7 +13,7 @@ import { getSite } from 'calypso/state/sites/selectors';
export default createSelector(
( state ) =>
Object.values( getSitesItems( state ) )
- .filter( ( site ) => site.visible === true )
+ .filter( isSiteVisible )
.map( ( site ) => getSite( state, site.ID ) ),
( state ) => [ getSitesItems( state ) ]
);
Thanks for the great exploration, @sejas and @zaguiini !
I'm a little bit hesitant about adding so many extra fields to SITE_EXCERPT_REQUEST_FIELDS
and SITE_EXCERPT_REQUEST_OPTIONS
, given useSiteExcerptsQuery
was meant to be a slimmed down version of getSites()
.
Because this particular problem is low priority for Site Switcher i1
, I think we should simply capture our learnings to P2 and pause on implementation.
@zaguiini Can you turn this into a P2 post, and then close the issue once it's published? Thanks!
Created a P2 post: pdKhl6-Kq-p2.
Thanks @zaguiini ! Great summary 😊
The standard Redux
/rest/v1.2/me/sites
API request is quite slow for users with hundreds of sites. We could cut the request time in half if we could useuseSiteExcerptsQuery
.However,
useSiteExcerptsQuery
only includes a subset of the Redux data, so we need to evaluate whether it has all of the data we need for the Site Switcher.From pdKhl6-GX-p2
Related https://github.com/Automattic/wp-calypso/issues/67581
Done is:
useSiteExcerptsQuery
.useSiteExcerptsQuery
to the Site Switcher. If we do, we open a separate issue for implementation.