-
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
fix: skip dangerouslySetInnerHtml hydration warning if it's undefined #18676
fix: skip dangerouslySetInnerHtml hydration warning if it's undefined #18676
Conversation
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 b72da4e:
|
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 5c05857:
|
packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js
Outdated
Show resolved
Hide resolved
); | ||
if (expectedHTML !== serverHTML) { | ||
warnForPropDifference(propKey, serverHTML, expectedHTML); | ||
if (nextHtml != null) { |
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.
So.. what if nextHtml
is null
or undefined
on the client, but it was not null
on the server? Wouldn't this miss a warning?
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 had the exact same question. This is pretty similar to the second test I added (https://github.com/facebook/react/pull/18676/files#diff-ab371863932cd2e8f0ba14ff2eaab380R568). I can change it not render anything on the client or add a new one that goes from { __html: "server" }
to undefined
.
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.
Adding new one sounds good.
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'm just asking how it works.
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 didn't read much of the surrounding code and relied on the tests doing what I expected so I can't tell you how it works.
Maybe I should convert it to a table and it.each
so that it's more visible how the props change between client and server? The tests are mostly setup.
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 prefer explicitly written tests to it.each
. Don't worry about duplication.
I didn't read much of the surrounding code and relied on the tests doing what I expected so I can't tell you how it works.
I think we need to figure out how it works before we can be comfortable 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.
I think we need to figure out how it works before we can be comfortable merging this. :-)
Sure thing. Let me trace this for each test case. Maybe this clears this up.
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.
When tracing the code I wasn't sure if this might result in a loss of component stacks so I added these to the test to make sure.
It looks like the reconciler is going through all the instances that aren't hydrated and deletes them:
deleteHydratableInstance(fiber, nextInstance); |
The host config is then responsible for warning about the deletion of hydrateable text/elements:
export function didNotHydrateInstance( |
So it's no longer warning about a mismatch in dangerouslySetInnerHTML
but rather that the resulting html mismatches.
It seems like both approaches do not create a helpful error message. In the version on master
it displays the wrong content of dangerouslySetInnerHTML
thinking that it's <p>server</p>
while this content came from the children
prop. In the version on this PR we no longer see that dangerouslySetInnerHTML
was part of the issue.
Could you shoot me a Twitter DM or email? I'm not sure what's broken, but want to look into it! |
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.
makes sense
Summary
Closes #17170
Test Plan
Added tests to
ReactServerRenderingHydration-test.js
that make sure thatdangerouslySetInnerHTML
isundefined
and the markup matchesdangerouslySetInnerHTML
isundefined
and the markup does not match.Verify fix with codesandboxes in #17170:
react-dom/server
/cc @CompuIvesreact-dom/server
: https://codesandbox.io/s/dangerouslysetinnerhtml-undefined-e14qg?file=/src/index.js logs no more warnings as expected.