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.

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

Give context aware message if markup reuse fails #2629

Merged
merged 1 commit into from
Dec 1, 2014

Conversation

arnihermann
Copy link
Contributor

If markup reuse fails, give context for the failure by showing where output from both server and client side markup start to differ:

image

return;
} else {
rootElement.removeAttribute(ReactMarkupChecksum.CHECKSUM_ATTR_NAME);
var rootMarkup = rootElement.outerHTML;
var diffIndex = longestCommonSubstring(markup, rootMarkup);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to blow up. n^2 storage where n is the length of the html. Going to be huge very quickly and you don't really need it. Can you just find the first character that differs instead and display [-20;+20] range from there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 816b011.

@arnihermann arnihermann force-pushed the diff-msg-on-checksum-fail branch from 626e51f to 816b011 Compare November 30, 2014 16:48
*
* @return {int} the index of the character where the strings diverge
*/
function longestCommonSubstring(string1, string2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name doesn't reflect what it does anymore

The context is an extra message with comparison of where server and
client markup started to differ.
@arnihermann arnihermann force-pushed the diff-msg-on-checksum-fail branch from bc494a5 to a352b94 Compare December 1, 2014 10:16
vjeux added a commit that referenced this pull request Dec 1, 2014
Give context aware message if markup reuse fails
@vjeux vjeux merged commit f5a9ea8 into facebook:master Dec 1, 2014
@arnihermann arnihermann deleted the diff-msg-on-checksum-fail branch December 1, 2014 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants