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

fix: skip dangerouslySetInnerHtml hydration warning if it's undefined #18676

Merged
merged 6 commits into from
Apr 20, 2020
Merged

fix: skip dangerouslySetInnerHtml hydration warning if it's undefined #18676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b14d38a
test: Add failing case for dangerouslySetInnerHtml=undefined
eps1lon Apr 20, 2020
47c0e9e
fix: skip dangerouslySetInnerHtml warning if it's undefined
eps1lon Apr 20, 2020
b72da4e
test: add similar test that should trigger the warning
eps1lon Apr 20, 2020
5badbd9
chore: Remove redundant nullish check
eps1lon Apr 20, 2020
63387c2
Poke yarn_test_www_variant which timed out
eps1lon Apr 20, 2020
5c05857
test: Add smaller test for innerHTML=string to innerHTML=undefined
eps1lon Apr 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,4 +548,57 @@ describe('ReactDOMServerHydration', () => {
expect(ref.current).toBe(div);
expect(element.innerHTML).toBe('<div>Hello World</div>');
});

// regression test for https://github.com/facebook/react/issues/17170
it('should not warn if dangerouslySetInnerHtml=undefined', () => {
const domElement = document.createElement('div');
const reactElement = (
<div dangerouslySetInnerHTML={undefined}>
<p>Hello, World!</p>
</div>
);
const markup = ReactDOMServer.renderToStaticMarkup(reactElement);
domElement.innerHTML = markup;

ReactDOM.hydrate(reactElement, domElement);

expect(domElement.innerHTML).toEqual(markup);
});

it('should warn if innerHTML mismatches with dangerouslySetInnerHTML=undefined and children on the client', () => {
const domElement = document.createElement('div');
const markup = ReactDOMServer.renderToStaticMarkup(
<div dangerouslySetInnerHTML={{__html: '<p>server</p>'}} />,
);
domElement.innerHTML = markup;

expect(() => {
ReactDOM.hydrate(
<div dangerouslySetInnerHTML={undefined}>
<p>client</p>
</div>,
domElement,
);

expect(domElement.innerHTML).not.toEqual(markup);
}).toErrorDev(
'Warning: Text content did not match. Server: "server" Client: "client"',
);
});

it('should warn if innerHTML mismatches with dangerouslySetInnerHTML=undefined on the client', () => {
const domElement = document.createElement('div');
const markup = ReactDOMServer.renderToStaticMarkup(
<div dangerouslySetInnerHTML={{__html: '<p>server</p>'}} />,
);
domElement.innerHTML = markup;

expect(() => {
ReactDOM.hydrate(<div dangerouslySetInnerHTML={undefined} />, domElement);

expect(domElement.innerHTML).not.toEqual(markup);
}).toErrorDev(
'Warning: Did not expect server HTML to contain a <p> in <div>',
);
});
});
11 changes: 5 additions & 6 deletions packages/react-dom/src/client/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1110,12 +1110,11 @@ export function diffHydratedProperties(
} else if (propKey === DANGEROUSLY_SET_INNER_HTML) {
const serverHTML = domElement.innerHTML;
const nextHtml = nextProp ? nextProp[HTML] : undefined;
const expectedHTML = normalizeHTML(
domElement,
nextHtml != null ? nextHtml : '',
);
if (expectedHTML !== serverHTML) {
warnForPropDifference(propKey, serverHTML, expectedHTML);
if (nextHtml != null) {
Copy link
Collaborator

@gaearon gaearon Apr 20, 2020 •
edited
Loading

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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. :-)

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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:

react/packages/react-reconciler/src/ReactFiberHydrationContext.old.js

Line 462 in dc630d3

deleteHydratableInstance(fiber, nextInstance);

The host config is then responsible for warning about the deletion of hydrateable text/elements:

react/packages/react-dom/src/client/ReactDOMHostConfig.js

Line 957 in 58c895e

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.

const expectedHTML = normalizeHTML(domElement, nextHtml);
if (expectedHTML !== serverHTML) {
warnForPropDifference(propKey, serverHTML, expectedHTML);
}
}
} else if (propKey === STYLE) {
// $FlowFixMe - Should be inferred as not undefined.
Expand Down