fix(styled): Ensure no hydration mismatch with React.useId #2542

eps1lon · 2021-11-09T11:34:59Z

What:

Fixes reactwg/react-18#111 (reply in thread)

Why:

If React.useId is used then emotion currently causes hydration mismatches

How:

Render same amount of siblings on client and server (<style /> is replaced with <React.Fragment /> on the client).
Pattern is copied from https://github.com/vercel/next.js/pull/31102/files#diff-63632e6e580e69692941c89bab0a0e0436ade3f9543ef610925f795928d9d5fdR579-R612.
Fix is verified in https://codesandbox.io/s/friendly-bassi-6buos?file=/src/App.js:410-429

Checklist:

~~[ ]~~ Documentation Don't think this needs documentation
Tests Unclear if you have a hydration test suite.
Broken with @emotion/styled@11.3.0: https://codesandbox.io/s/sad-breeze-qznnl?file=/src/Sidebar.js
Fixed with this PR: https://codesandbox.io/s/serene-platform-r4eyr?file=/src/Sidebar.js
Code complete
Changeset

changeset-bot · 2021-11-09T11:35:04Z

🦋 Changeset detected

Latest commit: d386c78

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

This PR includes changesets to release 3 packages

Name	Type
@emotion/jest	Minor
@emotion/react	Minor
@emotion/styled	Minor

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 · 2021-11-09T11:40:49Z

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 d386c78:

Sandbox	Source
Emotion	Configuration
romantic-euclid-kzgl2	PR
modest-newton-s9yhj	PR
beautiful-mendel-bbhit	PR

codecov · 2021-11-09T11:41:04Z

Codecov Report

Merging #2542 (76ddec5) into main (516fe45) will increase coverage by 0.19%.
The diff coverage is 97.22%.

❗ Current head 76ddec5 differs from pull request most recent head d386c78. Consider uploading reports for the commit d386c78 to get more accurate results
| Impacted Files | Coverage Δ | |
|---|---|---|
| packages/jest/src/utils.js | 96.47% <90.90%> (-0.43%) | ⬇️ |
| packages/jest/src/create-enzyme-serializer.js | 97.36% <96.55%> (-2.64%) | ⬇️ |
| packages/jest/src/create-serializer.js | 100.00% <100.00%> (+6.31%) | ⬆️ |
| packages/jest/src/enzyme-tickler.js | 100.00% <100.00%> (ø) | |
| packages/react/src/class-names.js | 100.00% <100.00%> (ø) | |
| packages/react/src/emotion-element.js | 100.00% <100.00%> (ø) | |
| packages/styled/src/base.js | 100.00% <100.00%> (ø) | |

Andarist · 2021-11-09T16:49:48Z

Thank you for taking a look at this - since your comment last week I was planning to get to it but you beat me to it :) I was actually thinking if adding a fake element like this would fix the issue - I'm glad that we are able to resolve this without ditching the zero-config approach 👍

Andarist · 2021-11-09T16:50:19Z

packages/jest/src/utils.js

+  // TODO: Does approach work with older Emotion versions? Does it cause false positives with components named "Fragment" that aren't `React.Fragment`?
+  const node =
+    nodeWithFragment.name() === 'Fragment'
+      ? nodeWithFragment.children()
+      : nodeWithFragment


I'll take a look into this issue and resolve this TODO comment before merging this

@eps1lon i've tweaked this and left the comment about the chosen solution, what do you think - is relying on Symbol.for OK here or should we add react-is as a dependency and use that?

