React 18: Fix @automattic/global-styles package #77319

arthur791004 · 2023-05-24T10:27:59Z

Related to #77048, #77046 -- this change fixes compatibility with WordPress packages after the react 18 update

Proposed Changes

Create a gutenberg-bridge folder inside the @automattic/global-styles package and
- Unlock the private APIs we need and re-export them for Calypso to use.
- Re-implement mergeBaseAndUserConfigs function as Gutenberg doesn't export it
- Create withExperimentalBlockEditorProvider to wrap the component with ExperimentalBlockEditorProvider and eliminate the compatStyles. The reason is the compatStyles are not required on Calypso and it will cause warnings since some of compatStyles might not have an id so that we cannot use it as key when rendering.
Use the fixed version of @wordpress/private-apis to ensure we have the consistent __private symbol and avoid we're not able to unlock the private APIs due to the unequal private symbol. Or is dedupe enough?

There is a warning from Gutenberg below, but it looks like GB won't encounter this one 🤔

Warning: validateDOMNesting(...): <svg> cannot appear as a child of <head>.

Testing Instructions

Theme Showcase

Go to /themes/<your_site>
Ensure the style variation discs look good
Pick a theme with style variations
Ensure the style variations still look good on the theme details page

Design Picker

Go to /setup?siteSlug=<your_site>
Continue until you land on the Design Picker screen
Ensure the style variation discs look good
Pick a theme with style variations
Ensure the style variations still look good on the preview screen

Pattern Assembler

Go to /setup?siteSlug=<your_site>
Continue until you land on the Design Picker screen
Scroll down to the bottom
Pick the BCPA CTA (Start designing)
Select header/homepage/footer/colors/fonts
Ensure everything looks good as before

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 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-ajp-p2)?

matticbot · 2023-05-24T10:28:04Z

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

noahtallen

Overall, this looks good to me. I really like the gutenberg-bridge approach, which should ideally make it easier to change things in the future. I tested all the features you mentioned in the PR, and they are all testing great for me! Do we use any of these in the editor via ETK?

One thing I'm worried about is that the unlock API package version is from a newer release than the other Gutenberg packages. However, the unlock package doesn't have dependencies on other packages in Gutenberg, so I think it will be ok.

I think that most of the CI failures are from the underlying React PR and not from this one.

noahtallen · 2023-05-25T18:28:14Z

packages/global-styles/src/components/global-styles-provider/index.tsx

+		},
+		[ setUserConfig ]
+	);
+	return [ !! true, userConfig, setConfig ];


What's the purpose of !! true here?

To indicate the config is ready. We don't want to fetch the current user config from the server at the current stage, so it's always ready at the begining

I think Noah was suggesting that we could remove the !! boolean casting, since it's already a boolean.

noahtallen · 2023-05-25T18:32:03Z

packages/global-styles/src/gutenberg-bridge/index.tsx

+import type { GlobalStylesObject } from '../types';
+
+const { unlock } = __dangerousOptInToUnstableAPIsOnlyForCoreModules(
+	'I know using unstable features means my plugin or theme will inevitably break on the next WordPress release.',


I hadn't realized they added this message, but I'm definitely a fan since it forces us to be explicit about it 😅

Yup, I love it too 😉 Great tactics from a devex perspective. 👍

noahtallen · 2023-05-25T18:52:59Z

packages/global-styles/src/gutenberg-bridge/index.tsx

+		}
+	};
+
+	return mergeWith( {}, base, user, mergeTreesCustomizer );


We would like to avoid lodash if possible, but this could be a use case which is too difficult to replicate natively.

Yes, we have to take time to see how to support mergeWith natively but I think it's not the goal of this PR. So, I'd prefer to do it later 🙂

I'd also love to not see new Lodash usage introduced.

We've been using deepmerge as a replacement for merge and mergeWith in Gutenberg and it mostly works well. In this case, it appears to be a simple scenario where we want to disable merging arrays and just have them overriden. This can be achieved with something like:

import deepmerge from 'deepmerge'; import { isPlainObject } from 'is-plain-object'; deepmerge( obj1, obj2, { isMergeableObject: isPlainObject, } );

and if you want to avoid using is-plain-object for some reason, you could do something like (untested):

import deepmerge from 'deepmerge'; deepmerge( obj1, obj2, { isMergeableObject: ( value ) => ! Array.isArray( value ), } );

Updated! TIL: deepmerge 👍

noahtallen · 2023-05-25T19:07:25Z

packages/global-styles/src/hooks/use-sync-global-styles-user-config.ts

