Fix moving of client-side inserted style tags from Emotion 10 when intending to hydrate Emotion 11 styles resulting in losing styles

[danieldelcore]

Closes: #2210
Related: #2209

What:

• @emotion/cache currently modifies style tags 'belonging' to other cacheProviders.
• @emotion/cache also currently searches for style tags in both the head and body and remounts them to the head.

Why:

• To fix issues when running multiple emotion instances & multiple cache providers

How:

• We limited the selector to search for tags only in the body to ensure that the head is not modified (and styles mounted via CSSOM are not lost)
• We made sure that we only modify elements with a cache key that is relevant to that instance

Checklist:

• Documentation N/A
• Tests
• Code complete
• Changeset

Reproduction

https://github.com/seancurtis/emotion-concurrency

Screen.Recording.2021-04-28.at.4.07.29.pm.mov

Emotion11.mp4

[changeset-bot]

🦋 Changeset detected

Latest commit: b80e1c6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@emotion/cache	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b80e1c6:

Sandbox	Source
Emotion	Configuration

[codecov]

Codecov Report

Merging #2361 (5931357) into main (780bc17) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 5931357 differs from pull request most recent head b80e1c6. Consider uploading reports for the commit b80e1c6 to get more accurate results
| Impacted Files | Coverage Δ | |
|---|---|---|
| packages/cache/src/index.js | 97.80% <100.00%> (+0.02%) | ⬆️ |

[emmatown]

I've updated this so it fixes the issue. The issue is very specific to v10 and v11 on the same page, multiple Emotion 11 caches already don't suffer from this problem. (ofc they will suffer from other issues but nothing as problematic as destroying all previously inserted client-side inserted styles from Emotion 10)

We limited the selector to search for tags only in the body to ensure that the head is not modified (and styles mounted via CSSOM are not lost)

This wouldn't have solved the issue completely because it would still have the issue if someone used Emotion 10 with a custom container that was the body element or inside the body.

We made sure that we only modify elements with a cache key that is relevant to that instance

That code very intentionally targets all Emotion 11 server rendered style elements(and unintentionally before this change, it targeted client-side inserted Emotion 10 styles which caused the problem you were having) because they have to be moved before the React hydration starts. If we did it on a cache-specific basis and you created a cache inside the render of a React component, the style elements would be moved too late and React would warn and break things, e.g. https://codesandbox.io/s/blissful-fog-r73qb.

[Andarist]

I'm wondering - was this supposed to test a fix or is it just a new test for something that was not tested up until now?

[danieldelcore]

Hey 👋 yeah it was a bit of both, without the fix for filtering the cache key (data-emotion="emo"), both style tags would be removed. AFAIK that's why we were having issues using the CacheProvider.

[emmatown]

This test doesn't test any changed behavior, the styles in this test are both from server-rendering which was already handled correctly.

[Andarist]

based on this assertion it's rather hard to see if this asserts the correct thing - maybe it would be better to render those Emotion 10 styles to the body? that way we'd know that they have not been moved to the head

[danieldelcore]

Yea this test is a bit of a weird one tbh. It's intended to assert the cause of our original issue which is essentially style tags on the head being re-mounted to the head when a cache provider instantiates itself, resulting in CSSOM styles being lost.

Since we're not able to test CSSOM styles in jest, I feel like it's probably a good idea just to remove it instead.
What do you think? 😄

[emmatown]

This is the only test that tests behavior that has changed, I've updated it so it's more clear

[Andarist]

ofc they will suffer from other issues but nothing as problematic as destroying all previously inserted client-side inserted styles from Emotion 10

Do you have any particular problems in mind? We probably should document or fix potential problems.

---

I think the possible permutations of this problem are:

First	Second	Assessment
SSRed 10	init different 10	I think we can just skip over this since this has been long enough in production that people worked around this
client 10	init different 10	Same as above
SSRed 10	init 11	This is not a problem since the SSR format of Emotion 10 was slightly different (key in the attribute key and not in the value)
client 10	init 11	This is being fixed by this PR
SSRed 11	init different 11	Handled OK thanks to data-s
client 11	init different 11	Same as above
SSRed 11	init 10	This is not a problem since the SSR format of Emotion 10 was slightly different (key in the attribute key and not in the value)
client 11	init 10	Same as above

We can't deal with clashing keys in any combination and our recommendation is to always use unique keys if there is a chance of a clash (like for example if somebody develops a browser extension or an embeddable widget).

Have I missed something?

[danieldelcore]

This is fantastic! Thanks so much for jumping in @mitchellhamilton @Andarist ❤️.
I'll work on that feedback today 👍

[emmatown]

Do you have any particular problems in mind? We probably should document or fix potential problems.

What I meant was using the same key is always going to have some problems and they are:

• Using stylisPlugins - you absolutely MUST use a different key when using custom stylis plugins because we hash the styles before they go through stylis so without the hashing so without using a different key, you will have different styles
• Flushing after SSR - it's unclear which cache owns the styles(i believe how it works right now, multiple caches can own it?) so it might be removed
• custom containers after SSR(+prepend after SSR i think?) - things might be moved to the wrong container after SSR because it's unclear which cache owns the styles

For people who don't care about these things(which is the vast majority of people imo), multiple caches with the same key should work fine. I'm not saying people should intentionally use the same cache key but they will probably unintentionally use multiple caches with the same key because different versions of emotion will be run in the same app.

SSRed 10 | init different 10 | I think we can just skip over this since this has been long enough in production that people worked around this

What's the problem you're referencing here that people have worked around(as in specifically this row)?

[danieldelcore]

Hey folks, just pushed up some changes to address your feedback. LMK if there's anything else you'd like me to look at 👍

[MatthewCharlton]

Are there any other reasons why 'css' was used as the default key and not some kind of unique ID generated at runtime?

[emmatown]

Are there any other reasons why 'css' was used as the default key and not some kind of unique ID generated at runtime?

We need keys to match from a server-render to a client-render.

[Andarist]

What's the problem you're referencing here that people have worked around(as in specifically this row)?

I'm not aware of any problems there - I've just skipped thinking about those entirely but included them in the table for completeness of the listed combinations 😅

[jacek-leoniec]

Any timeline when this will be released?