-
Notifications
You must be signed in to change notification settings - Fork 47.4k
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
Print attributes in hydration errors #24167
Conversation
if (value.length > 30) { | ||
trimmedValue = value.substr(0, 30) + '...'; | ||
} | ||
str += ' ' + attributeName + '="' + trimmedValue + '"'; |
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.
This doesn't escape trimmedValue but I think that's fine. If we wanted to escape we'd need to move the escaping utility from the server folder or copypaste here.
str += ' ...'; | ||
break; | ||
} | ||
const attributeName = attributeNames[i]; |
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 could get a React name for this but seems confusing. E.g. it would still print style
as a string. So I figured it should just use HTML in the message.
Comparing: 3787230...c5232b2 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
@@ -1244,9 +1270,9 @@ export function warnForInsertedHydratedElement( | |||
} | |||
didWarnInvalidHydration = true; | |||
console.error( | |||
'Expected server HTML to contain a matching <%s> in <%s>.', | |||
'Expected server HTML to contain a matching <%s> in %s.', |
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.
Is it clear what the "in" really means here? That's the thing that I always found vague. Like are these the mismatches, or is one parent and one child?
I feel like the message you use some formatting.
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.
Yeah, I'd propose changing it more significantly. Something like
There is a mismatch between the client and server render output.
The mismatch occurred inside of this parent element:
<div class="blabla parent">
The last successfully matched child was:
<h1 class="blabla">
After it, the server rendered:
<h3 class="blabla">
But the client rendered:
<h2 class="blabla">
Fix the error by making the client and the server output the same.
Thoughts?
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.
This has the same data but is easier to parse for me at least. More visual:
There is a mismatch between the client and server render output.
The mismatch occurred inside of this element:
<div class="blabla parent">
<h1 class="blabla">...</h1>
- <h3 class="blabla"> <--- server
+ <h2 class="blabla"> <--- client
...
</div>
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 format translates to missing and extra nodes too.
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.
Aside from stylistic nits/bikeshedding the overall approach is good with me
The mildest version I can think of that still adds something useful to the messages. Doesn't require any extra wiring but should help when you have class names or styles. This primarily annotates the parent so it's not as helpful as it would be if we said which child didn't match. But I think it's better than status quo.