Upgrade to Emotion 11 #31476

sarayourfriend · 2021-05-04T19:53:47Z

Description

Upgrades to Emotion 11, includes a workaround for Storybook: storybookjs/storybook#13300 (comment)

This is currently broken, the build will fail not being able to resolve @emotion/styled-base but I'm not sure why it's trying to resolve that anymore. What is still using @emotion/styled^10.0.0?

How has this been tested?

Run Gutenberg with this PR and verify that Emotion components are still working as expected. Check that the StyleProvider is working as expected for iframes in FSE.

Also run storybook npm run storybook:dev and ensure that all Emotion components are working as expected.

Types of changes

Dependency upgrade. Non-breaking change.

Checklist:

My code is tested.
My code follows the WordPress code style.
[N/A] My code follows the accessibility standards.
[N/A] I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

tyxla

I've noticed that Emotion cache requires providing a key, otherwise Storybook ends up being broken.

This is currently broken, the build will fail not being able to resolve @emotion/styled-base but I'm not sure why it's trying to resolve that anymore. What is still using @emotion/styled^10.0.0?

Seems like @storybook/theming is still using @emotion/styled^10.0.27: https://github.com/WordPress/gutenberg/blob/upgrade/emotion-11/package-lock.json#L10617

tyxla · 2021-05-04T20:47:28Z

packages/components/src/style-provider/index.js

@@ -1,7 +1,7 @@
 /**
 * External dependencies
 */
-import { CacheProvider } from '@emotion/core';
+import { CacheProvider } from '@emotion/react';


Down there when we invoke createCache, we'll need to provide it with a key prop, otherwise I'm getting an error:

Simply changing:

return createCache( { container } );

to something like:

return createCache( { key: 'something', container } );

should do the job IMHO.

But StyleProvider could be used any number of places. I think we'd probably want an instance id?

Yeah, but at the same time do we really need multiple caches?

Yes 🙂 We need one per iframe for iframes to receive styling.

gziolo · 2021-05-05T06:44:49Z

@sarayourfriend, thank you for starting this PR.

I assume that we plan to use emotion outside of @wordpress/components, right? We should consider making a proxy for all emotion methods used to account for future updates to the public API they have. This PR is a great example of why using external libraries directly isn't a great fit for projects that need to ensure backward compatibility and predictable extensibility. We use proxies for:

