Skip to content
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.

Sign up for GitHub

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

Stepper: Support loading in useTracksEventProps #98219

Merged
merged 2 commits into from
Jan 10, 2025
+65 −35
Merged

Stepper: Support loading in useTracksEventProps #98219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dd6f5a7
Support loading in `useTracksEventProps`
alshakero Jan 10, 2025
621c61b
Merge branch 'trunk' into stepper-tracking/support-loading
vykes-mac Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions client/landing/stepper/declarative-flow/connect-domain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { CONNECT_DOMAIN_FLOW } from '@automattic/onboarding';
import { useSelect, useDispatch } from '@wordpress/data';
import { translate } from 'i18n-calypso';
import { useEffect } from 'react';
import { useEffect, useMemo } from 'react';
import { useFlowLocale } from 'calypso/landing/stepper/hooks/use-flow-locale';
import { domainMapping } from 'calypso/lib/cart-values/cart-items';
import { triggerGuidesForStep } from 'calypso/lib/guides/trigger-guides-for-step';
Expand Down Expand Up @@ -109,12 +109,17 @@ const connectDomain: Flow = {
useTracksEventProps() {
const { domain, provider } = useDomainParams();

return {
[ STEPPER_TRACKS_EVENT_STEP_NAV_SUBMIT ]: {
domain,
provider,
},
};
return useMemo(
() => ( {
eventsProperties: {
[ STEPPER_TRACKS_EVENT_STEP_NAV_SUBMIT ]: {
domain,
provider,
},
},
} ),
[ domain, provider ]
);
},
useStepNavigation( _currentStepSlug, navigate ) {
const flowName = this.name;
Expand Down
9 changes: 6 additions & 3 deletions .../declarative-flow/internals/components/step-route/hooks/use-step-route-tracking/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,10 @@ export const useStepRouteTracking = ( { flow, stepSlug, skipStepRender }: Props
const pathname = window.location.pathname;
const flowVariantSlug = flow.variantSlug;
const flowName = flow.name;
const customProperties = flow.useTracksEventProps?.();
const isLoading = customProperties?.isLoading;
const signupStepStartProps = useSnakeCasedKeys( {
input: flow.useTracksEventProps?.()?.[ STEPPER_TRACKS_EVENT_SIGNUP_STEP_START ],
input: customProperties?.eventsProperties[ STEPPER_TRACKS_EVENT_SIGNUP_STEP_START ],
} );

/**
Expand All @@ -71,7 +73,8 @@ export const useStepRouteTracking = ( { flow, stepSlug, skipStepRender }: Props

useEffect( () => {
// We wait for the site to be fetched before tracking the step route.
if ( ! hasRequestedSelectedSite ) {
// And if `isLoading` is true, it means the flow is still loading custom properties.
if ( ! hasRequestedSelectedSite || isLoading ) {
return;
}

Expand Down Expand Up @@ -134,5 +137,5 @@ export const useStepRouteTracking = ( { flow, stepSlug, skipStepRender }: Props
// We also leave out pathname. The respective event (calypso_page_view) is recorded behind a timeout and we don't want to trigger it again.
// - window.location.pathname called inside the effect keeps referring to the previous path on a redirect. So we moved it outside.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [ flowName, hasRequestedSelectedSite, stepSlug, skipStepRender ] );
}, [ flowName, hasRequestedSelectedSite, stepSlug, skipStepRender, isLoading ] );
};
8 changes: 5 additions & 3 deletions client/landing/stepper/declarative-flow/internals/hooks/use-sign-up-start-tracking/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ export const useSignUpStartTracking = ( { flow }: Props ) => {
const flowName = flow.name;
const flowVariant = flow.variantSlug;
const isSignupFlow = flow.isSignupFlow;
const customPropsConfig = flow.useTracksEventProps?.();
const isLoading = customPropsConfig?.isLoading;
const signupStartEventProps = useSnakeCasedKeys( {
input: flow.useTracksEventProps?.()[ STEPPER_TRACKS_EVENT_SIGNUP_START ],
input: customPropsConfig?.eventsProperties[ STEPPER_TRACKS_EVENT_SIGNUP_START ],
} );

/**
Expand All @@ -44,7 +46,7 @@ export const useSignUpStartTracking = ( { flow }: Props ) => {
}, [ isSignupFlow, flowName ] );

useEffect( () => {
if ( ! isSignupFlow ) {
if ( ! isSignupFlow || isLoading ) {
return;
}

Expand All @@ -56,5 +58,5 @@ export const useSignUpStartTracking = ( { flow }: Props ) => {
...( flowVariant && { flow_variant: flowVariant } ),
},
} );
}, [ isSignupFlow, flowName, ref, signupStartEventProps, flowVariant ] );
}, [ isSignupFlow, flowName, ref, signupStartEventProps, isLoading, flowVariant ] );
};
8 changes: 6 additions & 2 deletions ...anding/stepper/declarative-flow/internals/hooks/use-sign-up-start-tracking/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,9 @@ describe( 'useSignUpTracking', () => {
flow: {
...signUpFlow,
useTracksEventProps: () => ( {
[ STEPPER_TRACKS_EVENT_SIGNUP_START ]: { extra: 'props' },
eventsProperties: {
[ STEPPER_TRACKS_EVENT_SIGNUP_START ]: { extra: 'props' },
},
} ),
} satisfies Flow,
queryParams: { ref: 'another-flow-or-cta' },
Expand Down Expand Up @@ -136,7 +138,9 @@ describe( 'useSignUpTracking', () => {
flow: {
...signUpFlow,
useTracksEventProps: () => ( {
[ STEPPER_TRACKS_EVENT_SIGNUP_START ]: { extra: 'props' },
eventsProperties: {
[ STEPPER_TRACKS_EVENT_SIGNUP_START ]: { extra: 'props' },
},
} ),
} satisfies Flow,
} );
Expand Down
13 changes: 8 additions & 5 deletions ...nding/stepper/declarative-flow/internals/hooks/use-step-navigation-with-tracking/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { OnboardSelect } from '@automattic/data-stores';
import { useSelect } from '@wordpress/data';
import { useCallback, useMemo, useRef } from '@wordpress/element';
import { useCallback, useMemo } from '@wordpress/element';
import {
STEPPER_TRACKS_EVENT_STEP_NAV_EXIT_FLOW,
STEPPER_TRACKS_EVENT_STEP_NAV_GO_BACK,
Expand Down Expand Up @@ -36,8 +36,6 @@ export const useStepNavigationWithTracking = ( {
}, [] );

const tracksEventPropsFromFlow = flow.useTracksEventProps?.();
const tracksEventPropsFromFlowRef = useRef( tracksEventPropsFromFlow );
tracksEventPropsFromFlowRef.current = tracksEventPropsFromFlow;

const handleRecordStepNavigation = useCallback(
( {
Expand All @@ -56,11 +54,16 @@ export const useStepNavigationWithTracking = ( {
providedDependencies: dependencies,
additionalProps: {
...( eventProps ?? {} ),
...( tracksEventPropsFromFlowRef.current?.[ event ] ?? {} ),
// Don't add eventProps if `useTracksEventProps` is still loading.
// It's not tight, but it's a trade-off to avoid firing events with incorrect props.
// It's a tiny edge case where the use navigates before this hook is ready.
...( tracksEventPropsFromFlow?.isLoading
? undefined
: tracksEventPropsFromFlow?.eventsProperties?.[ event ] ?? {} ),
},
} );
},
[ intent, goals, currentStepRoute, flow ]
[ intent, tracksEventPropsFromFlow, goals, currentStepRoute, flow ]
);

return useMemo(
Expand Down
8 changes: 7 additions & 1 deletion client/landing/stepper/declarative-flow/internals/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,13 @@ export type UseSideEffectHook< FlowSteps extends StepperStep[] > = (
* Can pass any properties that should be recorded for the respective events.
*/
export type UseTracksEventPropsHook = () => {
[ key in ( typeof STEPPER_TRACKS_EVENTS )[ number ] ]?: Record< string, string | number | null >;
/**
* This flag is needed to indicate that the custom props are still loading. And the return value will be ignored until it's false.
*/
isLoading?: boolean;
eventsProperties: Partial<
Record< ( typeof STEPPER_TRACKS_EVENTS )[ number ], Record< string, string | number | null > >
>;
};

export type Flow = {
Expand Down
30 changes: 17 additions & 13 deletions client/landing/stepper/declarative-flow/onboarding.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ const onboarding: Flow = {
isSignupFlow: true,
__experimentalUseBuiltinAuth: true,
useTracksEventProps() {
const isGoalsAtFrontExperiment = useGoalsFirstExperiment()[ 1 ];
const [ isLoading, isGoalsAtFrontExperiment ] = useGoalsFirstExperiment();
const userIsLoggedIn = useSelector( isUserLoggedIn );
const goals = useSelect(
( select ) => ( select( ONBOARD_STORE ) as OnboardSelect ).getGoals(),
Expand All @@ -63,21 +63,25 @@ const onboarding: Flow = {

return useMemo(
() => ( {
[ STEPPER_TRACKS_EVENT_SIGNUP_START ]: {
is_goals_first: isGoalsAtFrontExperiment.toString(),
...( isGoalsAtFrontExperiment && { step: 'goals' } ),
is_logged_out: initialLoggedOut.current.toString(),
},
[ STEPPER_TRACKS_EVENT_SIGNUP_STEP_START ]: {
...( isGoalsAtFrontExperiment && {
isLoading,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posting here so we can keep a discussion.

Cool thinking! I've nearly completely lost hope in this API, at least in its current form, and primarily from the way that we pick the events to guide analyses. But having an async isLoading prop that propagates this way can indeed create misalignment. My feeling is this is not insignificant risk and it also creates ambiguity/trust issues with the API:

Another risk is if all of these conditions are met:

  1. A flow customizes calypso_signup_start using the hook.
  2. The customization needs time load.
  3. They do not customize calypso_signup_step_start as well.

The calypso_signup_step_start will fire before calypso_signup_start. This is the only risky combo.

So a usage that for whatever reason (common reason apparently, like an experiment running) will need to customise all the events handled by the API to ensure there is correct order. Is this correct, from what you are saying?

Just thinking about the above, assuming correct, it's almost like this prop doesn't accomplish anything more than not passing custom props until isLoading is true:

...( ! isLoading && {
    [ STEPPER_TRACKS_EVENT_SIGNUP_START ]: {
        is_goals_first: isGoalsAtFrontExperiment.toString(),
        ...( isGoalsAtFrontExperiment && { step: 'goals' } ),
        is_logged_out: ( ! userIsLoggedIn ).toString(),
    }, 
    ...
} )

If the two are equivalent, then I'd consider this instead and not have it as part of the API. It feels more ambiguous when something's part of the API but with conditions of use.

I wonder if other caveats may exist too, but I've been away from this for quite a while to have perspective 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Christos! Long time no see.

I've nearly completely lost hope in this API, at least in its current form, and primarily from the way that we pick the events to guide analyses

Totally on the same page here. I'll reconsider this once I'm done with pdDR7T-22t-p2.

it's almost like this prop doesn't accomplish anything more than not passing custom props until isLoading is true:

That's not entirely correct. When the prop is true, the event does not fire at all. In the snippet you share, the event will fire but without the customizations passed by the flow..

eventsProperties: {
[ STEPPER_TRACKS_EVENT_SIGNUP_START ]: {
is_goals_first: isGoalsAtFrontExperiment.toString(),
} ),
...( initialGoals.current.length && {
goals: initialGoals.current.join( ',' ),
} ),
...( isGoalsAtFrontExperiment && { step: 'goals' } ),
is_logged_out: initialLoggedOut.current.toString(),
},

[ STEPPER_TRACKS_EVENT_SIGNUP_STEP_START ]: {
...( isGoalsAtFrontExperiment && {
is_goals_first: 'true',
} ),
...( initialGoals.current.length && {
goals: initialGoals.current.join( ',' ),
} ),
},
},
} ),
[ isGoalsAtFrontExperiment, initialGoals, initialLoggedOut ]
[ isGoalsAtFrontExperiment, initialLoggedOut, initialGoals, isLoading ]
);
},
useSteps() {
Expand Down
5 changes: 4 additions & 1 deletion client/landing/stepper/declarative-flow/tailored-ecommerce-flow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@ const ecommerceFlow: Flow = {
[]
);

return useMemo( () => ( { [ STEPPER_TRACKS_EVENT_SIGNUP_START ]: { recur } } ), [ recur ] );
return useMemo(
() => ( { eventsProperties: { [ STEPPER_TRACKS_EVENT_SIGNUP_START ]: { recur } } } ),
[ recur ]
);
},
useSteps() {
const steps = [
Expand Down
Loading