Stronger plan getter types

[noahtallen]

This was inspired by #78292. Essentially, when you call getPlan with a known plan ID, the data is guaranteed to exist, but our types don't enforce that!

This PR adjusts those types to get much more detailed type info:

1. Change PLANS_LIST into an internal data structure
2. Move all plans inside of the object instantiation, instead of adding plans later in the file.
3. Add a PlanSlug type which is the keys of the PLANS_LIST
4. Add export const PLANS_LIST = PLANS_LIST_RAW as Record< PlanSlug, Plan | JetpackPlan | WPComPlan >; so that we know the IDs ahead of time
5. Update getPlan to use PlanSlug with conditional types so we can guarantee a result in some cases
6. Update all other plan getter functions to use this as well

The big question I have is the difference between this approach vs the existing PlanSlug type in ./types here. 🤔

[github-actions]

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

[noahtallen]

What's the difference between this and the keys of PLANS_LIST?

[sirbrillig]

I'm not sure. It was added by #52836 so maybe a clue is there, but a lot of the types in this package are messy since it was a big dynamic mess of data before I dumped it in this package.

[noahtallen]

Cool, I'll check that out!

[matticbot]

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit better-plan-types on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

[matticbot]

This PR modifies the release build for happy-blocks

To test your changes on WordPress.com, run install-plugin.sh happy-blocks better-plan-types on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-r7r-p2

[sirbrillig]

Amazing work! I remember finding it very hard to type the huge and quite variable list of products in this data and I am so glad to see it being done a little more elegantly. If you can get the tests and types to pass, I think this will be a huge improvement.

[tyxla]

Nice work @noahtallen 👍 This is on the right track.

There might be some additional cases we'll need to cover that don't seem to be properly typed right now. A quick look demoed that we don't have support for the following product slugs (but are using them in the code):

export type AdditionalProductSlugs =
	| typeof PRODUCT_AKISMET_ENTERPRISE_YEARLY
	| typeof PRODUCT_AKISMET_FREE
	| typeof PLAN_CHARGEBACK
	| 'concierge-session'
	| typeof WPCOM_FEATURES_CUSTOM_DESIGN
	| 'domain_map'
	| 'domain_redemption'
	| typeof PLAN_FREE
	| typeof PLAN_HOST_BUNDLE
	| typeof PLAN_JETPACK_FREE
	| typeof PRODUCT_JETPACK_SEARCH_FREE
	| typeof WPCOM_FEATURES_NO_ADVERTS
	| 'offsite_redirect'
	| 'unlimited_space'
	| typeof PRODUCT_WPCOM_UNLIMITED_THEMES
	| typeof WPCOM_FEATURES_VIDEOPRESS
	| typeof PLAN_VIP
	| 'wordpress-com-credits'
	| typeof PRODUCT_1GB_SPACE
	| typeof WPCOM_DIFM_LITE
	| typeof GOOGLE_WORKSPACE_BUSINESS_STARTER_MONTHLY
	| typeof TITAN_MAIL_MONTHLY_SLUG
	| typeof PLAN_WPCOM_ENTERPRISE
	| typeof PLAN_WPCOM_FLEXIBLE;

So we might need to either add them to the existing plans and product slugs, or temporarily add them to a separate type as we're fixing them.

Getting this finished will also require some changes to existing code that treats plan slugs and product slugs just as strings. But it's definitely worth it 👍

Unless you beat me to it, I'd also be happy to help with this effort.

[tyxla]

BTW I've pushed a simple plan reordering commit that will help the tests pass. Now only type errors remain.

[noahtallen]

There might be some additional cases we'll need to cover that don't seem to be properly typed right now.

My thinking is that we can only guarantee a result for getPlan when the plan exists in PLANS_LIST. In other words, if the slug is not a key of PLANS_LIST, getPlan will return undefined. Searching through the code, something like PLAN_CHARGEBACK doesn't have a plan definition. So if you call getPlan( PLAN_CHARGEBACK ), you won't get a result.

