Skip to content

Woo installer: Add email field to address step #59970

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

Merged
allilevine merged 6 commits into from
Jan 12, 2022
Merged

Woo installer: Add email field to address step #59970

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
629a0de
Add email field to address step
allilevine Jan 11, 2022
2990bb6
Simplify return
allilevine Jan 11, 2022
b2f9c66
Simplify onChange
allilevine Jan 11, 2022
51af260
Add correct types to hook
allilevine Jan 12, 2022
33983db
Update profile object with spread to preserve data
allilevine Jan 12, 2022
7c096db
Fix TypeScript error
allilevine Jan 12, 2022
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
Prev Previous commit
Fix TypeScript error
  • Loading branch information
@allilevine
allilevine committed Jan 12, 2022
commit 7c096db0ca7aada5158803f3c986b66a9b1745a5
2 changes: 1 addition & 1 deletion client/state/site-settings/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export function getSiteSettingsSaveRequestStatus( state, siteId ) {
woocommerce_store_city?: string;
woocommerce_store_postcode?: string;
woocommerce_default_country?: string;
woocommerce_onboarding_profile?: string;
woocommerce_onboarding_profile?: array;
Copy link
Contributor

@noahtallen noahtallen Jan 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hey! I was working on #72348, which resulted in array being changed to Array. I think this caused a cascade of type issues which were unfortunately not caught in this branch!

Firstly, it seems like woocommerce_onboarding_profile might be an Object, given that it's set to Record<string, unknown>. But beyond that, I don't think JSDoc/Typescript fully understood the array type.

The result is that before I made changes in #72348, the return type for the getter function was "any", I think because Typescript couldn't infer the type:

wp-calypso/client/signup/steps/woocommerce-install/hooks/use-site-settings/index.tsx

Lines 56 to 66 in ea8bb7f

function get( option: optionNameType ) {
if ( updates[ option ] ) {
return updates[ option ];
}
if ( ! settings || Object.keys( settings ).length === 0 ) {
return '';
}
return settings[ option ] || '';
}

This made all the consuming code compile correctly, but once the JSDoc type gets fixed, Typescript starts inferring the correct return type. Unfortunately, the other types aren't specific enough, so I had to hardcode any as the return type for the getter.

To fix that, I think a couple things need to happen:

  1. Array should probably change to Object here, unless it is an array?
  2. The woocommerce_onboarding_profile needs to be fully typed -- at least the parts of it used in step-store-address.
  3. The getter needs to conditionally infer the right type. So when onboarding profile is the input, then the type of the profile should be returned, and when the others are the input, then "string" is the return type. (This doesn't happen automatically unfortunately)

Something similar to this:

	function get(option: typeof WOOCOMMERCE_ONBOARDING_PROFILE): OnboardingProfile;
	function get( option: optionNameType ): string {
		if ( updates[ option ] ) {
			return updates[ option ];
		}

		if ( ! settings || Object.keys( settings ).length === 0 ) {
			return '';
		}

		return settings[ option ] || '';
	}

unfortunately, OnboardingProfile does need to be more specific than Record<string, unknown>, because the consumers need more type info to compile correctly. (Otherwise I would have just added the record type to fix it in that PR!)

Copy link
Contributor

@noahtallen noahtallen Jan 21, 2023

Choose a reason for hiding this comment

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

I didn't want to get into that myself, since I'm not familiar with the area. Is that something someone in this PR could take a look at?

Copy link
Member Author

@allilevine allilevine Jan 23, 2023

Choose a reason for hiding this comment

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

I didn't want to get into that myself, since I'm not familiar with the area. Is that something someone in this PR could take a look at?

@noahtallen Thanks for letting us know 🙌 How urgent is this fix? Is it blocking a time-sensitive change?

@daledupreez I think Serenity now owns the Woo onboarding flow, is adjusting the types here something you can pick up? Is this code still used?

Copy link
Contributor

@noahtallen noahtallen Jan 23, 2023

Choose a reason for hiding this comment

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

Nope, it's not urgent! In the worst case, we just continue not having the right types here. Since (i guess) this has been working in prod for a while, it's not a customer-facing issue. I think the code itself works correctly, it's just that the types don't quite line up.

}} SiteSettingsItem
*/

Expand Down