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

Core: Revert Emotion :first-child (etc) workarounds #21213

Merged
merged 7 commits into from Apr 5, 2023

Conversation

tmeasday
Copy link
Member

Closes #20730

What I did

As discussed here, I reverted #18361.

In the end there were no other styles that needed to be escaped.

How to test

Run a sandbox, check both story + docs rendering.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@tmeasday
Copy link
Member Author

@Andarist I am still seeing a problem on this branch. It seems like nested usage of the comment is not working (I'll call out the line of code that's causing the issue):

image

@ndelangen
Copy link
Member

@tmeasday let's discuss, when you're available?

@Andarist
Copy link
Contributor

Andarist commented Mar 2, 2023

Oh, I missed this notification last week - sorry!. Could you give me repro steps so I could jump directly into investigating this?

@tmeasday
Copy link
Member Author

tmeasday commented Mar 2, 2023

Hey @Andarist sure thing, here are the instructions:

  1. Check out this branch: fix-emotion-warnings-for-good
  2. Run yarn start from the root
  3. Browse to any story file, e.g. http://localhost:6006/?path=/story/example-button--primary
  4. Also browse to any attached docs file, e.g. http://localhost:6006/?path=/docs/example-button--docs

I'm not sure if 3&4 are the same issue or not.

@tmeasday
Copy link
Member Author

tmeasday commented Mar 7, 2023

@Andarist let me know if you need further help reproducing

@Andarist
Copy link
Contributor

Andarist commented Mar 7, 2023

Thanks for this I extracted a repro case for myself, it happens with those styles:

&&{border-spacing:0;color:#C9CDCF;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:0;margin-bottom:0;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){}td:nth-of-type(3){}th:last-of-type, td:last-of-type{padding-right:20px;}th{color:rgba(201,205,207,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:0;margin-right:0;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:0;}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:0;}}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:0;}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:0;}}tbody{tr{background:transparent;overflow:hidden;border-top-width:1px;border-top-style:solid;border-top-color:#27292a;}td{background:#1B1C1D;border-top:1px solid rgba(255,255,255,.1);}}}

Gonna investigate this - thanks for the ping. In this case, the element.children contain the comment but we search for it in the parent's children. I need to take another look at how Stylis parses this.

@tmeasday
Copy link
Member Author

@Andarist any luck? We are hoping to release soon, I'm wondering if we should just fallback to the Cache Provider solution in docs.

@Andarist
Copy link
Contributor

I’ll get to it over the weekend

@Andarist
Copy link
Contributor

I've diagnosed the problem - now I "only" need to figure out the fix. I hate the code responsible for those warnings 🤣

@Andarist Andarist mentioned this pull request Apr 5, 2023
3 tasks
@Andarist
Copy link
Contributor

Andarist commented Apr 5, 2023

I fixed the issue in Emotion (here), bumped the @emotion/cache in this repo here and merged next into this branch. This fixed almost all of the issues.

One issue persisted though and I diagnosed it as a bug in stylis and I reported it here. Luckily, it was easy to workaround it by just moving the comment outside of the parenthesis so I've done that and pushed it out.

Let me know if you find this warning being printed in any other case.

@tmeasday
Copy link
Member Author

tmeasday commented Apr 5, 2023

Amazing @Andarist!

@tmeasday
Copy link
Member Author

tmeasday commented Apr 5, 2023

@Andarist (or @ndelangen) can you approve (because I created the PR)?

@tmeasday tmeasday added patch:yes Bugfix & documentation PR that need to be picked to main branch and removed ci: do not merge labels Apr 5, 2023
@Andarist
Copy link
Contributor

Andarist commented Apr 5, 2023

a girl self-patting herself and saying 'you are welcome everyone'

The relevant code was already removed on `next` in 5892ea4
@shilman shilman merged commit 773fdab into next Apr 5, 2023
57 checks passed
@shilman shilman deleted the fix-emotion-warnings-for-good branch April 5, 2023 12:46
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Apr 6, 2023
shilman added a commit that referenced this pull request Apr 6, 2023
Core: Revert Emotion `:first-child` (etc) workarounds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: docs bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove emotion first-child warnings in docs
4 participants