-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix(styled): Ensure no hydration mismatch with React.useId #2542
Conversation
🦋 Changeset detectedLatest commit: d386c78 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
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:
|
Codecov Report
|
df83ee6
to
2fb055e
Compare
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 👍 |
packages/jest/src/utils.js
Outdated
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look into this issue and resolve this TODO comment before merging this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
> | ||
Hello | ||
</div> | ||
<Fragment> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
packages/styled/src/base.js
Outdated
@@ -132,27 +132,31 @@ let createStyled: CreateStyled = (tag: any, options?: StyledOptions) => { | |||
newProps.ref = ref | |||
|
|||
const ele = React.createElement(finalTag, newProps) | |||
let possiblyStyleElement = <></> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
) | ||
} | ||
return ele | ||
// Need to return the same number of siblings or else `React.useId` will cause hydration mismatches. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <Global/>
renders eithernull
or<style/>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Global
seems safe. Fixed class-names and emotion-element.
<> | ||
{possiblyStyleElement} | ||
{ele} | ||
</> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
.changeset/strange-kids-change.md
Outdated
'@emotion/styled': patch | ||
--- | ||
|
||
Fix hydration mismatches if `React.useId` is used |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/[email protected]
: 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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ❤️
There was a problem hiding this comment.
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)
b61eb1f
to
67350e0
Compare
67350e0
to
aac6191
Compare
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') | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat
a174629
to
8c3d765
Compare
|
||
const enzymeSerializer = createEnzymeToJsonSerializer({}) | ||
const enzymeToJsonSerializer = createEnzymeToJsonSerializer({ | ||
map: json => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
What:
Fixes reactwg/react-18#111 (reply in thread)
Why:
If
React.useId
is used then emotion currently causes hydration mismatchesHow:
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 documentationBroken with
@emotion/[email protected]
: https://codesandbox.io/s/sad-breeze-qznnl?file=/src/Sidebar.jsFixed with this PR: https://codesandbox.io/s/serene-platform-r4eyr?file=/src/Sidebar.js