From fa7e76325b3c3069d3337794c3f8d14f8bff139a Mon Sep 17 00:00:00 2001 From: Omar Alshaker Date: Wed, 11 Dec 2024 13:01:24 +0100 Subject: [PATCH 01/11] Fix Stepper lazy loading --- .../declarative-flow/internals/index.tsx | 21 +++++++++---------- .../stepper/utils/enhanceFlowWithAuth.ts | 5 ++++- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/client/landing/stepper/declarative-flow/internals/index.tsx b/client/landing/stepper/declarative-flow/internals/index.tsx index 69c67a33d609d..cf5a777919cb3 100644 --- a/client/landing/stepper/declarative-flow/internals/index.tsx +++ b/client/landing/stepper/declarative-flow/internals/index.tsx @@ -90,24 +90,23 @@ export const FlowRenderer: React.FC< { flow: Flow } > = ( { flow } ) => { // See https://github.com/Automattic/wp-calypso/pull/82981. const selectedSite = useSelector( ( state ) => site && getSite( state, siteSlugOrId ) ); - // this pre-loads all the lazy steps down the flow. + // this pre-loads the next step in the flow. useEffect( () => { + // Omit user step if user is logged in. + const steps = isLoggedIn ? flowSteps.filter( ( step ) => step.slug !== 'user' ) : flowSteps; + const nextStepIndex = steps.findIndex( ( step ) => step.slug === currentStepRoute ) + 1; + const nextStep = steps[ nextStepIndex ]; + if ( siteSlugOrId && ! selectedSite ) { // If this step depends on a selected site, only preload after we have the data. // Otherwise, we're still waiting to render something meaningful, and we don't want to // potentially slow that down by having the CPU busy initialising future steps. return; } - Promise.all( flowSteps.map( ( step ) => 'asyncComponent' in step && step.asyncComponent() ) ); - // Most flows sadly instantiate a new steps array on every call to `flow.useSteps()`, - // which means that we don't want to depend on `flowSteps` here, or this would end up - // running on every render. We thus depend on `flow` instead. - // - // This should be safe, because flows shouldn't return different lists of steps at - // different points. But even if they do, worst case scenario we only fail to preload - // some steps, and they'll simply be loaded later. - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ flow, siteSlugOrId, selectedSite ] ); + if ( nextStepIndex > 0 && nextStep && 'asyncComponent' in nextStep ) { + nextStep.asyncComponent(); + } + }, [ siteSlugOrId, selectedSite, currentStepRoute, flowSteps, isLoggedIn ] ); const stepNavigation = useStepNavigationWithTracking( { flow, diff --git a/client/landing/stepper/utils/enhanceFlowWithAuth.ts b/client/landing/stepper/utils/enhanceFlowWithAuth.ts index 278aa4cf4f59d..2e2c823bcf732 100644 --- a/client/landing/stepper/utils/enhanceFlowWithAuth.ts +++ b/client/landing/stepper/utils/enhanceFlowWithAuth.ts @@ -2,7 +2,10 @@ import type { Flow, StepperStep } from '../declarative-flow/internals/types'; const USER_STEP: StepperStep = { slug: 'user', - asyncComponent: () => import( '../declarative-flow/internals/steps-repository/__user' ), + asyncComponent: () => + import( + /* webpackChunkName: "stepper-user-step" */ '../declarative-flow/internals/steps-repository/__user' + ), }; function useInjectUserStepIfNeeded( flow: Flow ): StepperStep[] { From 9a21f0f78bb96a10b710d803a1311903d598c4a1 Mon Sep 17 00:00:00 2001 From: Omar Alshaker Date: Wed, 11 Dec 2024 13:37:41 +0100 Subject: [PATCH 02/11] Remove await from `geolocateCurrencySymbol` --- client/landing/stepper/index.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/client/landing/stepper/index.tsx b/client/landing/stepper/index.tsx index 5063d7805626d..a64fc56cfd99a 100644 --- a/client/landing/stepper/index.tsx +++ b/client/landing/stepper/index.tsx @@ -158,7 +158,7 @@ window.AppBoot = async () => { reduxStore.dispatch( setCurrentFlowName( flow.name ) ); reduxStore.dispatch( setSelectedSiteId( siteId ) as unknown as AnyAction ); - await geolocateCurrencySymbol(); + geolocateCurrencySymbol(); const root = createRoot( document.getElementById( 'wpcom' ) as HTMLElement ); @@ -179,7 +179,6 @@ window.AppBoot = async () => { /> - { 'development' === process.env.NODE_ENV && ( ) } From e2ae9bf9741b5d2e660fa8223b396c9a8e0f8adc Mon Sep 17 00:00:00 2001 From: Omar Alshaker Date: Wed, 11 Dec 2024 14:01:36 +0100 Subject: [PATCH 03/11] Add code comment --- client/landing/stepper/index.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/client/landing/stepper/index.tsx b/client/landing/stepper/index.tsx index a64fc56cfd99a..a2212327e4b29 100644 --- a/client/landing/stepper/index.tsx +++ b/client/landing/stepper/index.tsx @@ -158,6 +158,7 @@ window.AppBoot = async () => { reduxStore.dispatch( setCurrentFlowName( flow.name ) ); reduxStore.dispatch( setSelectedSiteId( siteId ) as unknown as AnyAction ); + // No need to await this, it's not critical to the boot process and will slow booting down. geolocateCurrencySymbol(); const root = createRoot( document.getElementById( 'wpcom' ) as HTMLElement ); From f4b79d38edf67174a2bfe337cf9cb113341389c3 Mon Sep 17 00:00:00 2001 From: Omar Alshaker Date: Wed, 11 Dec 2024 15:08:26 +0100 Subject: [PATCH 04/11] Load dashicons in masterbar only when logged in --- client/layout/masterbar/logged-in.scss | 1 + client/layout/masterbar/style.scss | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 client/layout/masterbar/logged-in.scss diff --git a/client/layout/masterbar/logged-in.scss b/client/layout/masterbar/logged-in.scss new file mode 100644 index 0000000000000..ceb54b16795d4 --- /dev/null +++ b/client/layout/masterbar/logged-in.scss @@ -0,0 +1 @@ +@import url( //s1.wp.com/wp-includes/css/dashicons.css?v=20150727 ); diff --git a/client/layout/masterbar/style.scss b/client/layout/masterbar/style.scss index b70c95ab173b0..a400a264120ef 100644 --- a/client/layout/masterbar/style.scss +++ b/client/layout/masterbar/style.scss @@ -8,7 +8,6 @@ $masterbar-color-secondary: #101517; @import "@wordpress/base-styles/breakpoints"; @import "@wordpress/base-styles/mixins"; @import "calypso/assets/stylesheets/shared/animation"; -@import url( //s1.wp.com/wp-includes/css/dashicons.css?v=20150727 ); @import "@automattic/typography/styles/variables"; // Hide the masterbar on WP Mobile App views. From a7080d047cb5417547ecb431dac3d63c47ef83cf Mon Sep 17 00:00:00 2001 From: Omar Alshaker Date: Wed, 11 Dec 2024 15:28:19 +0100 Subject: [PATCH 05/11] Actually import --- client/layout/masterbar/logged-in.jsx | 1 + 1 file changed, 1 insertion(+) diff --git a/client/layout/masterbar/logged-in.jsx b/client/layout/masterbar/logged-in.jsx index d38db1c067e14..8ac7567c350ec 100644 --- a/client/layout/masterbar/logged-in.jsx +++ b/client/layout/masterbar/logged-in.jsx @@ -63,6 +63,7 @@ import Item from './item'; import Masterbar from './masterbar'; import { MasterBarMobileMenu } from './masterbar-menu'; import Notifications from './masterbar-notifications/notifications-button'; +import './logged-in.scss'; const NEW_MASTERBAR_SHIPPING_DATE = new Date( 2022, 3, 14 ).getTime(); const MENU_POPOVER_PREFERENCE_KEY = 'dismissible-card-masterbar-collapsable-menu-popover'; From 30b4197b5510ce7dd3aec9ea17ef15f91fa263be Mon Sep 17 00:00:00 2001 From: Omar Alshaker Date: Wed, 11 Dec 2024 16:58:35 +0100 Subject: [PATCH 06/11] Fix effect dependencies --- .../stepper/declarative-flow/internals/index.tsx | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/client/landing/stepper/declarative-flow/internals/index.tsx b/client/landing/stepper/declarative-flow/internals/index.tsx index cf5a777919cb3..ab27c119e9efd 100644 --- a/client/landing/stepper/declarative-flow/internals/index.tsx +++ b/client/landing/stepper/declarative-flow/internals/index.tsx @@ -106,7 +106,15 @@ export const FlowRenderer: React.FC< { flow: Flow } > = ( { flow } ) => { if ( nextStepIndex > 0 && nextStep && 'asyncComponent' in nextStep ) { nextStep.asyncComponent(); } - }, [ siteSlugOrId, selectedSite, currentStepRoute, flowSteps, isLoggedIn ] ); + // Most flows sadly instantiate a new steps array on every call to `flow.useSteps()`, + // which means that we don't want to depend on `flowSteps` here, or this would end up + // running on every render. We thus depend on `flow` instead. + // + // This should be safe, because flows shouldn't return different lists of steps at + // different points. But even if they do, worst case scenario we only fail to preload + // some steps, and they'll simply be loaded later. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [ siteSlugOrId, selectedSite, currentStepRoute, flow, isLoggedIn ] ); const stepNavigation = useStepNavigationWithTracking( { flow, From 69cc7b516e1cf068c0cef627c82ebde843010dc2 Mon Sep 17 00:00:00 2001 From: Omar Alshaker Date: Wed, 11 Dec 2024 18:18:39 +0100 Subject: [PATCH 07/11] Discard changes to client/layout/masterbar/logged-in.jsx --- client/layout/masterbar/logged-in.jsx | 1 - 1 file changed, 1 deletion(-) diff --git a/client/layout/masterbar/logged-in.jsx b/client/layout/masterbar/logged-in.jsx index 8ac7567c350ec..d38db1c067e14 100644 --- a/client/layout/masterbar/logged-in.jsx +++ b/client/layout/masterbar/logged-in.jsx @@ -63,7 +63,6 @@ import Item from './item'; import Masterbar from './masterbar'; import { MasterBarMobileMenu } from './masterbar-menu'; import Notifications from './masterbar-notifications/notifications-button'; -import './logged-in.scss'; const NEW_MASTERBAR_SHIPPING_DATE = new Date( 2022, 3, 14 ).getTime(); const MENU_POPOVER_PREFERENCE_KEY = 'dismissible-card-masterbar-collapsable-menu-popover'; From 176a91297d2696cacbf05f5f076116e9f43e91c1 Mon Sep 17 00:00:00 2001 From: Omar Alshaker Date: Wed, 11 Dec 2024 18:18:47 +0100 Subject: [PATCH 08/11] Discard changes to client/layout/masterbar/style.scss --- client/layout/masterbar/style.scss | 1 + 1 file changed, 1 insertion(+) diff --git a/client/layout/masterbar/style.scss b/client/layout/masterbar/style.scss index a400a264120ef..b70c95ab173b0 100644 --- a/client/layout/masterbar/style.scss +++ b/client/layout/masterbar/style.scss @@ -8,6 +8,7 @@ $masterbar-color-secondary: #101517; @import "@wordpress/base-styles/breakpoints"; @import "@wordpress/base-styles/mixins"; @import "calypso/assets/stylesheets/shared/animation"; +@import url( //s1.wp.com/wp-includes/css/dashicons.css?v=20150727 ); @import "@automattic/typography/styles/variables"; // Hide the masterbar on WP Mobile App views. From ee4b585729ed04f0b7fd95bf67a358cf25e7b634 Mon Sep 17 00:00:00 2001 From: Omar Alshaker Date: Wed, 11 Dec 2024 18:32:46 +0100 Subject: [PATCH 09/11] Delete client/layout/masterbar/logged-in.scss --- client/layout/masterbar/logged-in.scss | 1 - 1 file changed, 1 deletion(-) delete mode 100644 client/layout/masterbar/logged-in.scss diff --git a/client/layout/masterbar/logged-in.scss b/client/layout/masterbar/logged-in.scss deleted file mode 100644 index ceb54b16795d4..0000000000000 --- a/client/layout/masterbar/logged-in.scss +++ /dev/null @@ -1 +0,0 @@ -@import url( //s1.wp.com/wp-includes/css/dashicons.css?v=20150727 ); From d387a8cdf2a4f8ad2da15e35feaad2cfa271c2ee Mon Sep 17 00:00:00 2001 From: Omar Alshaker Date: Wed, 11 Dec 2024 19:10:06 +0100 Subject: [PATCH 10/11] Fine-tune the user step --- .../stepper/declarative-flow/internals/index.tsx | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/client/landing/stepper/declarative-flow/internals/index.tsx b/client/landing/stepper/declarative-flow/internals/index.tsx index ab27c119e9efd..f1a6895152640 100644 --- a/client/landing/stepper/declarative-flow/internals/index.tsx +++ b/client/landing/stepper/declarative-flow/internals/index.tsx @@ -92,10 +92,8 @@ export const FlowRenderer: React.FC< { flow: Flow } > = ( { flow } ) => { // this pre-loads the next step in the flow. useEffect( () => { - // Omit user step if user is logged in. - const steps = isLoggedIn ? flowSteps.filter( ( step ) => step.slug !== 'user' ) : flowSteps; - const nextStepIndex = steps.findIndex( ( step ) => step.slug === currentStepRoute ) + 1; - const nextStep = steps[ nextStepIndex ]; + const nextStepIndex = flowSteps.findIndex( ( step ) => step.slug === currentStepRoute ) + 1; + const nextStep = flowSteps[ nextStepIndex ]; if ( siteSlugOrId && ! selectedSite ) { // If this step depends on a selected site, only preload after we have the data. @@ -103,7 +101,13 @@ export const FlowRenderer: React.FC< { flow: Flow } > = ( { flow } ) => { // potentially slow that down by having the CPU busy initialising future steps. return; } - if ( nextStepIndex > 0 && nextStep && 'asyncComponent' in nextStep ) { + if ( + // Don't load anything on user step because the user step will hard-navigate anyways. + currentStepRoute !== 'user' && + nextStepIndex > 0 && + nextStep && + 'asyncComponent' in nextStep + ) { nextStep.asyncComponent(); } // Most flows sadly instantiate a new steps array on every call to `flow.useSteps()`, From cf057876db486343b2c393aeaef02107922cb418 Mon Sep 17 00:00:00 2001 From: Omar Alshaker Date: Thu, 12 Dec 2024 13:36:32 +0100 Subject: [PATCH 11/11] Address feedback --- .../landing/stepper/declarative-flow/internals/index.tsx | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/client/landing/stepper/declarative-flow/internals/index.tsx b/client/landing/stepper/declarative-flow/internals/index.tsx index f1a6895152640..aef885c78b77d 100644 --- a/client/landing/stepper/declarative-flow/internals/index.tsx +++ b/client/landing/stepper/declarative-flow/internals/index.tsx @@ -95,6 +95,11 @@ export const FlowRenderer: React.FC< { flow: Flow } > = ( { flow } ) => { const nextStepIndex = flowSteps.findIndex( ( step ) => step.slug === currentStepRoute ) + 1; const nextStep = flowSteps[ nextStepIndex ]; + // 0 implies the findIndex returned -1. + if ( nextStepIndex === 0 || ! nextStep ) { + return; + } + if ( siteSlugOrId && ! selectedSite ) { // If this step depends on a selected site, only preload after we have the data. // Otherwise, we're still waiting to render something meaningful, and we don't want to @@ -104,8 +109,6 @@ export const FlowRenderer: React.FC< { flow: Flow } > = ( { flow } ) => { if ( // Don't load anything on user step because the user step will hard-navigate anyways. currentStepRoute !== 'user' && - nextStepIndex > 0 && - nextStep && 'asyncComponent' in nextStep ) { nextStep.asyncComponent(); @@ -118,7 +121,7 @@ export const FlowRenderer: React.FC< { flow: Flow } > = ( { flow } ) => { // different points. But even if they do, worst case scenario we only fail to preload // some steps, and they'll simply be loaded later. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ siteSlugOrId, selectedSite, currentStepRoute, flow, isLoggedIn ] ); + }, [ siteSlugOrId, selectedSite, currentStepRoute, flow ] ); const stepNavigation = useStepNavigationWithTracking( { flow,