-	// return user;
+	const { user, setUserConfig } = useContext( GlobalStylesContext );
+	useEffect( () => {
+		if ( ! enabled ) {


Note: this file changed on trunk, so it might need to be refreshed. (I resolved the merge conflict in the base PR to maintain compatibility with this branch)

Rebased. Thanks for noting that!

… ones

…nsistent

tyxla · 2023-05-26T10:51:30Z

packages/global-styles/src/gutenberg-bridge/index.tsx

+		}
+	};
+
+	return mergeWith( {}, base, user, mergeTreesCustomizer );


I'd also love to not see new Lodash usage introduced.

We've been using deepmerge as a replacement for merge and mergeWith in Gutenberg and it mostly works well. In this case, it appears to be a simple scenario where we want to disable merging arrays and just have them overriden. This can be achieved with something like:

import deepmerge from 'deepmerge'; import { isPlainObject } from 'is-plain-object'; deepmerge( obj1, obj2, { isMergeableObject: isPlainObject, } );

and if you want to avoid using is-plain-object for some reason, you could do something like (untested):

import deepmerge from 'deepmerge'; deepmerge( obj1, obj2, { isMergeableObject: ( value ) => ! Array.isArray( value ), } );

tyxla · 2023-05-26T10:55:05Z

packages/global-styles/src/components/global-styles-provider/index.tsx

@@ -1,7 +1,6 @@
-// import { GlobalStylesContext } from '@wordpress/edit-site/build-module/components/global-styles/context';
-// import { mergeBaseAndUserConfigs } from '@wordpress/edit-site/build-module/components/global-styles/global-styles-provider';
 import { isEmpty, mapValues } from 'lodash';


I bet those Lodash usages should be pretty straightforward to remove. Let me know if I can aid with suggestions.

tyxla · 2023-05-26T10:58:21Z

packages/subscriber/src/components/add-form/index.tsx

+			Button: (
+				<Button
+					variant="link"
+					target="__blank"


I assume you meant:

Suggested change

target="__blank"

target="_blank"

Fixed. But I found there are lots of __blank in our codebase 😂

Bummer. It's a good opportunity to fix them 😅 Fixed them all in #77484

tyxla · 2023-05-26T10:58:32Z

packages/subscriber/src/components/add-form/index.tsx

+							Button: (
+								<Button
+									variant="link"
+									target="__blank"


Suggested change

target="__blank"

target="_blank"

tyxla · 2023-05-26T11:00:47Z

packages/help-center/src/components/help-center-notice.tsx

-							<ExternalLink href={ localizeUrl( 'https://wordpress.com/support/com-vs-org/' ) } />
+							<ExternalLink
+								href={ localizeUrl( 'https://wordpress.com/support/com-vs-org/' ) }
+								children={ null }


Why is that necessary? children will be undefined by default.

The problem is that some components require children via TS, which is hard to support in some cases (like when interpolating)

Makes sense, thanks.

Intuitively, I would suggest resolving this by altering the type to make the children prop optional, but a double-check at the ExternalLink code makes me think twice about it. Why do we have an external link without any text? Sounds like it could be an accessibility issue.

Actually, it has as it used by createInterpolateElement function to replace the custom-defined tag with the specified component, and there is the text between the tag that is the text for the ExternalLink. For example,

// Input createInterpolateElement( '{{a}}Learn more{{/a}}', { a: <ExternalLink /> } ); // Output <ExternalLink>Learn more</ExternalLink>

Cool! Then (sorry for going in circles 😅 ), I'd ask the following question: what do you feel is the best in this case - passing children={ null }, ignoring the TS rule, or fixing the ExternalLink type to support an optional children prop`? If you picked one of those, why did you prefer that choice?

I think the ExternalLink still needs the children so we should not make it optional. So, I'd prefer to pass children={ null } or even use ts-expect-error to bypass the rule and see whether we can fix this issue via createInterpolateElement as it should let TS know it will pass the children to that component. However, I'm not whether it's possible or not 😓

Sounds good, thanks for confirming, let's keep the children={ null } then 👍 No need to overcomplicate things.

I also asked about this in Slack a couple weeks ago (it's a broader issue uncovered in the react 18 update): p1684448743933079-slack-CK1QYS6P6

tyxla · 2023-05-26T11:00:50Z

packages/help-center/src/components/help-center-notice.tsx

-							<ExternalLink href={ localizeUrl( 'https://wordpress.org/support/forums/' ) } />
+							<ExternalLink
+								href={ localizeUrl( 'https://wordpress.org/support/forums/' ) }
+								children={ null }


Why is that necessary? children will be undefined by default.

tyxla · 2023-05-26T11:02:16Z

packages/global-styles/src/hooks/use-sync-global-styles-user-config.ts

+					{} as GlobalStylesObject
+				)
+		);
+	}, [ globalStyles ] );


Is there a chance globalStyles will be a new object on every re-render and setUserConfig will be called every time? Should we memoize the config merging and call setUserConfig only if the memoized version changes?

We handle this case outside when using this hook.

Should we memoize the config merging and call setUserConfig only if the memoized version changes?

How could we detect the memoized version changes? If the globalStyles is a new object on every re-render, then the memoized version will be changed as well.

I was mostly thinking out loud and leaving this for your consideration, but if it's handled already, no need to worry about it 👍

tyxla · 2023-05-26T11:03:11Z

packages/global-styles/src/gutenberg-bridge/index.tsx

+import type { GlobalStylesObject } from '../types';
+
+const { unlock } = __dangerousOptInToUnstableAPIsOnlyForCoreModules(
+	'I know using unstable features means my plugin or theme will inevitably break on the next WordPress release.',


Yup, I love it too 😉 Great tactics from a devex perspective. 👍

tyxla · 2023-05-26T11:04:22Z

packages/global-styles/src/components/font-pairing-variations/preview.tsx

+										textAlign: 'center',
+									} }
+								>
+									{ translate( 'Default' ) }


Note that such simple strings often needs a context for the translators to know how to translate it in various languages.

noahtallen · 2023-05-30T21:57:37Z

I'll go ahead and merge this into the main React 18 PR so that we have all the fixes in one place.

arthur791004 · 2023-05-31T02:53:15Z

Thank you!

noahtallen · 2023-06-22T21:59:49Z

I've been doing a lot more work on the React 18 PR, and in an effort to split out much as possible from that PR, I've pinned the WordPress packages to a lower version (versions from around December 2022). The benefit is that we won't need to bundle this fix into the React 18 PR, but we'll still need to land it in the PR after that when we update the WordPress packages to the current versions. So I restored the branch in anticipation of that

noahtallen · 2023-06-27T23:34:41Z

Reopened to merge onto the wordpress package updates: #78715

arthur791004 self-assigned this May 24, 2023

arthur791004 force-pushed the try-react-18-global-styles-pkg branch from ba2b3c3 to 4b85d57 Compare May 24, 2023 10:54

arthur791004 requested a review from noahtallen May 24, 2023 11:11

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

arthur791004 changed the title ~~WIP React 18: Fix @automattic/global-styles package~~ React 18: Fix @automattic/global-styles package May 24, 2023

arthur791004 requested a review from a team May 24, 2023 11:12

noahtallen requested a review from a team May 24, 2023 18:27

arthur791004 force-pushed the try-react-18-global-styles-pkg branch from 4b85d57 to 7c64d8e Compare May 25, 2023 03:51

noahtallen approved these changes May 25, 2023

View reviewed changes

noahtallen reviewed May 25, 2023

View reviewed changes

arthur791004 force-pushed the try-react-18-global-styles-pkg branch 2 times, most recently from 69a48b5 to ce2e393 Compare May 26, 2023 06:35

arthur791004 added 8 commits May 26, 2023 14:36

@automattic/global-styles: Replace @wordpress/edit-site apis with new…

5097365

… ones

Fix types

bb2f670

Use fixed version of private-apis package to make __private symbol co…

62f87b1

…nsistent

Use ExperimentalBlockEditorProvider to initialize Iframe

1c536b3

Fix unable to get fontFamilies

f00df83

Fix styles

d727644

Fix style variation preview

6d642ac

Fix types

335e26e

arthur791004 force-pushed the try-react-18-global-styles-pkg branch from ce2e393 to 335e26e Compare May 26, 2023 06:38

tyxla reviewed May 26, 2023

View reviewed changes

arthur791004 added 4 commits May 29, 2023 14:40

Replace __blank with _blank

953334b

Replace mergeWith with deepmerge

84d4bf3

Get rid of isEmpty and mapValues in global-stypes pkg

53e0730

Add comments

5eef674

arthur791004 force-pushed the try-react-18-global-styles-pkg branch from 8fb2e94 to 5eef674 Compare May 29, 2023 07:22

tyxla mentioned this pull request May 29, 2023

Fix: Replace target="__blank" with target="_blank" #77484

Merged

noahtallen merged this pull request into try-react-18 May 30, 2023

noahtallen deleted the try-react-18-global-styles-pkg branch May 30, 2023 21:57

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 May 30, 2023

arthur791004 mentioned this pull request May 31, 2023

Rewrite @automattic/global-styles to align with Calypso's React 18 upgrade #77048

Closed

2 tasks

noahtallen restored the try-react-18-global-styles-pkg branch June 22, 2023 21:15

noahtallen mentioned this pull request Jun 23, 2023

React 18 #77046

Merged

25 tasks

noahtallen mentioned this pull request Jun 27, 2023

Fix global styles package for new WordPress package versions #78715

Merged

React 18: Fix @automattic/global-styles package #77319

React 18: Fix @automattic/global-styles package #77319

Conversation

arthur791004 commented May 24, 2023 • edited Loading

Proposed Changes

Testing Instructions

Pre-merge Checklist

matticbot commented May 24, 2023 • edited Loading

noahtallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthur791004 May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahtallen May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthur791004 May 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahtallen commented May 30, 2023

arthur791004 commented May 31, 2023

noahtallen commented Jun 22, 2023

noahtallen commented Jun 27, 2023

arthur791004 commented May 24, 2023 •

edited

Loading

matticbot commented May 24, 2023 •

edited

Loading

arthur791004 May 26, 2023 •

edited

Loading

noahtallen May 26, 2023 •

edited

Loading

arthur791004 May 29, 2023 •

edited

Loading