Stepper API: add initialize method and deprecate useSteps
#97999
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~467 bytes added 📈 [gzipped]) Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~8384 bytes added 📈 [gzipped]) 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 (~4760 bytes removed 📉 [gzipped]) 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. Generated by performance advisor bot at iscalypsofastyet.com. |
alshakero
changed the title
bootFlow methodbootFlow method
… into fix/use-steps-hook
matticbot
added
the
[Status] Needs Review
alshakero
changed the title
bootFlow methodinitialize method and deprecate useSteps
… into fix/use-steps-hook
… into fix/use-steps-hook
| __experimentalUseBuiltinAuth?: boolean; | ||
|
|
||
| /** | ||
| * The steps of the flow. **Please don't use this variable unless absolutely necessary**. It's means to be used internally by the Stepper. |
There was a problem hiding this comment.
| * The steps of the flow. **Please don't use this variable unless absolutely necessary**. It's means to be used internally by the Stepper. | |
| * The steps of the flow. **Please don't use this variable unless absolutely necessary**. It's meant to be used internally by the Stepper. |
client/landing/stepper/index.tsx
Outdated
| return; | ||
| } | ||
|
|
||
| // Checking for initialize implies this a V2 flow. |
There was a problem hiding this comment.
| // Checking for initialize implies this a V2 flow. | |
| // Checking for initialize implies this is a V2 flow. |
| // Checking for initialize implies this a V2 flow. | ||
| // CLEAN UP: once the `onboarding` flow is migrated to V2, this can be cleaned up to only support V2 | ||
| // The `onboarding` flow is the only flow that uses in-stepper auth so far, so all the auth logic catering V1 can be deleted. | ||
| if ( 'initialize' in flow && flowSteps ) { |
There was a problem hiding this comment.
Do we need to check for initialize here? If we have flowSteps then we already checked in
let flowSteps = 'initialize' in flow ? await flow.initialize() : null;
right?
There was a problem hiding this comment.
Good catch. I thought TS wouldn't guess the version!
TS complains if I don't check for initialize, because __flowSteps doesn't exist in V1 flows.
| @@ -135,6 +136,8 @@ const availableFlows: Record< string, () => Promise< { default: Flow } > > = { | |||
| ), | |||
| [ MIGRATION_FLOW ]: () => | |||
| import( /* webpackChunkName: "migration-flow" */ '../declarative-flow/migration' ), | |||
| [ EXAMPLE_FLOW ]: () => | |||
| import( /* webpackChunkName: "migration-flow" */ '../declarative-flow/example' ), | |||
There was a problem hiding this comment.
| import( /* webpackChunkName: "migration-flow" */ '../declarative-flow/example' ), | |
| import( /* webpackChunkName: "example-flow" */ '../declarative-flow/example' ), |
| @@ -36,7 +36,7 @@ const getNextStep = ( currentStep: string, steps: StepperStep[] ): string | unde | |||
|
|
|||
| const SiteIntent = Onboard.SiteIntent; | |||
|
|
|||
| const pluginBundleFlow: Flow = { | |||
| const pluginBundleFlow: FlowV1 = { | |||
There was a problem hiding this comment.
Is there a particular reason we chose these flows and upgraded their type, or can it be done in a following PR if needed?
There was a problem hiding this comment.
Yes because these flows depend on useAssertConditions which doesn't exist in V2.
|
Testing steps Test a flow
Test a flow with non-Stepper auth
Test a variant flow
|
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
|
@alshakero this is still on hold to better work with the new proposal? |
|
No it's good to go. I'll merge soon. |
github-actions
bot
removed
the
[Status] Needs Review
|
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/17203594 Some locales (Hebrew, Japanese) have been temporarily machine-translated due to translator availability. All other translations are usually ready within a few days. Untranslated and machine-translated strings will be sent for translation next Monday and are expected to be completed by the following Friday. Thank you @alshakero for including a screenshot in the description! This is really helpful for our translators. |
Co-authored-by: Anthony Grullon <[email protected]> Co-authored-by: Emanuele Buccelli <[email protected]>
|
Translation for this Pull Request has now been finished. |
Proposal
This implements a new method to Stepper framework API called
initialize. This method is an asynchronous method that is called once per flow to boot the flow. It should do any prep actions (fetch experiments, assert conditions, redirect to other flows, etc...), and most importantly return the steps.This also deprecated the
useStepshook because it has a few flaws.Why
Issues with
useStepshookThe
useStepshook gives the impression that steps can be dynamic, which is incorrect for a few reasons:useAssertConditionshooks. Which blocks rendering the flow until it returnsSUCCESS. The real purpose of this hook is to turn the repeating pattern of React renders into a classic procedural asynchronous function.Enter
async initializeuseSideEffectsanduseAssertConditionsare no longer neededSince
initializeruns once and first, these two hooks are not needed in most cases.Example without
useSideEffectsBefore
After
Example without
useAssertConditionsBefore
After
Testing steps
This change is completely backwards compatible and shouldn't change anything for the existing flows. But we should test to make sure we don't break anything.
Test a flow
Test a flow with non-Stepper auth
Test a variant flow