Stepper: Apply extended props to `calypso_signup_start`. Update `tailored-ecommerce` flow #93941

chriskmnds · 2024-08-27T09:06:30Z

Part of addressing #94108

Proposed Changes

Updates usage of useSignupStartEventProps to be replaced by useTracksEventProps, as per intended use to cover all signup framework events.
Updates Stepper event slugs with STEPPER_TRACKS_EVENT_SIGNUP_START
Introduces a utility hook useSnakeCasedKeys to help pass props in their snake_cased form, allowing consuming end (flows) to work with camelCased variables.
- This was motivated by similar logic happening in recordSubmitStep for the additional-props
- It is still questionable whether we need this or whether this is the right place and form to define, or whether we could have a recordTracksEvent wrapper in Stepper that would do this instead. We can remove and not bother i.e. for the few rare cases where a flow needs to pass additional props, it can do it in snake_case form and so be it.

DO NOT MERGE - depended on ongoing discussions in #94175

Why are these changes being made?

The useSignupStartEventProps prop/hook was defined for flows to extend the props recorded by calypso_signup_start. The newer hook useTracksEventProps was built to serve this need more generally (across all framework events, including calypso_signup_start).

Testing Instructions

Ensure calypso_signup_start is recorded correctly for isSignupFlow flows
Ensure tailored-ecommerce flow passes the recur prop to the calypso_signup_start Tracks event
Ensure flow_variant is passed correctly for any sub/child flow to the calypso_signup_start Tracks event

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

github-actions · 2024-08-27T09:06:44Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-121056

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-121056&env=jetpack

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-121056&env=a8c-for-agencies

matticbot · 2024-08-27T09:07:37Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~25 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-subscriptions       +280 B  (+0.0%)      +45 B  (+0.0%)
entry-main                +280 B  (+0.0%)      +47 B  (+0.0%)
entry-login               +280 B  (+0.0%)      +46 B  (+0.0%)
entry-stepper             +152 B  (+0.0%)      +26 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~327 bytes removed 📉 [gzipped])

name                     parsed_size           gzip_size
themes                        -208 B  (-0.0%)      -44 B  (-0.0%)
jetpack-connect               -208 B  (-0.0%)      -44 B  (-0.0%)
domains                       -208 B  (-0.0%)      -65 B  (-0.0%)
signup                        -195 B  (-0.1%)      -59 B  (-0.1%)
posts-custom                  -195 B  (-0.0%)      -41 B  (-0.0%)
posts                         -195 B  (-0.0%)      -41 B  (-0.0%)
checkout                      -195 B  (-0.0%)      -52 B  (-0.0%)
accept-invite                 -195 B  (-0.1%)      -55 B  (-0.1%)
tailored-ecommerce-flow       +105 B  (+0.2%)      +33 B  (+0.5%)

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 (~213 bytes removed 📉 [gzipped])

name                                                         parsed_size           gzip_size
async-load-design-blocks                                          -208 B  (-0.0%)      -44 B  (-0.0%)
async-load-calypso-my-sites-site-settings-seo-settings-form       -208 B  (-0.1%)      -53 B  (-0.1%)
async-load-calypso-components-web-preview-component               -195 B  (-0.0%)      -56 B  (-0.0%)
async-load-calypso-blocks-editor-checkout-modal                   -195 B  (-0.0%)      -60 B  (-0.0%)

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.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

alshakero

Man this is becoming so niccce.

alshakero · 2024-08-30T10:30:48Z

client/landing/stepper/declarative-flow/tailored-ecommerce-flow.ts

 		const recur = useSelect(
 			( select ) => ( select( ONBOARD_STORE ) as OnboardSelect ).getEcommerceFlowRecurType(),
 			[]
 		);

-		return { recur };
+		return useMemo( () => ( { [ STEPPER_TRACKS_EVENT_SIGNUP_START ]: { recur } } ), [ recur ] );


I think we should save people who don't use useMemo by serializing the object somehow before passing it to the effect.

I have the same feeling. By definition though, useTracksEventProps being a hook, it can have internal state or rerun/react to changes in external state (such a store), so it will force rerenders up the tree as per the natural life cycle.

