Skip to content

Improve warning in react mount #4324

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

Merged
merged 5 commits into from
Jul 10, 2015

Conversation

drd
Copy link

@drd drd commented Jul 9, 2015

While I was debugging a problem reusing server-rendered markup, I was thrown off the track for a while because React was telling me there were differences in various entity encodings and attribute representations between the server- and client-rendered markup. For instance, the server markup would contain   while the client would contain a literal \u00a0 character. Also, empty attributes on the client would be rendered as attribute whereas the server markup would show attribute="". After finally realizing that the real problem was in fact a different string being rendered on server vs client, I came up with this method to present more useful information when this situation arises.

@drd drd force-pushed the improve-warning-in-react-mount branch from c567947 to 902b607 Compare July 9, 2015 21:06
@jimfb
Copy link
Contributor

jimfb commented Jul 9, 2015

Your unit test passes even without your changes to ReactMount :/.

@drd
Copy link
Author

drd commented Jul 9, 2015

Man. I definitely checked that at one point ;) Let me see what happened.

@drd
Copy link
Author

drd commented Jul 9, 2015

There we go.   is treated differently than other entities.

normalizedMarkup = normalizer.innerHTML;
} else {
normalizer = document.createElement('iframe');
document.body.appendChild(normalizer);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should not be appending a child to the body here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer not to, but unfortunately iframes only get a valid contentDocument when in the DOM. I've addressed this by immediately removing it from the DOM.

var difference = ' (client) ' +
markup.substring(diffIndex - 20, diffIndex + 20) +
normalizedMarkup.substring(diffIndex - 20, diffIndex + 20) +
'\n (server) ' + rootMarkup.substring(diffIndex - 20, diffIndex + 20);

invariant(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invariant shouldn't be in devmode block. @zpao should this even be an invariant? Seems like this could just be a warning?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the devmode block ends on line 901.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't in a dev block and yes it should be an invariant. The message makes it pretty clear that React probably breaks when rendering to the document element with content that doesn't checksum.

We have a dev block lower for the warning. We could perhaps have a more concise difference there.

@jimfb
Copy link
Contributor

jimfb commented Jul 10, 2015

Ok, all looks reasonable to me. Thanks @drd!

jimfb added a commit that referenced this pull request Jul 10, 2015
@jimfb jimfb merged commit 52ad6bc into facebook:master Jul 10, 2015
@leolannenmaki
Copy link

Is there any chance that this fix could be applied to the 0.13.x branch as well? I'm currently running a custom version of React which is just 0.13.3 with this pull request applied on top.

@jimfb
Copy link
Contributor

jimfb commented Jul 29, 2015

@leolannenmaki We don't generally backport anything except critical fixes. React 0.14 should be out soon :). In the mean time, you can develop/test against master, or use one of the prebuilt packages mentioned in #4311, or do what you're already doing now.

@leolannenmaki
Copy link

@jimfb ok, thanks. Looking forward to 0.14 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants