Migrate away from deprecated react types #78292

noahtallen · 2023-06-15T23:01:50Z

Proposed Changes

React 18 includes fixes and changes for certain React types. Notably, ReactFragment is changing from {} | Iterable<ReactNode> to simply Iterable<ReactNode>. Additionally, the ReactChild and ReactText types have been deprecated.

The first change is big because it makes ReactFragment much narrower -- so types which used to be accepted now aren't.

The core place this impacts us is the translation function, which relied on ReactFragment before. With the {} type, that means the translation function currently accepts substitutions which might not exist. We don't want that behavior; we only want to accept substitutions which do exist.

Since this requires a lot of changes, I propose we separate it from #77046 and work on it here:

We can patch @types/react to simply remove {} on ReactFragment. This lets us get the biggest part of the upgrade in the repo more quickly. (We'll remove this after React 18 #77046 is merged.)
Ban ReactChild and ReactText across the repo to prevent new uses.
Migrate away from ReactChild and ReactText
Fix type issues associated with all these changes

Testing Instructions

Typecheck should pass. Most changes should be to types.

github-actions · 2023-06-15T23:02:03Z

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

matticbot · 2023-06-15T23:02:17Z

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

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

name                 parsed_size           gzip_size
entry-subscriptions        +72 B  (+0.0%)      +29 B  (+0.0%)

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

Sections (~136 bytes added 📈 [gzipped])

name                             parsed_size           gzip_size
scan                                  +125 B  (+0.0%)      +24 B  (+0.0%)
reader                                 +82 B  (+0.0%)      +34 B  (+0.0%)
checkout                               +81 B  (+0.0%)      +36 B  (+0.0%)
jetpack-cloud-partner-portal           +68 B  (+0.0%)      +19 B  (+0.0%)
domains                                +55 B  (+0.0%)      +25 B  (+0.0%)
plans                                  +47 B  (+0.0%)      +18 B  (+0.0%)
hosting                                +42 B  (+0.0%)      +12 B  (+0.0%)
themes                                 +37 B  (+0.0%)      +17 B  (+0.0%)
theme                                  +37 B  (+0.0%)      +17 B  (+0.0%)
settings-performance                   +37 B  (+0.0%)      +17 B  (+0.0%)
migrate                                +37 B  (+0.0%)      +17 B  (+0.0%)
home                                   +37 B  (+0.0%)      +17 B  (+0.0%)
earn                                   +37 B  (+0.0%)      +17 B  (+0.0%)
site-purchases                         +35 B  (+0.0%)      +26 B  (+0.0%)
purchases                              +35 B  (+0.0%)      +26 B  (+0.0%)
people                                 +35 B  (+0.0%)      +13 B  (+0.0%)
plugins                                +15 B  (+0.0%)       +7 B  (+0.0%)
import                                 -15 B  (-0.0%)       -6 B  (-0.0%)
jetpack-connect                        +13 B  (+0.0%)       +3 B  (+0.0%)
jetpack-cloud-pricing                  +13 B  (+0.0%)       +3 B  (+0.0%)
jetpack-cloud-plugin-management        +13 B  (+0.0%)       +6 B  (+0.0%)
backup                                 +12 B  (+0.0%)       +8 B  (+0.0%)
activity                               +12 B  (+0.0%)       +9 B  (+0.0%)
site-setup-flow                        +11 B  (+0.0%)       +9 B  (+0.0%)

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 (~31 bytes added 📈 [gzipped])

name                                             parsed_size           gzip_size
async-load-calypso-my-sites-checkout-modal             +51 B  (+0.0%)      +22 B  (+0.0%)
async-load-calypso-blocks-editor-checkout-modal        +51 B  (+0.0%)      +23 B  (+0.0%)
async-load-signup-steps-plans-theme-preselected        +34 B  (+0.0%)      +15 B  (+0.0%)
async-load-signup-steps-plans                          +34 B  (+0.0%)      +15 B  (+0.0%)
async-load-signup-steps-domains                        +34 B  (+0.0%)      +15 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 · 2023-06-15T23:04:19Z

packages/components/src/highlight-cards/annual-highlight-cards.tsx

@@ -29,7 +29,7 @@ export default function AnnualHighlightCards( {

 	const header = (
 		<h3 className="highlight-cards-heading">
-			{ Number.isFinite( year )
+			{ year != null && Number.isFinite( year )


isFinite doesn't restrict the type enough :(

noahtallen · 2023-06-16T02:07:36Z

In cd26e5e, I went through every instance of ReactChild and refactored to what made sense for the scenario. I tried to go with the most restrictive type as much as possible, so using string was viable in a lot of places. I also used TranslateResult in several places which were dealing with more complex translations, ReactElement when components were being used, and finally ReactNode when we were dealing with children.

I also removed ReactFragment in one place in favor of ReactNode.

noahtallen · 2023-06-16T04:23:55Z

In 85338b5, I went through all the translate calls that were breaking. This was relatively discouraging, but I followed this approach:

If it was easy to add an extra guard without changing functionality, I did that.
If I could change the substitution to a blank string without breaking the sentence, I did that.
If there was a quick way to make the root type issue better, I fixed that.
If I couldn't find a better solution, I casted to the right type (as string)

None of this should functionally change how these translations were working. Ideally, the types wouldn't be this loose in the first place.

I really wish this part of our codebase was much better. Despite having strong type tools at our disposal, all of these issues happen because the vast majority of our code can't guarantee that some value exists. Part of this is the fault of the redux store -- (for example, when you get the site title, the site might not exist, and you might get no value back). But this means basically everything is doing way more null checks than we really should have to be doing.

The other problem is when types aren't architected with guarantees in mind from the start. For example, a static map of information, with constant keys known ahead of time, and yet the function to get a value can return undefined. It should be using keyof, etc to get really solid type info around that. But this wasn't as big a problem as the first thing.

noahtallen · 2023-06-16T04:24:37Z

Also, there is this eslint issue that might be a bug:

TypeError: Cannot read properties of undefined (reading 'name')
Occurred while linting client/my-sites/marketing/do-it-for-me/difm-lite-in-progress.tsx:69
Rule: "wpcalypso/i18n-no-variables"

I can't tell what it's seeing on that line, and neither does my IDE

tyxla · 2023-06-16T07:37:56Z

Also, there is this eslint issue that might be a bug:
TypeError: Cannot read properties of undefined (reading 'name')
Occurred while linting client/my-sites/marketing/do-it-for-me/difm-lite-in-progress.tsx:69
Rule: "wpcalypso/i18n-no-variables"
I can't tell what it's seeing on that line, and neither does my IDE

Fix coming up for this in #78305. I'll cherry-pick it here just to ensure that it fixes the issue, but we can land it separately if it does.

tyxla · 2023-06-16T08:11:28Z

Seems like checks passed with #78305. I'll go ahead and merge it and rebase the branch again.

tyxla

Thanks for separating the deprecated React type changes, that really helps make the primary React upgrade PR smaller.

I've done a thorough review and made some suggestions, also pinged a few teams for follow-up fixes that could improve some of the strings in the case of nullish args.

There are a couple of things I think we can improve in this PR before shipping it - let me know what you think!

Fantastic work, @noahtallen! 🚀

.yarn/patches/@types-react-npm-17.0.39-b4ac1f7bfe.patch

client/blocks/importer/components/importer-drag/config.tsx

client/components/jetpack/threat-item-header/index.tsx

client/components/jetpack/threat-item/utils.ts

tyxla · 2023-06-16T08:46:43Z

client/components/marketing-survey/cancel-purchase-form/step-components/upsell-step.tsx

@@ -277,7 +277,7 @@ export default function UpsellStep( { upsell, site, purchase, ...props }: StepPr
 							'But we’d love to see you stick around to build on what you started. ' +
 							'How about a free month of your %(currentPlan)s plan subscription to continue building your site?',
 						{
-							args: { planName: getPlan( purchase.productSlug )?.getTitle() },
+							args: { planName: getPlan( purchase.productSlug )?.getTitle() ?? '' },


The string looks almost good if planName is an empty string. It will only have double spacing. cc @taggon

I'm not convinced the plan ever doesn't exist here 🤔

client/my-sites/marketing/do-it-for-me/difm-lite-in-progress.tsx

client/my-sites/people/team-invite/index.tsx

client/my-sites/plans-comparison/plans-comparison-action.tsx

tyxla · 2023-06-16T09:27:00Z

client/signup/steps/difm-site-picker/site-delete-confirmation-dialog.tsx

+										siteTitle: siteTitle ?? '',
+										siteAddress: siteDomain ?? '',


The string would end up awkward with some additional spacing if siteTitle and/or siteAddress are empty. Is this something we can/should address as a side task, cc @Automattic/martech-decepticons?

I think this is another scenario where the component probably doesn't make sense when the site doesn't exist, so I'd recommend improving types to assert that a site does indeed exist here

client/my-sites/checkout/composite-checkout/hooks/use-prepare-products-for-cart.ts

noahtallen · 2023-06-16T21:25:08Z

Thanks for the comments @sirbrillig and @tyxla! Ideally, we wouldn't have so many null issues in the first place, and have better guarantees through the type system. But that would take a lot of effort -- this PR has to fix nearly 300 different issues!

Ultimately, my approach in the PR was to try to find some balance on each issue one by one. If it was easy to add a more thorough fix, I did so, but ultimately, this mostly doesn't change how these components work on trunk. In reality, a lot of this data exists at runtime, but many components/types aren't designed to have strong type guarantees. (Hence why I used as string in a lot of places.)

For example, everything about a certain component might be designed with the assumption that a site is selected, and there could even be code that uses something like site.ID further up the tree (and even a component which might short-circuit if site doesn't exist!). But since the getSelectedSite selector doesn't have any return type guarantees, every component underneath it in the tree might add site | undefined or similar to its props to just get the types to work.

The vast majority of changes in this PR (related to translate) fall into that category. At the end of the day, the proper fix is to get more strict type guarantees throughout these components. But IMO that's too much to handle in this PR (which only exists to unblock #77046, which is blocking a lot of other things!)

I'll go through each of the comments so far and implement improvements, since those are likely higher impact, but I just wanted to mention most of this was an active consideration as I was working on the PR! There is not a straightforward solution, and at the end of the day, we'll need to improve our education around this so that teams know how to better utilize strong type guarantees. And hopefully the changes in this PR help people notice that more frequently!

noahtallen · 2023-06-16T23:51:16Z

I opened #78350 to explore better types for plans, which showed up a lot in the code. I think it'd be relatively easy to get a lot stricter types for the related code since we know most of the info up front

tyxla

I think this looks great! Thanks @noahtallen, great work as always. 💯

As you recognized, there are some compromises we're making, but it's for the greater good and we're heading towards stricter types in #78350 anyway. I think we should go ahead and ship this as-is.

I also think it would be a good idea if the pinged teams take a look at the code just in case. There could be some opportunities for improving the types or the logic to cover all those ede cases.

a8ci18n · 2023-06-21T00:18:36Z

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/8058917

Hi @noahtallen, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include all of the following strings:

Vulnerable WordPress version.
The installed version of WordPress has a known vulnerability.
Let's start your blog!
this website
this site
Start with %(planTitle)s

Thank you in advance!

noahtallen · 2023-06-21T01:46:47Z

^ Unfortunately, I don't have a good way to test these flows, since I think they would only happen very rarely. It is low impact enough that I'm not very concerned about these strings

tyxla · 2023-06-21T06:05:00Z

Agreed 👍 And what we're adding is already an improvement to what we had before, so I'm not concerned at all either.

a8ci18n · 2023-06-30T09:39:26Z

Translation for this Pull Request has now been finished.

noahtallen requested review from a team as code owners June 15, 2023 23:01

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

noahtallen changed the title ~~Update deprecated react types~~ Migrate away from deprecated react types Jun 15, 2023

noahtallen requested a review from a team June 15, 2023 23:02

noahtallen self-assigned this Jun 15, 2023

noahtallen commented Jun 15, 2023

View reviewed changes

noahtallen force-pushed the update-deprecated-react-types branch from 93a457d to 1778353 Compare June 15, 2023 23:12

noahtallen mentioned this pull request Jun 16, 2023

React 18 #77046

Merged

25 tasks

noahtallen requested review from a team as code owners June 16, 2023 02:05

noahtallen force-pushed the update-deprecated-react-types branch from 1936b0d to 6a002b8 Compare June 16, 2023 02:19

noahtallen requested a review from a team as a code owner June 16, 2023 04:11

noahtallen requested a review from southp June 16, 2023 04:11

noahtallen force-pushed the update-deprecated-react-types branch from 89bba1e to 85338b5 Compare June 16, 2023 04:14

tyxla mentioned this pull request Jun 16, 2023

eslint-plugin-calypso: Fix spread operator error #78305

Merged

tyxla force-pushed the update-deprecated-react-types branch from addc29a to 5425582 Compare June 16, 2023 07:54

tyxla force-pushed the update-deprecated-react-types branch from 5425582 to d130754 Compare June 16, 2023 08:13

tyxla reviewed Jun 16, 2023

View reviewed changes

sirbrillig reviewed Jun 16, 2023

View reviewed changes

client/my-sites/checkout/composite-checkout/hooks/use-prepare-products-for-cart.ts Outdated Show resolved Hide resolved

client/my-sites/checkout/composite-checkout/hooks/use-prepare-products-for-cart.ts Outdated Show resolved Hide resolved

noahtallen mentioned this pull request Jun 16, 2023

Stronger plan getter types #78350

Closed

noahtallen requested a review from tyxla June 16, 2023 23:51

tyxla approved these changes Jun 19, 2023

View reviewed changes

noahtallen added 8 commits June 20, 2023 14:23

Patch React types to include future React 18 bug fix

dbe014d

Ban deprecated React types

b7567c8

Update translation args to avoid deprecated types

60500b0

Rework deprecated react types

bd46f64

Fix translate() errors associated with undefined arguments

20ba94f

Update translateresult argument in another case

ff2c56f

Review improvements

78d1f7b

Migrate a few new uses of ReactChild to TranslateResult

3842051

noahtallen force-pushed the update-deprecated-react-types branch from 1162986 to 3842051 Compare June 20, 2023 21:29

noahtallen merged commit 5541449 into trunk Jun 20, 2023
3 checks passed

noahtallen deleted the update-deprecated-react-types branch June 20, 2023 22:03

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 Jun 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate away from deprecated react types #78292

Migrate away from deprecated react types #78292

noahtallen commented Jun 15, 2023 •

edited

Loading

github-actions bot commented Jun 15, 2023 •

edited by matticbot

Loading

matticbot commented Jun 15, 2023 •

edited

Loading

noahtallen Jun 15, 2023

noahtallen commented Jun 16, 2023 •

edited

Loading

noahtallen commented Jun 16, 2023

noahtallen commented Jun 16, 2023 •

edited

Loading

tyxla commented Jun 16, 2023

tyxla commented Jun 16, 2023

tyxla left a comment

tyxla Jun 16, 2023

noahtallen Jun 16, 2023 •

edited

Loading

tyxla Jun 16, 2023

noahtallen Jun 16, 2023

noahtallen commented Jun 16, 2023

noahtallen commented Jun 16, 2023

tyxla left a comment

a8ci18n commented Jun 21, 2023

noahtallen commented Jun 21, 2023

tyxla commented Jun 21, 2023

a8ci18n commented Jun 30, 2023

Migrate away from deprecated react types #78292

Migrate away from deprecated react types #78292

Conversation

noahtallen commented Jun 15, 2023 • edited Loading

Proposed Changes

Testing Instructions

github-actions bot commented Jun 15, 2023 • edited by matticbot Loading

matticbot commented Jun 15, 2023 • edited Loading

noahtallen Jun 15, 2023

Choose a reason for hiding this comment

noahtallen commented Jun 16, 2023 • edited Loading

noahtallen commented Jun 16, 2023

noahtallen commented Jun 16, 2023 • edited Loading

tyxla commented Jun 16, 2023

tyxla commented Jun 16, 2023

tyxla left a comment

Choose a reason for hiding this comment

tyxla Jun 16, 2023

Choose a reason for hiding this comment

noahtallen Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

tyxla Jun 16, 2023

Choose a reason for hiding this comment

noahtallen Jun 16, 2023

Choose a reason for hiding this comment

noahtallen commented Jun 16, 2023

noahtallen commented Jun 16, 2023

tyxla left a comment

Choose a reason for hiding this comment

a8ci18n commented Jun 21, 2023

noahtallen commented Jun 21, 2023

tyxla commented Jun 21, 2023

a8ci18n commented Jun 30, 2023

noahtallen commented Jun 15, 2023 •

edited

Loading

github-actions bot commented Jun 15, 2023 •

edited by matticbot

Loading

matticbot commented Jun 15, 2023 •

edited

Loading

noahtallen commented Jun 16, 2023 •

edited

Loading

noahtallen commented Jun 16, 2023 •

edited

Loading

noahtallen Jun 16, 2023 •

edited

Loading