Upgrade to Emotion 11

[sarayourfriend]

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]

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.

[sarayourfriend]

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

[tyxla]

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

[sarayourfriend]

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

[gziolo]

@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]

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]

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]

Should we do that as part of this PR?

[gziolo]

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]

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

[sarayourfriend]

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]

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]

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

[youknowriad]

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

[sarayourfriend]

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]

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

[scinos]

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]

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/core@11.0.0, 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/cache@11.1.3
@emotion/core@11.0.0
@emotion/css@11.1.3
@emotion/memoize@0.7.4
@emotion/native@11.0.0
@emotion/primitives-core@11.0.0
@emotion/serialize@1.0.2
@emotion/sheet@1.0.1
@emotion/utils@1.0.0
css-to-react-native@3.0.0
csstype@3.0.8
postcss-value-parser@4.1.0

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]

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]

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

[sarayourfriend]

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]

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]

Thanks @scinos that fixed it!

[sarayourfriend]

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

[gziolo]

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]

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]

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]

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

[sarayourfriend]

Closing in favor of #32216