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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 14 commits into from Nov 14, 2021
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
862729d
Current behavior for useId
eps1lon Nov 9, 2021
2d79250
fix(styled): Ensure no hydration mismatch with React.useId
eps1lon Nov 9, 2021
865ff4a
yarn changeset
eps1lon Nov 9, 2021
7589dda
Update snapshots
eps1lon Nov 9, 2021
a3fb719
Dirty fix for enzyme matchers
eps1lon Nov 9, 2021
e98ebd8
Fragment -> Noop
eps1lon Nov 9, 2021
aac6191
Apply same pattern to class-names and emotion-element
eps1lon Nov 9, 2021
77888db
tweak fragment unwrapping in `@emotion/jest`
Andarist Nov 11, 2021
8c3d765
Fixed enzyme-related serialization tests
Andarist Nov 12, 2021
d56c781
Fix more cases and add new tests
Andarist Nov 13, 2021
298afaa
add tests for avoiding id mismatches when using css prop and ClassNam…
Andarist Nov 13, 2021
60d992e
Fix flow errors
Andarist Nov 13, 2021
76ddec5
Reset html before each rehydration test
Andarist Nov 13, 2021
d386c78
tweak changesets
Andarist Nov 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/strange-kids-change.md
View file
Open in desktop
@@ -0,0 +1,5 @@
---
'@emotion/styled': patch
---

Fix hydration mismatches if `React.useId` is used
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -261,6 +261,8 @@
"react-router-dom": "^4.2.2",
"react-scripts": "1.1.5",
"react-test-renderer": "16.8.6",
"react18": "npm:react@alpha",
"react18-dom": "npm:react-dom@alpha",
"svg-tag-names": "^1.1.1",
"through": "^2.3.8",
"unified": "^6.1.6",
Expand Down
7 changes: 6 additions & 1 deletion packages/jest/src/utils.js
View file
Open in desktop
Expand Up @@ -58,7 +58,12 @@ function getClassNameProp(node) {
return (node && node.prop('className')) || ''
}

function getClassNamesFromEnzyme(selectors, node) {
function getClassNamesFromEnzyme(selectors, nodeWithFragment) {
// 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().at(1)
: nodeWithFragment
// We need to dive in to get the className if we have a styled element from a shallow render
const isShallow = shouldDive(node)
const nodeWithClassName = findNodeWithClassName(
Expand Down