-
Notifications
You must be signed in to change notification settings - Fork 46.3k
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
Hydration warnings with DOM diff in react-reconciler (#10085) #13602
Hydration warnings with DOM diff in react-reconciler (#10085) #13602
Conversation
Details of bundled changes.Comparing: b87aabd...191edbc react-reconciler
schedule
Generated by 🚫 dangerJS |
Hi @gaearon , quick question: do you guys override the
What do you think?
Thanks. |
No.
Sounds good |
0f89fa3
to
81834b6
Compare
@@ -123,20 +123,29 @@ describe('ReactMount', () => { | |||
|
|||
it('should warn if mounting into left padded rendered markup', () => { | |||
const container = document.createElement('container'); | |||
container.innerHTML = ReactDOMServer.renderToString(<div />) + ' '; | |||
container.innerHTML = ' ' + ReactDOMServer.renderToString(<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.
Here and below I fixed the test code to match the test title:
'should warn if mounting into left padded rendered markup'
– the padding (the space) on the left, renderToString output on the right (was the opposite)'should warn if mounting into right padded rendered markup'
– the padding (the space) on the right, renderToString output on the left (was the opposite)
" {' '}\n" + | ||
'- <h1>children <b>text</b></h1>\n' + | ||
'+ <h2>children <b>text</b></h2>\n' + | ||
' <div data-ssr-mismatch-padding-after="1" />\n' + |
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.
Changes to the diff printer compared to PR #12063:
- always prints elements with no children as self-closing, assuming the diff output is renderer-agnostic and is presented in JSX-like syntax, not in the underlying HTML syntax; previously the diff printer had access to
omittedCloseTags
fromreact-dom
, now the diff printer is renderer-agnostic so cannot rely on that. - applies some logic to determine if plain strings should be printed in curly braces and quotes, or as plain text (see
printChildrenValue
andshouldPrintStringAsRawUnescapedText
inReactFiberHydrationWarning.js
) to produce more natural, less cluttered output.
No change:
- the HTML comment nodes and other non-hydratable host nodes are still printed out to add more context to the diff (
react-reconciler
asksreact-dom
to print them via aReactDOMHostConfig
function); this is debatable, you may want to remove them to make the output JSX-only, and simplify some code.
).toWarnDev( | ||
'Warning: Prop `data-ssr-extra-prop` did not match. ' + | ||
'Server: null ' + | ||
'Client: true\n' + |
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.
Change to the warning message compared to PR #12063:
- the values are not converted to strings before being printed with
JSON.stringify
, so they no longer appear in quotes if they are not strings
" for {'SSRMismatchTest default text'} in <div>.\n\n" + | ||
' <div>\n' + | ||
'- <span data-reactroot="">SSRMismatchTest default text</span>\n' + | ||
"+ {'SSRMismatchTest default text'}\n" + |
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 decided to always print the top-level plain text children as strings in curly braces, but this can be changed with a single boolean flip, see printElementOrTextForHydrationDiff
.
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.
Refactored this flag to be an enum value, PRINT_CHILDREN_VALUE_MODE_DEFAULT
or PRINT_CHILDREN_VALUE_MODE_RAW
(see getHydrationDiff
).
@@ -988,7 +940,7 @@ export function diffHydratedProperties( | |||
// Validate that the properties correspond to their expected values. | |||
let serverValue; | |||
const propertyInfo = getPropertyInfo(propKey); | |||
if (suppressHydrationWarning) { | |||
if (rawProps[SUPPRESS_HYDRATION_WARNING] === true) { | |||
// Don't bother comparing. We're ignoring all these warnings. |
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.
We still need to check rawProps[SUPPRESS_HYDRATION_WARNING] === true
here as per the comment, to skip the heavy comparison code below if the warning was suppressed.
didNotMatchHydratedChildrenPropValue( | ||
tag, | ||
rawProps, | ||
(domElement: any), |
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 (domElement: any)
typecasting is ugly, but it was required to convince Flow that Element
is compatible with Instance
when typechecking with renderers other than dom
, such as yarn flow native
, where react-dom
should not be used at all, but alas it's still typechecked.
@@ -1239,3 +1138,5 @@ export function restoreControlledState( | |||
return; | |||
} | |||
} | |||
|
|||
export {normalizeHTMLTextOrAttributeValue}; |
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 normalizeHTMLTextOrAttributeValue
function is used in ReactDOMHostConfig
, so exporting it here. It's not a function
declaration because the function implementation gets substituted in __DEV__
.
@@ -47,7 +44,8 @@ export type Props = { | |||
export type Container = Element | Document; | |||
export type Instance = Element; | |||
export type TextInstance = Text; | |||
export type HydratableInstance = Element | Text; | |||
export type HydratableInstance = Instance | TextInstance; | |||
export type HostInstance = HydratableInstance | Container | Node; |
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 new type HostInstance
means all possible host node types (like comment nodes and other non-hydratable nodes) that the hydration process can potentially meet during its DOM traversal, not just Element
(Instance
) and Text
(TextInstance
).
❓I still haven't figured out how to make yarn flow custom
typecheck succeed. The assumption is that HostInstance
is at least a union of HydratableInstance | Container
, but there is no strict guarantee that it is defined like that. Please advice, this is the only remaining thing that fails in CI now. ❓
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 new type
HostInstance
means all possible host node types (like comment nodes and other non-hydratable nodes) that the hydration process can potentially meet during its DOM traversal, not justElement
(Instance
) andText
(TextInstance
).
❓I still haven't figured out how to makeyarn flow custom
typecheck succeed. The assumption is thatHostInstance
is at least a union ofHydratableInstance | Container
, but there is no strict guarantee that it is defined like that. Please advice, this is the only remaining thing that fails in CI now. ❓
I decided to simplify the code and remove printing of non-hydratable nodes. This refactor resulted in removing HostInstance
type as unnecessary, and removing HydratableInstance
type (inlining the alias). See 3ad9c5f description for details.
@gaearon Please take a look. |
@gaearon I resolved the outstanding issues by removing the requirement to print in the diff the non-hydratable nodes like HTML comments and other weird nodes that should not affect the hydration. This resulted in simpler |
'Server: "server text" ' + | ||
'Client: "client text"', | ||
// Without the component stack here because it's empty: rendering a text node directly into the root node. | ||
{withoutStack: true}, |
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.
Without the component stack here because it's empty: rendering a text node directly into the root node
A thought came to me here: why doesn't React show in the stack the place where the render
or hydrate
was called? This would be 1) consistent, there'd always be a stack; 2) useful for apps with multiple render entry points.
High-level overview (asked by @gaearon via Twitter DM). BEFOREAFTER |
9c8093f
to
cb284f6
Compare
👆Rebased onto current |
@gaearon @sompylasar Is this ready to merge? This would be a huge boon in debugging SSR errors! |
How is this approach affected by Partial Hydration? #14717 and #14884 It seems like this approach just logs the deltas as they are encountered (as opposed to putting together a log of all the differences at the etc) so maybe that's fine? It would probably need to consider that the HTML might not be the final HTML yet. |
@sebmarkbage Thanks for looking into this and linking to your Partial Hydration work! I noticed some of it before and how it slightly conflicts with my changes related to Flow typings, but I'll need to get a closer look and get some integrated tests together with Partial Hydration to answer your question. This approach builds a diff between the Fiber tree (leaf host nodes) and the DOM tree synchronously at the time the hydration mismatch is encountered. In my current understanding, with Partial Hydration some Fibers won't have leaf host nodes where they might be present in the DOM, so the sequential diff may even not be possible to produce as the sizes of the holes under suspended hydration that we'd need to skip in the actual DOM would be unknown until it's rendered. I'll try to find some time within the next few weeks after I'm back from 🏖, but no promises as I'll be busy with learning new things on the new job (haven't yet announced publicly 🙊). |
Is there any way I can test this PR on our production codebase? |
You can build it and try it? 🙂 Not sure it there's anything specific that you are asking. |
cb284f6
to
c4e285e
Compare
👆Rebased onto current |
c4e285e
to
3ac81d9
Compare
Cannot run |
57b45af
to
8f5cefd
Compare
👆Rebased onto current |
New functions in ReactFiberHostConfig: ``` getHostInstanceDisplayName, getHostInstanceProps, isHydratableInstance, isTextInstance, getTextInstanceText, compareTextForHydrationWarning, comparePropValueForHydrationWarning, ``` Moved from ReactDOMComponent to ReactFiberHydrationWarning: ``` warnForUnmatchedText, // removed warnForDeletedHydratableElement, // renamed to warnForDeletedHydratableInstance warnForDeletedHydratableText, // removed warnForInsertedHydratedElement, // renamed to warnForInsertedHydratedInstance warnForInsertedHydratedText, // renamed to warnForInsertedHydratedTextInstance warnForTextDifference, warnForPropDifference, warnForExtraAttributes, SUPPRESS_HYDRATION_WARNING, didWarnInvalidHydration, ``` The warnFor... functions are no longer exported and are an implementation detail of ReactFiberHydrationWarning. Renamed in ReactDOMComponent, ReactDOMHostConfig: ``` normalizeMarkupForTextOrAttribute -> normalizeHTMLTextOrAttributeValue normalizeHTML -> normalizeHTMLMarkup ``` Moved from ReactFiberHostConfig to ReactFiberHydrationWarning: ``` didNotMatchHydratedContainerTextInstance, didNotMatchHydratedTextInstance, didNotHydrateContainerInstance, didNotHydrateInstance, didNotFindHydratableContainerInstance, didNotFindHydratableContainerTextInstance, didNotFindHydratableContainerSuspenseInstance, didNotFindHydratableInstance, didNotFindHydratableTextInstance, didNotFindHydratableSuspenseInstance, ``` New functions exported from ReactFiberHydrationWarning: ``` findHydrationWarningHostInstanceIndex, didNotMatchHydratedChildrenPropValue, didNotMatchHydratedPropValue, didNotMatchHydratedPropsHostInstanceHasExtraAttributes, ```
8f5cefd
to
504a451
Compare
The screenshot above seems very verbose. Seems like we should somehow clip the length in the middle? I'm not sure all this info is valuable. |
It says "did not expect I agree that it's too much and can be clipped. As you may notice, the attribute values are clipped, but not elements themselves. Usually the difference is not so dramatic. |
Also the first line phrasing "did not expect" was kept from the original implementation. We can think of a better phrasing to make it clearer. In the code there's also a TODO to think about phrasing in terms of universal hydration, not just server/client. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
Hey stale[bot], I still have hopes to ship this some day! |
Thanks a lot for exploring this. The scope ended up being larger than we expected, and we went with a simpler (although perhaps less precise) solution: #10085 (comment). But this was still a very useful exploration. |
We're adding diffs in #28512. |
An attempt to implement the hydration warning and DOM diff in react-reconciler package as requested by @gaearon here: #12063 (review)
The tests and fixtures were copied from the original PR: #12063 and slightly modified (see my review below).