Ignore CR/LF differences when warning about markup mismatch - #11119
Conversation
|
Deploy preview ready! Built with commit 72e67ff |
|
I haven’t thought about attributes though. I guess they also need it. |
sebmarkbage
left a comment
sebmarkbage
left a comment
There was a problem hiding this comment.
If all you want to do is ignore the warning your logic should be able to be self-contained completely in warnForTextDifference and fully behind a __DEV__ flag so we don't need to ship this logic nor use it in production.
I.e. we shouldn't need to change the behavior of the isDifferent flag if we don't expect to patch it up.
| if (normalizedClientText === serverText) { | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
This return value is meant to indicate that it should patch it up. So this says that we will patch up the DOM if it differs, which is not what your comment says.
gaearon
changed the title
It's always going to "pass" because we normalize HTML anyway. Except that it won't pass because we intentionally don't patch up dangerouslyInnerHTML.
aickin
left a comment
aickin
left a comment
There was a problem hiding this comment.
Looks great. I love the test cases especially!
| var normalizeMarkupForTextOrAttribute = function(markup: mixed): string { | ||
| const markupString = typeof markup === 'string' | ||
| ? markup | ||
| : '' + (markup: any); |
There was a problem hiding this comment.
Not super proud but I couldn't get Flow to understand I'm just trying to cast to string.
|
They took some head scratching 😛 |
|
Nvm, failed my reading comprehension of the source code. |
|
I believe the PR normalises both now. |
) * Add regression test for CR-insensitive hydration * Normalize CR when comparing server HTML to DOM * Move tests in the file * Add a failing test for comparing attributes * Normalize CR for attributes too * Add a test case for CR in dangerouslySetInnerHTML * Undo the fix per feedback * Change the fix to be DEV-only and still patch up LF -> CR * Remove the dangerouslySetInnerHTML test It's always going to "pass" because we normalize HTML anyway. Except that it won't pass because we intentionally don't patch up dangerouslyInnerHTML. * Fix issue that Flow failed to catch * Add null character tests * Normalize both client and server value for the warning * Fix the bug * Normalize replacement character away as well * Fix outdated comment
8 participants
Fixes #11103.
According to the spec, HTML parser turns
\r\nor lone\rinto\n.Since we compare JS app string (which might have
\r) with the representation parsed from HTML (which can't), we have a false positive text mismatch warning.I am solving this by adding a second check. It's only activated when we have a mismatch, so it only penalizes users with mismatching markup, or users with
\rin content.I opted not to patch up the markup when the only difference is\rnormalization. This is because we never did that before, and nobody complained. So it's probably unobservable to the user anyway (even though it's observable from the DOM—you can't have\rby settinginnerHTMLor parsing, but you can if youcreateTextNodes with it directly).An alternative could be to always normalize\rwhen creating text nodes (and patch up markup if we encounter it). This would also make the behavior consistent betweeninnerHTMLandcreateTextNodepath on the client side.It seems like we can always do the latter if we want to, but the former is an unobtrusive fix that can be applied now.Updated it to just ignore the warning, but still patch up. Production logic doesn't change now.
Later updated to normalize both server and client values before emitting the warning. I started stripping both null and "replacement" character (browser sometimes turns null into it) since that's the only other corner case I know.