I think we unfortunately have a mismatch where we statically define some plan slugs (WPCOM_PLANS), and use those for certain type interfaces, but these slugs won't be available in getPlan.

Not really sure how to handle this. At the least, getPlan should only guarantee a result if the plan list contains the plan, and that matches the current behavior... so we probably need to adjust the other parts of the code?

[tyxla]

There might be some additional cases we'll need to cover that don't seem to be properly typed right now.

My thinking is that we can only guarantee a result for getPlan when the plan exists in PLANS_LIST. In other words, if the slug is not a key of PLANS_LIST, getPlan will return undefined. Searching through the code, something like PLAN_CHARGEBACK doesn't have a plan definition. So if you call getPlan( PLAN_CHARGEBACK ), you won't get a result.

I think we unfortunately have a mismatch where we statically define some plan slugs (WPCOM_PLANS), and use those for certain type interfaces, but these slugs won't be available in getPlan.

Not really sure how to handle this. At the least, getPlan should only guarantee a result if the plan list contains the plan, and that matches the current behavior... so we probably need to adjust the other parts of the code?

I agree about the best way to handle this - the types should be aware of all possible products and plans and types should be as robust as possible. But I doubt this is something we want to do in a single iteration, it can be hard since many of those touch many areas and you said, we'll need to touch various other parts of the code.

That said, I've pushed 17fa32c that addresses the remaining issues in a rather open way, and while it's not perfect (for example, string for the key of PLANS_LIST_RAW is too permitting), it allows us to reap the benefits of the plan type improvements suggested in this PR. We can always follow up and tighten them even more in subsequent PRs.

Let me know what you think.

[matticbot]

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

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

name           parsed_size           gzip_size
entry-stepper        -34 B  (-0.0%)      +64 B  (+0.0%)
entry-main           -34 B  (-0.0%)      +64 B  (+0.0%)

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

Async-loaded Components (~63 bytes added 📈 [gzipped])

name                                           parsed_size           gzip_size
async-load-calypso-blocks-inline-help-popover        -34 B  (-0.0%)      +63 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.

[noahtallen]

I like that it works, but I'm worried that we'll cause issues if someone passes in a plan slug which isn't a key of the plan list. In that case, they may not correctly handle potentially undefined plans correctly.

That said, I'd hope that we don't have invalid plan keys floating around. This does still seem like an improvement, so I'm ok with it.

The final thing I'm not sure about is the other PlanSlug definition here:

wp-calypso/packages/calypso-products/src/types.ts

Line 135 in 17fa32c

export type PlanSlug = WPComPlanSlug | JetpackPlanSlug;

We probably shouldn't have two, but I'm not sure which to remove :)

[tyxla]

I like that it works, but I'm worried that we'll cause issues if someone passes in a plan slug which isn't a key of the plan list. In that case, they may not correctly handle potentially undefined plans correctly.

I agree and as I mentioned, the best way to work around this is to incrementally improve the types and add support for all plans. Doesn't have to be in a single PR.

The final thing I'm not sure about is the other PlanSlug definition here:

wp-calypso/packages/calypso-products/src/types.ts

Line 135 in 17fa32c

export type PlanSlug = WPComPlanSlug | JetpackPlanSlug;

We probably shouldn't have two, but I'm not sure which to remove :)

At the time when all plans that exist in WPComPlanSlug | JetpackPlanSlug exist in PLANS_LIST, I'd say we can remove the type you linked and go with the keyof PLANS_LIST one. That might also mean we'd have to improve WPComPlanSlug and JetpackPlanSlug too, to utilize PLANS_LIST as well so there's a single source of truth.

[noahtallen]

That might also mean we'd have to improve WPComPlanSlug and JetpackPlanSlug too, to utilize PLANS_LIST as well so there's a single source of truth.

Yeah, I can see a future with different objects for each plan type; then we can create a union type so that if you pass a WPComSlug, you get a WPComPlan return type

[noahtallen]

If folks want to move forward with this in the future, it'll probably be more useful to open a new PR.