Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migration to Emotion 11 #17640

Merged
merged 20 commits into from Apr 7, 2022
Merged

Migration to Emotion 11 #17640

merged 20 commits into from Apr 7, 2022

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Mar 6, 2022

Issue:

What I did

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Mar 6, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a263bf7. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

# Conflicts:
#	addons/docs/package.json
#	addons/storyshots/storyshots-core/package.json
#	lib/ui/src/containers/menu.tsx
#	yarn.lock
@ndelangen ndelangen self-requested a review March 21, 2022 14:43
lib/theming/src/index.ts Outdated Show resolved Hide resolved
@ndelangen
Copy link
Member

amazing work, what's left to do?

@Andarist
Copy link
Contributor Author

I got to the bottom of those changes in the baselines.

TLDR - there was probably a parsing bug in Stylis v3 (or in @emotion/stylis) and the previous version was incorrect. The new one is actually correct, both on the technical level and also visually.

Those styles here were producing this css~ string:

&&{border-collapse:collapse;border-spacing:0;color:#FFFFFF;td, th{padding:0;border:none;vertical-align:top;text-overflow:ellipsis;}font-size:13px;line-height:20px;text-align:left;width:100%;margin-top:25px;margin-bottom:40px;thead th:first-of-type, td:first-of-type{width:25%;}th:first-of-type, td:first-of-type{padding-left:20px;}th:nth-of-type(2), td:nth-of-type(2){width:35%;}td:nth-of-type(3){width:15%;}th:last-of-type, td:last-of-type{padding-right:20px;width:25%;}th{color:rgba(255,255,255,0.55);padding-top:10px;padding-bottom:10px;padding-left:15px;padding-right:15px;}td{padding-top:10px;padding-bottom:10px;&:not(:first-of-type){padding-left:15px;padding-right:15px;}&:last-of-type{padding-right:20px;}}margin-left:1px;margin-right:1px;tr:first-child/* emotion-disable-server-rendering-unsafe-selector-warning-please-do-not-use-this-the-warning-exists-for-a-reason */{td:first-child/* emotion-disable-server-rendering-unsafe-selector-warning-please-do-not-use-this-the-warning-exists-for-a-reason */, th:first-child/* emotion-disable-server-rendering-unsafe-selector-warning-please-do-not-use-this-the-warning-exists-for-a-reason */{border-top-left-radius:4px;}td:last-child/* emotion-disable-server-rendering-unsafe-selector-warning-please-do-not-use-this-the-warning-exists-for-a-reason */, th:last-child/* emotion-disable-server-rendering-unsafe-selector-warning-please-do-not-use-this-the-warning-exists-for-a-reason */{border-top-right-radius:4px;}}tr:last-child/* emotion-disable-server-rendering-unsafe-selector-warning-please-do-not-use-this-the-warning-exists-for-a-reason */{td:first-child/* emotion-disable-server-rendering-unsafe-selector-warning-please-do-not-use-this-the-warning-exists-for-a-reason */, th:first-child/* emotion-disable-server-rendering-unsafe-selector-warning-please-do-not-use-this-the-warning-exists-for-a-reason */{border-bottom-left-radius:4px;}td:last-child/* emotion-disable-server-rendering-unsafe-selector-warning-please-do-not-use-this-the-warning-exists-for-a-reason */, th:last-child/* emotion-disable-server-rendering-unsafe-selector-warning-please-do-not-use-this-the-warning-exists-for-a-reason */{border-bottom-right-radius:4px;}}tbody{box-shadow:rgba(0, 0, 0, 0.20) 0 2px 5px 1px,
          rgba(255,255,255,0.15) 0 0 0 1px;border-radius:4px;@media not all and (min-resolution:.001dpcm){@supports (-webkit-appearance:none){border-width:1px;border-style:solid;border-color:rgba(255,255,255,0.15);}}tr{background:transparent;overflow:hidden;&:not(:first-child/* emotion-disable-server-rendering-unsafe-selector-warning-please-do-not-use-this-the-warning-exists-for-a-reason */){border-top-width:1px;border-top-style:solid;border-top-color:#404040;}}td{background:#333333;}}}th span, td span, td button{display:inline;background-color:rgba(255,255,255,.1);animation:animation-r0iffl 1.5s ease-in-out infinite;color:transparent;box-shadow:none;border-radius:0;}

In here basically, the whole block is wrapped with && { ... }, so everything inside should produce rules with a "double scope". However, with Emotion 10 we got such a rule out of this:

.css-1eaai6p td:last-child,.css-1eaai6p th:last-child{border-top-right-radius:4px;}

We can notice that it doesn't contain the double scope which is definitely not correct. From the very same string the new version produces such a rule:

.css-1eaai6p.css-1eaai6p tr:last-child td:last-child,.css-1eaai6p.css-1eaai6p tr:last-child th:last-child{border-bottom-right-radius:4px;}

We can also notice that the old one has only a single :last-child whereas the correct one has :last-child :last-child in it.

This caused a minor visual bug in the old version and that was fixed with this dependency upgrade.

A funny thing is that if I either:

  • prettify this CSS string
  • OR remove "ignoreSsrWarning" comments

then the whole thing parses correctly with the old version. So there was definitely some edge-case bug in the parsing there.

@ndelangen
Copy link
Member

I think this replaces this PR: #13300

import initStoryshots from '../dist/ts3.9';

initStoryshots({
framework: 'react',
configPath: path.join(__dirname, '..', '.storybook'),
renderer: mount,
snapshotSerializers: [enzymeSerializer(), emotionSerializer()],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was creating an infinite loop because the emotion serializer is already used in the root jest.config.js and this config was merged with this one here. As the result - 2 emotion serializers were applied here and this messed them up.

More details can be found here:
emotion-js/emotion#2704

@@ -1,7 +1,7 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Storyshots Demo/AccountForm Standard 1`] = `
.emotion-15 {
.emotion-0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those snapshots just have different generated "indices" for those masked class names

@@ -0,0 +1,266 @@
import { Interpolation } from '@emotion/react';
Copy link
Contributor Author

@Andarist Andarist Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's basically just a copy-paste (adjusted one) from @emotion/styled@10

Copy link
Member

@ndelangen ndelangen Apr 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a little note telling us to remove this when releasing 7.0

@Andarist Andarist changed the title [WIP] Migration to Emotion 11 Migration to Emotion 11 Mar 31, 2022
@Andarist Andarist marked this pull request as ready for review March 31, 2022 08:07
@Andarist Andarist requested a review from ndelangen March 31, 2022 08:31
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So exciting!! A couple bits

lib/cli/src/versions.ts Outdated Show resolved Hide resolved
lib/components/src/controls/Object.tsx Show resolved Hide resolved
# Conflicts:
#	addons/storyshots/storyshots-core/package.json
#	examples/official-storybook/package.json
#	lib/cli/src/versions.ts
#	yarn.lock
@ndelangen ndelangen mentioned this pull request Apr 7, 2022
# Conflicts:
#	addons/storyshots/storyshots-core/package.json
#	examples/official-storybook/package.json
#	lib/cli/src/versions.ts
#	yarn.lock
…tion11

# Conflicts:
#	addons/storyshots/storyshots-core/package.json
#	lib/cli/src/versions.ts
#	yarn.lock

import { Theme } from './types';

export { CacheProvider };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Andarist I think this CacheProvider export is now removed, can we add this back?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, never mind it IS exported on line 13!


import { Theme } from './types';

export { CacheProvider };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, never mind it IS exported on line 13!

@ndelangen
Copy link
Member

I'll merge as soon as CI is green(-ish)

@ndelangen
Copy link
Member

going to ignore this error:

[vite]: Rollup failed to resolve import "react-dom/client" from "node_modules/@storybook/react/dist/esm/client/preview/render.js".
This is most likely unintended because it can break your application at runtime.
If you do want to externalize this module explicitly add it to
build.rollupOptions.external

for not, since @IanVS is fixing that issue (caused by #17215)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants