Goals First: Prevent calypso_signup_start from firing multiple times

[vykes-mac]

Related to #

Proposed Changes

The calypso_signup_start event gets fired multiple times when goals are selected, this is because list of goals are apart of the memoization dependency in the useTracksEventProps hook. As a result each time the user changes the goals the memoization is invalidated and so the event is fired again.

• Remove the goals selection from the dependency so that calypso_signup_start is fired once.

See for more information #98097 (comment)

Why are these changes being made?

Testing Instructions

• go to /setup/onboarding
• Choose multiple goals and verify that the calypso_signup_start is fired once
• Go to the next step and verify the event is not fired again and that the selected goals are recorded in calypso_signup_step_start for the design,domain steps.

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 UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
• 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]

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

Automattic for Agencies live (direct link)

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

[matticbot]

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

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

• notifications
• wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug goals-first/prevent-event-from-firing-twice on your sandbox.

[matticbot]

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

Sections (~14 bytes added 📈 [gzipped])

name             parsed_size           gzip_size
onboarding-flow        +34 B  (+0.1%)      +14 B  (+0.2%)

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.
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.

[zaguiini]

Spreading the object creates a new reference every time, so it doesn't/fixes the problem in a way that might introduce more bugs in the future.

Instead, we could serialize the goals into a string and pass that string to the dependency array, like Paulo did here: https://github.com/Automattic/wp-calypso/pull/98084/files

[zaguiini]

Something to investigate in the future is why the goals array is unstable. It shouldn't be generating a new array every time as very few places use the setGoals handler.

[vykes-mac]

Instead, we could serialize the goals into a string and pass that string to the dependency array, like Paulo did here: https://github.com/Automattic/wp-calypso/pull/98084/files

Wouldn't this still cause re-render as the goals changed as the string will change also? We want to record the goals but in a sense remove the dependency in the useMemo.

[zaguiini]

Wouldn't this still cause re-render as the goals changed as the string will change also

Are the goals changing, or only the reference to the goals array? If the goals are the same, the serialized string will be the same, so it won't re-trigger the calculation.

[zaguiini]

Nice. I'd even add a comment saying that we're only interested in the goals that were selected whenever the user entered.

[p-jackson]

@alshakero Is useTracksEventProps another API that encourages an anti pattern? Implicitly re-sending tracks events when props change doesn't seem right, click events should be sent in response to explicit click handlers, "signup start" should be sent explicitly on some load event.

Basically I think the behaviour described by this unit test description is the opposite of what we would want.

wp-calypso/client/landing/stepper/declarative-flow/internals/hooks/use-sign-up-start-tracking/test/index.tsx

Line 134 in 4a11655

it( 'records the calypso_signup_start event multiple times when dependencies change', () => {

A getTracksEventProps function seems better. The function would only be called on demand when the stepper framework determines it's time to send "signup start" or "step start" events.

[p-jackson]

lgtm

[autumnfjeld]

@vykes-mac Should this comment say "when the user leaves the goals step" ?

[autumnfjeld]

Oh oh, nevermind. Of course when calypso_signup_start fires !! "Start" being the key word! 😄