react-is comes with its own problems (e.g. if it's a dependency people might have a different version of React installed. See facebook/react#20099 for more details).

I'd say pick the one that you're most comfortable with and see if it works for the scenarios that it's used with.

thanks for the feedback, I've noticed later that we are actually already using Symbol.for for different checks, so I'm just going to stick with this strategy

Andarist · 2021-11-09T16:51:46Z

packages/jest/test/__snapshots__/react-enzyme.test.js.snap

->
-  Hello
-</div>
+<Fragment>


I wonder if this shouldn't render <> but I'm not sure at the moment if this is under our control, gonna check this out

I've fixed the serialization tests here - but I have a feeling that our test suite is missing some cases that should currently fail. I will do some further investigation soon.

Andarist · 2021-11-09T17:01:06Z

packages/styled/src/base.js

@@ -132,27 +132,31 @@ let createStyled: CreateStyled = (tag: any, options?: StyledOptions) => {
        newProps.ref = ref

        const ele = React.createElement(finalTag, newProps)
+        let possiblyStyleElement = <></>


Do u happen to know if there is any potential perf difference between using a fragment like this and using a Noop component like in the referenced Next.js PR? The intuition tells me that if there is any then probably the fragment approach is better but it probably also doesn't matter

I noticed this as well when going over the referenced PR again. Maybe Fragments are less robust if they're implemented differently in various JSX runtimes. A no-op (() => null) is probably safer.

But even if there's no difference, let's follow nextjs (first come, first serve) to establish a de-facto standard.

Andarist · 2021-11-09T17:05:55Z

packages/styled/src/base.js

          )
        }
-        return ele
+        // Need to return the same number of siblings or else `React.useId` will cause hydration mismatches.


the same fix, most likely, should also be applied in those places:

emotion/packages/react/src/class-names.js

Line 142 in 516fe45

return ele

emotion/packages/react/src/emotion-element.js

Line 144 in 516fe45

return ele

emotion/packages/react/src/global.js

Line 137 in 516fe45

return null