Reakit (https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/composite/index.js)
React (https://github.com/WordPress/gutenberg/blob/trunk/packages/element/src/react.js)
Redux (https://github.com/WordPress/gutenberg/blob/3da717b8d0ac7d7821fc6d0475695ccf3ae2829f/packages/data/src/redux-store/index.js) - completely custom abstraction on top of it

This gives us room to bridge future breaking changes, but it also ensures that in the WordPress context those libraries are bundled only into one chunk/module.

Aside: It looks like Storybook is becoming a bottleneck for keeping tools and libraries up to date:

PostCSS 8 Build Tooling: Update development tools to work with PostCSS 8 #27821
webpack 5 Upgrade webpack to version 5 #26382
now emotion 11

sarayourfriend · 2021-05-05T13:57:49Z

We should consider making a proxy for all emotion methods used to account for future updates to the public API they have

Do you mean just re-exporting from a particular place? Would that be a new package or just a re-export from components?

gziolo · 2021-05-05T14:01:10Z

We should consider making a proxy for all emotion methods used to account for future updates to the public API they have

Do you mean just re-exporting from a particular place? Would that be a new package or just a re-export from components?

From @wordpress/components if ever need to. Internally in @wordpress/components we could re-export APIs we use from a single file as presented in the examples I shared earlier.

sarayourfriend · 2021-05-05T14:03:14Z

Should we do that as part of this PR?

gziolo · 2021-05-05T14:09:02Z

Should we do that as part of this PR?

It's all up to you. If we had that earlier, this PR would be a few import statements to update in a single file rather than in 50 😄

youknowriad · 2021-05-05T14:40:05Z

I'm personally not certain at all that we'd ever want to introduce emotion to other packages than components

sarayourfriend · 2021-05-05T15:37:15Z

I think we don't need a proxy necessarily... it's one more thing in the eco-system for JS devs to keep track of when they could just normally import from the regular places (we already have to learn about @wordpress/element etc).

If we're not going to use it outside of components is it really worth doing and further adding to the complexity of this project?

Maybe it's something to revisit if we begin to use it outside of components?

Additionally, PRs like this should be one in a thousand. Presumably they're not going to change their package names all over again.

sarayourfriend · 2021-05-06T13:33:06Z

I'm not sure how to proceed on this PR. Can @youknowriad or @gziolo give me some guidance? And maybe also rope in someone who knows more about build systems than I do to help figure out how to get @emotion/styled to always resolve to version 11? Right now in the regular build it still resolves to version 10 which depends on the no-longer-existant styled-base.

sarayourfriend · 2021-05-17T22:29:41Z

@youknowriad why would the PHP tests fail for this PR? Any ideas?

youknowriad · 2021-05-18T08:27:50Z

@sarayourfriend can't think of anything based on the list of changes. Did you try rebasing the PR?

sarayourfriend · 2021-05-19T17:50:27Z

I'm unable to reproduce the type error locally 🤔 Adding @emotion/react to the dev dependencies should have resolved this but it hasn't. Not sure what else to try... Maybe @sirreal if you care to take a look, if you have time, maybe you can sniff out what's going on here?

It seems like that the root cause of the failures at the moment. Once we fix that we should be able to move forward with the rest of the PR.

sirreal · 2021-05-20T16:50:09Z

I've been unable to reproduce the build issues either. Could there be some dependency caching that's causing problems?

scinos · 2021-05-24T14:42:16Z

Looking at the logs I think I know why I wasn't able to reproduce the issue:

Run preactjs/compressed-size-action@7d87f60a6b0c7d193b8183ce859ed00b356ea92f
PR #31476 is targetted at trunk (8dad8c8687c86c20b46c3ac682eab8c1c682069e)
[current] Install Dependencies
[current] Build using npm
/usr/bin/git reset --hard
HEAD is now at 0db3345 Merge a0bbaa4a96ae81ae8298329cb009210b3e2f796c into 2db0d5c100c6560915e53384beb2f51ed7180c34
[base] Checkout target branch
[base] Install Dependencies
[base] Build using npm

This action is:

Checking out the branch
Running npm ci
Running npm run build and it works.
Checking out trunk
Running npm ci again
Running npm run build again and it fails.

I was only testing the first two steps. Likely there are some cache/node_modules leftovers that are not clean in the second npm run build that are causing the problem

scinos · 2021-05-24T15:12:17Z

I can reproduce this locally. This is what is going on:

The action preactjs/compressed-size-action checks out upgrade/emotion-11 and runs npm ci.
upgrade/emotion-11 defines some new dependencies for packages/components that can't be hoisted. As such, we end up with node_modules/@emotion/core (v 10.1.1) and packages/components/node_modules/@emotion/core (v 11.0.0).
The action then checks out trunk and runs npm ci.
This cleans up node_modules but it does not clean up packages/components/node_modules. So any package inside packages/components that requires @emotion/core will load 11.0.0

I'm not sure what is the root problem where:

I'd expect npm ci to clean up node_modules and **/node_modules, but it does not. I don't know if it is supposed to do.
The action should ensure the directory is completely clean before checking out trunk but it does not. Likely they expect npm ci to clean up **/node_modules? The reality is that git checkout upgrade/emotion-11 && npm ci && git checkout trunk && npm ci is not the same as just git checkout trunk && npm ci

Possible solutions:

Patch the action to clean up local directory before checking out trunk. I think this is my prefered solution
Force the whole repo to use @emotion/[email protected], so we only have node_modules/@emotion/core and not packages/components/node_modules/@emotion/core. But this can blow up at any moment in the future.
Patch npm ci to clean up **/node_modules :goodluck:

For the record, I'm using @emotion/core as an example but there are more leftovers in packages/components/node_modules:

@emotion/[email protected]
@emotion/[email protected]
@emotion/[email protected]
@emotion/[email protected]
@emotion/[email protected]
@emotion/[email protected]
@emotion/[email protected]
@emotion/[email protected]
@emotion/[email protected]
[email protected]
[email protected]
[email protected]

Running npm ci on this branch and then on trunk leaves those packages in packages/components/node_modules, while a clean checkout of trunk has none of those. I assume @emotion/core is causing the main problem, but once that is solved any of those packages may cause it to blow up.

sirreal · 2021-05-24T15:16:06Z

I'd expect npm ci to clean up node_modules and **/node_modules, but it does not. I don't know if it is supposed to do.

I can't find it documented anywhere, but it does not remove nested node_modules.

sarayourfriend · 2021-05-24T16:10:01Z

Oh shoot @sirreal I'm so sorry, in rebasing I completely over-wrote your changes 😞 I'll try to fix this ASAP.

sarayourfriend · 2021-05-24T16:11:01Z

Actually never mind, I'm not sure how to replicate your changes. I'm so so sorry 😞 Maybe @scinos since you're already looking at this you can help fix my mistake? Again, so sorry y'all!

scinos · 2021-05-24T16:57:07Z

I don't have permissions :(

This is what I was trying to do to restore the branch:

$ git checkout upgrade/emotion-11
$ git reset --hard a0bbaa4a96ae81ae8298329cb009210b3e2f796c
$ git push --force-with-lease

sarayourfriend · 2021-05-24T17:18:37Z

Thanks @scinos that fixed it!

sarayourfriend · 2021-05-24T19:24:00Z

@gziolo What do you think of @scinos 's recommendations?

gziolo · 2021-05-24T19:28:34Z

You can also try this approach from https://cpojer.net/posts/dependency-managers-dont-manage-your-dependencies:

None of these worked well and often made things worse. I’ve found only one reliable way to ensure only one version of each Babel package is installed in a project: On every Babel upgrade, manually remove all entries from the yarn.lock file starting with @babel/, which gives Yarn a chance to start over for a subset of its dependency graph. You’ll end up with a single version of each Babel package – the latest ones.

You can even go one step further and install all new dependencies in the root package.json before you run npm i. Then remove them from the main package.json when everything works. It will keep the version already installed.

scinos · 2021-05-24T20:48:33Z

FWIW, I think the problem here is the action.

We have shown that the pattern checkout A && npm ci && checkout B && npm ci is not safe. It needs a manual clean up of **/node_modules before checking out any new branch, otherwise you'll end up with a tainted local copy. And the action preactjs/compressed-size-action does exactly that.

Unrelated on how we finally go with the emotion update, I'd suggest to get rid of any process (that includes the action) that assumes that running npm ci with an existing **/node_modules is safe.

sarayourfriend · 2021-05-24T23:40:14Z

preactjs/compressed-size-action

That action allows you to specify an alternative build-script to ci: https://github.com/preactjs/compressed-size-action#customizing-the-build

Can we use that to maintain the current build pipelines but get around this issue (probably in a separate PR)?

sarayourfriend · 2021-05-25T18:07:55Z

@scinos I'm going to open this in a fork so you can have access to contribute if that works for you?

sarayourfriend · 2021-05-25T18:11:13Z

Closing in favor of #32216

sarayourfriend added npm Packages Related to npm packages [Package] Components /packages/components Storybook Storybook and its stories for components labels May 4, 2021

sarayourfriend requested review from youknowriad, gziolo, diegohaz and tyxla May 4, 2021 19:53

sarayourfriend requested review from ajitbohra and chrisvanpatten as code owners May 4, 2021 19:53

sarayourfriend self-assigned this May 4, 2021

tyxla reviewed May 4, 2021

View reviewed changes

sarayourfriend mentioned this pull request May 4, 2021

Fully integrating vanilla Emotion as the canonical style system for CSS-in-JS in Gutenberg #30503

Closed

sarayourfriend mentioned this pull request May 17, 2021

Revert "Revert "components: Promote VisuallyHidden from ui into full components (#31244)"" #31902

Merged

sarayourfriend force-pushed the upgrade/emotion-11 branch from 269f8e3 to 736f0d9 Compare May 17, 2021 21:20

sarayourfriend requested a review from ellatrix as a code owner May 17, 2021 21:55

sarayourfriend force-pushed the upgrade/emotion-11 branch from fcc43f4 to 77926be Compare May 18, 2021 12:51

sarayourfriend requested a review from ciampo May 18, 2021 14:57

sarayourfriend force-pushed the upgrade/emotion-11 branch from 6279c92 to 4e05de2 Compare May 19, 2021 13:36

sarayourfriend force-pushed the upgrade/emotion-11 branch from a0bbaa4 to bad04b7 Compare May 24, 2021 16:09

sarayourfriend force-pushed the upgrade/emotion-11 branch 2 times, most recently from a0bbaa4 to 66cbc2b Compare May 24, 2021 17:18

sarayourfriend and others added 9 commits May 25, 2021 07:32

Upgrade to Emotion 11

bc072fd

Give each cache a unique key

c43dd57

Update remaining emotion dependencies

5afba78

Update stray bad import

dacdfaa

Fix emotion/jest serializer specification

948deb0

Update snapshots and tests to match new serializer style

b3dae36

Update native config

5e7f874

Add @emotion/react for type dependency

3ff61fd

Manual dedupe @emotion/styled

d0d9805

sarayourfriend force-pushed the upgrade/emotion-11 branch from 66cbc2b to d0d9805 Compare May 25, 2021 14:33

sarayourfriend mentioned this pull request May 25, 2021

Upgrade to Emotion 11 #32216

Closed

5 tasks

sarayourfriend closed this May 25, 2021

scinos mentioned this pull request May 25, 2021

Local dir is not clean up properly before switching to target branch preactjs/compressed-size-action#59

Closed

sirreal deleted the upgrade/emotion-11 branch December 29, 2023 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Emotion 11 #31476

Upgrade to Emotion 11 #31476

sarayourfriend commented May 4, 2021

tyxla left a comment

tyxla May 4, 2021

sarayourfriend May 4, 2021

tyxla May 6, 2021

sarayourfriend May 6, 2021

gziolo commented May 5, 2021

sarayourfriend commented May 5, 2021

gziolo commented May 5, 2021

sarayourfriend commented May 5, 2021

gziolo commented May 5, 2021

youknowriad commented May 5, 2021

sarayourfriend commented May 5, 2021

sarayourfriend commented May 6, 2021

sarayourfriend commented May 17, 2021

youknowriad commented May 18, 2021

sarayourfriend commented May 19, 2021

sirreal commented May 20, 2021

scinos commented May 24, 2021 •

edited

Loading

scinos commented May 24, 2021 •

edited

Loading

sirreal commented May 24, 2021

sarayourfriend commented May 24, 2021

sarayourfriend commented May 24, 2021

scinos commented May 24, 2021

sarayourfriend commented May 24, 2021

sarayourfriend commented May 24, 2021

gziolo commented May 24, 2021

scinos commented May 24, 2021

sarayourfriend commented May 24, 2021

sarayourfriend commented May 25, 2021

sarayourfriend commented May 25, 2021

Upgrade to Emotion 11 #31476

Upgrade to Emotion 11 #31476

Conversation

sarayourfriend commented May 4, 2021

Description

How has this been tested?

Types of changes

Checklist:

tyxla left a comment

Choose a reason for hiding this comment

tyxla May 4, 2021

Choose a reason for hiding this comment

sarayourfriend May 4, 2021

Choose a reason for hiding this comment

tyxla May 6, 2021

Choose a reason for hiding this comment

sarayourfriend May 6, 2021

Choose a reason for hiding this comment

gziolo commented May 5, 2021

sarayourfriend commented May 5, 2021

gziolo commented May 5, 2021

sarayourfriend commented May 5, 2021

gziolo commented May 5, 2021

youknowriad commented May 5, 2021

sarayourfriend commented May 5, 2021

sarayourfriend commented May 6, 2021

sarayourfriend commented May 17, 2021

youknowriad commented May 18, 2021

sarayourfriend commented May 19, 2021

sirreal commented May 20, 2021

scinos commented May 24, 2021 • edited Loading

scinos commented May 24, 2021 • edited Loading

sirreal commented May 24, 2021

sarayourfriend commented May 24, 2021

sarayourfriend commented May 24, 2021

scinos commented May 24, 2021

sarayourfriend commented May 24, 2021

sarayourfriend commented May 24, 2021

gziolo commented May 24, 2021

scinos commented May 24, 2021

sarayourfriend commented May 24, 2021

sarayourfriend commented May 25, 2021

sarayourfriend commented May 25, 2021

scinos commented May 24, 2021 •

edited

Loading

scinos commented May 24, 2021 •

edited

Loading