I'm really not too fond of "hacking" away from the normal data flow in React. Perhaps we need to accept the reality and ask consumers to be smart about it. ~~Otherwise, we can turn the hook to a regular function and that should make it clear that it won't be deviant to rerunning on state changes.~~ (Edit: i think that's just not gonna work at all)

I think that's more or less my thought process in general

Oh I meant the above as in more generally about the risk involved (as in connection to your comment in the other PR)

We can certainly do some sort of string-serialization on framework end to avoid same object not being memoized. Just the reality doesn't change that state changing from here will propagate up

alshakero · 2024-08-30T10:34:53Z

client/landing/stepper/utils/use-snake-cased-keys.ts

+	input?: Record< string, unknown >;
+}
+
+const useSnakeCasedKeys = ( { input }: Params ) => {


Can we call this something like useMemoizedSnakeCasedKeys or something indicative?

Hmm I'm not sure, but I wouldn't mind if that really helps. It sounds too low-level to me. Hooks not memoizing ought to be the exception for the most part, not the norm. If there's an object being returned (similarly with state selectors) they ought to memoize the value to avoid recomputing and ensuring referential integrity (avoiding rerenders up the chain). It feels weird to be calling hooks useMemoizedFoo, useMemoizedBar, etc.

p.s. there's a test added that acts as a fairly tough restriction/guarantee that this will be a memoized value.

alshakero · 2024-08-30T10:38:26Z

client/landing/stepper/utils/use-snake-cased-keys.ts

+}
+
+const useSnakeCasedKeys = ( { input }: Params ) => {
+	return useMemo( () => input && ( convertToSnakeCase( input ) as Params[ 'input' ] ), [ input ] );


I bet people will pass this function fresh objects every render and the useMemo won't be able to function. Maybe we can make a clever hook that memoizes objects by serializing keys and values somehow. Or use KCD's https://github.com/kentcdodds/use-deep-compare-effect.

It's definitely a possibility, but I think it sounds like a general issue with Stepper (and more generally) e.g. every single hook that's exposed to flows for implementation is subject to this concern - so is everything built on top to transform. It's almost like we need to wrap everything with such a clever serializer.

I think there may be value in something like that, but:

doing it here feels a bit random i.e. why only this hook? It needs to be planned out. I would also think a provision to "disable" something like that to be of value.

it will become a default performance penalty that happens on every update that goes through these hooks. We'd effectively need to "ban" use of useMemo and resolve to this default implementation that wraps everything. Why not trust that flows will be smart about it (especially as we go through a phase of minimizing them)? They only need to memoize the edge to "potentially" benefit (as none of this memoizing is or ought to be required)

For all I know, memoizing should be an optional behavior, not a default one. Rerenders don't usually translate to UI changes. The fact that useSnakeCasedKeys memorizes isn't required, just a provision to allow flows to opt-in when/if needed (as the alternative would mean "input" being memoized or not makes no difference).

…pdate tailored-ecommerce-flow cleanup

useSnakeCasedKeys

matticbot · 2024-10-02T12:49:49Z

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

notifications

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/stepper-tracking-signup_start_extend on your sandbox.

escapemanuele

I really like how the extra event properties are decided at a flow level, LGTM 👍

chriskmnds · 2024-10-03T16:23:34Z

Thanks for taking the time!

chriskmnds added the Stepper label Aug 27, 2024

chriskmnds added this to the Stepper Improvements: Improve and Abstract Tracks milestone Aug 27, 2024

chriskmnds self-assigned this Aug 27, 2024

chriskmnds requested a review from a team as a code owner August 27, 2024 09:06

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 27, 2024

chriskmnds requested a review from a team August 27, 2024 09:07

chriskmnds mentioned this pull request Aug 29, 2024

Stepper: Apply extended props to calypso_signup_step_start #93983

Merged

7 tasks

alshakero reviewed Aug 30, 2024

View reviewed changes

chriskmnds mentioned this pull request Sep 2, 2024

Stepper: Allow flows to extend the props passed to all the framework Tracks events #94108

Closed

chriskmnds changed the title ~~Stepper: Refactor usage of useSignupStartEventProps with useTracksEventProps. Update tailored-ecommerce flow~~ Stepper: Apply extended props to calypso_signup_start. Update tailored-ecommerce flow Sep 2, 2024

Base automatically changed from update/stepper-step-navigation-tracking-nav-event-remaining-events to trunk September 3, 2024 09:20

chriskmnds added the DO NOT MERGE label Sep 5, 2024

chriskmnds removed the DO NOT MERGE label Sep 30, 2024

chriskmnds added 7 commits October 2, 2024 15:26

Stepper: Replace useSignupStartEventProps with useTracksEventProps. U…

b582f7e

…pdate tailored-ecommerce-flow cleanup

useSnakeCasedKeys

068e402

useSnakeCasedKeys

apply useSnakeCasedKeys util

7bb3eac

clean up fix tests

dbdbefb

cleanup

6dba5da

cleanup

9f14a72

cleanup

ae77e60

chriskmnds force-pushed the update/stepper-tracking-signup_start_extend branch from 9c9a10f to ae77e60 Compare October 2, 2024 12:40

chriskmnds requested a review from a team October 2, 2024 12:42

escapemanuele approved these changes Oct 3, 2024

View reviewed changes

chriskmnds merged commit d521b18 into trunk Oct 3, 2024
12 checks passed

chriskmnds deleted the update/stepper-tracking-signup_start_extend branch October 3, 2024 16:23

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stepper: Apply extended props to `calypso_signup_start`. Update `tailored-ecommerce` flow #93941

Stepper: Apply extended props to `calypso_signup_start`. Update `tailored-ecommerce` flow #93941

chriskmnds commented Aug 27, 2024 •

edited

Loading

github-actions bot commented Aug 27, 2024 •

edited by matticbot

Loading

matticbot commented Aug 27, 2024 •

edited

Loading

alshakero left a comment •

edited

Loading

alshakero Aug 30, 2024

chriskmnds Aug 30, 2024 •

edited

Loading

chriskmnds Aug 30, 2024

alshakero Aug 30, 2024

chriskmnds Sep 2, 2024 •

edited

Loading

alshakero Aug 30, 2024

chriskmnds Sep 2, 2024 •

edited

Loading

matticbot commented Oct 2, 2024

escapemanuele left a comment

chriskmnds commented Oct 3, 2024

Stepper: Apply extended props to calypso_signup_start. Update tailored-ecommerce flow #93941

Stepper: Apply extended props to calypso_signup_start. Update tailored-ecommerce flow #93941

Conversation

chriskmnds commented Aug 27, 2024 • edited Loading

Proposed Changes

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

github-actions bot commented Aug 27, 2024 • edited by matticbot Loading

matticbot commented Aug 27, 2024 • edited Loading

alshakero left a comment • edited Loading

Choose a reason for hiding this comment

alshakero Aug 30, 2024

Choose a reason for hiding this comment

chriskmnds Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

chriskmnds Aug 30, 2024

Choose a reason for hiding this comment

alshakero Aug 30, 2024

Choose a reason for hiding this comment

chriskmnds Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

alshakero Aug 30, 2024

Choose a reason for hiding this comment

chriskmnds Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

matticbot commented Oct 2, 2024

escapemanuele left a comment

Choose a reason for hiding this comment

chriskmnds commented Oct 3, 2024

Stepper: Apply extended props to `calypso_signup_start`. Update `tailored-ecommerce` flow #93941

Stepper: Apply extended props to `calypso_signup_start`. Update `tailored-ecommerce` flow #93941

chriskmnds commented Aug 27, 2024 •

edited

Loading

github-actions bot commented Aug 27, 2024 •

edited by matticbot

Loading

matticbot commented Aug 27, 2024 •

edited

Loading

alshakero left a comment •

edited

Loading

chriskmnds Aug 30, 2024 •

edited

Loading

chriskmnds Sep 2, 2024 •

edited

Loading

chriskmnds Sep 2, 2024 •

edited

Loading