(maybe this one can be skipped as <Global/> renders either null or <style/>

Global seems safe. Fixed class-names and emotion-element.

Andarist · 2021-11-09T17:18:03Z

packages/styled/src/base.js

+          <>
+            {possiblyStyleElement}
+            {ele}
+          </>


@mitchellhamilton should we even render those style elements for compat caches? I mean - right now we need to because extractCritical "analyzes" the rendered HTML. Is this mainly to handle the default cache of the vanilla Emotion?

With @emotion/react (and @emotion/styled) each render gets its unique cache by default (well, each top emotion-aware component gets one by default but let's skip over that detail for now). I wonder if in this context we shouldn't skip rendering those style elements altogether (presumably we'll only be able to do that with the new SSR APIs that we've been talking about)? Basically, when using any of our SSR APIs we can expect a custom cache to be used (when using @emotion/react) and we should be able to get access to that cache and just grab its content (instead of retrieving that from the rendered HTML). Or am I missing something?

I think the goal with the new SSR APIs is to rely roughly on a similar approach as outlined above, right? We can't fully backport this to the old APIs (they will simply get dropped in the next major version anyway) but I wonder if there is any reason why this has not been done closer to this approach from the start.

should we even render those style elements for compat caches?

I'm pretty sure we don't right now. We retrieve the content from the cache, not the html (we look at the html to see what is used because it was originally made from the case where you're just using a single cache for multiple renders, we don't look at the html for the content of the styles though)

Yep - this is kinda what I've meant. That with the new API we wouldn't have to look at the rendered HTML because we could rely on caches being exclusive to renders.

Andarist · 2021-11-09T17:19:42Z

.changeset/strange-kids-change.md

+'@emotion/styled': patch
+---
+
+Fix hydration mismatches if `React.useId` is used


I wonder - would you be open to creating a PR targeting this PR that would add tests for this (this would require bumping React version)? I would love to have a way to verify changes like this but I'm also hesitant to land React 18 on our main branch before it hits its stable mark.

A robust hydration test suite is pretty involved since you really need two separated processes: one for SSR and one for hydrating.

I would love to have a way to verify changes like this but I'm also hesitant to land React 18 on our main branch before it hits its stable mark.

The linked codesandbox in the issue description should be sufficient for manual verification. It has a hydration mismatch with the latest version on npm but none with the codesandbox build from this PR:

Broken with @emotion/styled@11.3.0: https://codesandbox.io/s/sad-breeze-qznnl?file=/src/Sidebar.js
Fixed with this PR: https://codesandbox.io/s/serene-platform-r4eyr?file=/src/Sidebar.js

A robust hydration test suite is pretty involved since you really need two separated processes: one for SSR and one for hydrating.

Ye, I'm not for something like that now. It's possible to just mock whatever isBrowser checks before server and client renders to get a good representation of the real situation. For example this PR has introduced such tests here (IIRC)

Ah, thanks for the pointers. Will look into it.

I would love to have a way to verify changes like this but I'm also hesitant to land React 18 on our main branch before it hits its stable mark.

I can install it behind an alias and let jest use it for specific tests.

Oh, that would be cool - I have never used aliases like this. Hopefully, this won't be too problematic ;p Having that in place would be awesome ❤️

Test illustrating the behavior without this change: 862729d (#2542)

Andarist · 2021-11-09T20:43:48Z

packages/react/__tests__/rehydration.js

+    jest
+      .mock('react', () => {
+        return jest.requireActual('react18')
+      })
+      .mock('react-dom', () => {
+        return jest.requireActual('react18-dom')
+      })
+      .mock('react-dom/server', () => {
+        return jest.requireActual('react18-dom/server')
+      })


…es component

Andarist · 2021-11-13T20:06:48Z

packages/jest/src/create-enzyme-serializer.js


-const enzymeSerializer = createEnzymeToJsonSerializer({})
+const enzymeToJsonSerializer = createEnzymeToJsonSerializer({
+  map: json => {


Note for me, we probably should try to map this in our implementation - just how we do it for CSS prop elements. I've always got it working with this diff:

diff

diff --git a/packages/jest/src/create-enzyme-serializer.js b/packages/jest/src/create-enzyme-serializer.js index 52ec73be..47787dbe 100644 --- a/packages/jest/src/create-enzyme-serializer.js +++ b/packages/jest/src/create-enzyme-serializer.js @@ -9,21 +9,7 @@ import { unwrapFromPotentialFragment } from './utils' -const enzymeToJsonSerializer = createEnzymeToJsonSerializer({ - map: json => { - if (typeof json.node.type === 'string') { - return json - } - const isRealStyled = json.node.type.__emotion_real === json.node.type - if (isRealStyled) { - return { - ...json, - children: json.children.slice(-1) - } - } - return json - } -}) +const enzymeToJsonSerializer = createEnzymeToJsonSerializer() // this is a hack, leveraging the internal/implementation knowledge about the enzyme's ShallowWrapper // there is no sane way to get this information otherwise though diff --git a/packages/jest/src/create-serializer.js b/packages/jest/src/create-serializer.js index 75b984c3..a89b5cce 100644 --- a/packages/jest/src/create-serializer.js +++ b/packages/jest/src/create-serializer.js @@ -13,7 +13,9 @@ import { getKeys, flatMap, isPrimitive, - hasIntersection + hasIntersection, + isStyledElementType, + isStyledComponentType } from './utils' function getNodes(node, nodes = []) { @@ -149,6 +151,12 @@ const createConvertEmotionElements = (keys: string[]) => (node: any) => { type: node.props.__EMOTION_TYPE_PLEASE_DO_NOT_USE__ } } + if (isStyledComponentType(node.node)) { + return { + ...node, + children: node.children.slice(-1) + } + } if (isReactElement(node)) { return copyProps({}, node) } diff --git a/packages/jest/src/utils.js b/packages/jest/src/utils.js index 6eb7dbd1..72f655d1 100644 --- a/packages/jest/src/utils.js +++ b/packages/jest/src/utils.js @@ -98,12 +98,17 @@ export function isEmotionCssPropElementType(val: any): boolean { ) } +export function isStyledComponentType(val: any): boolean { + if (!val) return false + const { type } = val + return type.__emotion_real === type +} + export function isStyledElementType(val: any): boolean { if (val.$$typeof !== Symbol.for('react.element')) { return false } - const { type } = val - return type.__emotion_real === type + return isStyledComponentType(val) } export function isEmotionCssPropEnzymeElement(val: any): boolean {

The only thing missing here is that this should flatten multiple (two?) levels of our "wrappers". After unwrapping the CSS prop element we should also try to unwrap the styled element.

I decided that I don't care about this that much now though, the current solution is working fine so I prefer to land this sooner than later and maybe revisit this in the future.

eps1lon force-pushed the fix/useId-mismatch branch from df83ee6 to 2fb055e Compare November 9, 2021 12:23

eps1lon marked this pull request as ready for review November 9, 2021 12:33

Andarist reviewed Nov 9, 2021

View reviewed changes

eps1lon force-pushed the fix/useId-mismatch branch from b61eb1f to 67350e0 Compare November 9, 2021 18:44

eps1lon added 7 commits November 9, 2021 19:50

Current behavior for useId

862729d

fix(styled): Ensure no hydration mismatch with React.useId

2d79250

yarn changeset

865ff4a

Update snapshots

7589dda

Dirty fix for enzyme matchers

a3fb719

Fragment -> Noop

e98ebd8

Apply same pattern to class-names and emotion-element

aac6191

eps1lon force-pushed the fix/useId-mismatch branch from 67350e0 to aac6191 Compare November 9, 2021 18:50

Andarist reviewed Nov 9, 2021

View reviewed changes

Andarist added 2 commits November 11, 2021 12:18

tweak fragment unwrapping in @emotion/jest

77888db

Fixed enzyme-related serialization tests

8c3d765

Andarist force-pushed the fix/useId-mismatch branch from a174629 to 8c3d765 Compare November 12, 2021 13:47

Andarist added 4 commits November 13, 2021 14:19

Fix more cases and add new tests

d56c781

add tests for avoiding id mismatches when using css prop and ClassNam…

298afaa

…es component

Fix flow errors

60d992e

Reset html before each rehydration test

76ddec5

Andarist reviewed Nov 13, 2021

View reviewed changes

tweak changesets

d386c78

Andarist merged commit eb013d2 into emotion-js:main Nov 14, 2021

github-actions bot mentioned this pull request Nov 14, 2021

Version Packages #2539

Merged

eps1lon mentioned this pull request Nov 15, 2021

Bump Emotion mui/material-ui#29681

Merged

1 task

eps1lon deleted the fix/useId-mismatch branch November 17, 2021 21:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(styled): Ensure no hydration mismatch with React.useId #2542

fix(styled): Ensure no hydration mismatch with React.useId #2542

eps1lon commented Nov 9, 2021 •

edited

changeset-bot bot commented Nov 9, 2021 •

edited

codesandbox-ci bot commented Nov 9, 2021 •

edited

codecov bot commented Nov 9, 2021 •

edited

Andarist commented Nov 9, 2021

Andarist Nov 9, 2021

Andarist Nov 11, 2021

eps1lon Nov 11, 2021

Andarist Nov 12, 2021

Andarist Nov 9, 2021

Andarist Nov 12, 2021

Andarist Nov 9, 2021

eps1lon Nov 9, 2021

Andarist Nov 9, 2021

eps1lon Nov 9, 2021

Andarist Nov 9, 2021

emmatown Nov 9, 2021

Andarist Nov 10, 2021

Andarist Nov 9, 2021

eps1lon Nov 9, 2021

Andarist Nov 9, 2021

eps1lon Nov 9, 2021

Andarist Nov 9, 2021

eps1lon Nov 9, 2021

Andarist Nov 9, 2021

Andarist Nov 13, 2021

fix(styled): Ensure no hydration mismatch with React.useId #2542

fix(styled): Ensure no hydration mismatch with React.useId #2542

Conversation

eps1lon commented Nov 9, 2021 • edited

changeset-bot bot commented Nov 9, 2021 • edited

🦋 Changeset detected

codesandbox-ci bot commented Nov 9, 2021 • edited

codecov bot commented Nov 9, 2021 • edited

Codecov Report

Andarist commented Nov 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon commented Nov 9, 2021 •

edited

changeset-bot bot commented Nov 9, 2021 •

edited

codesandbox-ci bot commented Nov 9, 2021 •

edited

codecov bot commented Nov 9, 2021 •

edited