Fix false positive <noscript> rehydration text difference warning in React 16 #11157
Conversation
|
cc @syranide as the resident DOM quirk expert |
|
@sebmarkbage This one's for you |
|
|
||
| if (parent.tagName === 'NOSCRIPT') { | ||
| // <noscript> content is parsed as text, but only if the browser parses it | ||
| // together with <noscript> tag itself. So we have to wrap it once more. |
There was a problem hiding this comment.
a = document.createElement('noscript');
a.innerHTML = '<b>foobar</b>';
console.log(a.childNodes);Outputs a single TextNode for me in both Chrome and Edge (it does not parse the b-tag), which seems to contradict the comment and the whole reason for this existing... right? Is there more to it than that?
There was a problem hiding this comment.
Interesting. What you're saying is right for document.createElement(), but not for document.implementation.createHTMLDocument('').createElement() which is what we're using. I'm not sure why there is a difference (is it because it doesn't have a body or something like this?)
There was a problem hiding this comment.
There doesn't seem to be a difference between the wrapper and not in this test:
http://jsbin.com/mofexaxaya/2/edit?js,console
What is different?
|
I'd like to understand where this difference is coming from and whether it actually only affects |
|
The only reason we do this in a new document is to avoid custom elements being created for registered tags and subsequently having side-effects. I don't know if this easy work around is enough but if it's not, we could also say that such custom elements are broken and just use the same document. |
| // together with <noscript> tag itself. So we have to wrap it once more. | ||
| var wrapperElement = document.createElement('div'); | ||
| wrapperElement.innerHTML = '<noscript>' + html + '</noscript>'; | ||
| var noscriptElement = wrapperElement.firstElementChild; |
There was a problem hiding this comment.
Why not just firstChild (since it goes back longer in browser support and seems sufficient)?
There was a problem hiding this comment.
It seems to me that the reason this happens is because the detached document doesn't have "scripting enabled" because it doesn't have a browsing context.
I don't see why wrapping it matters but that doesn't matter because it breaks in another case.
<div
dangerouslySetInnerHTML={{
__html: `<noscript><b>Enable JavaScript to run this app.</b></noscript>`,
}}
/>If the <noscript /> tag is nested deeply in any innerHTML you have the same problem.
I don't see a way to special case this nor a way to enable scripting on custom documents to force this parsing. Maybe we should just switch to using the normal document and bite the bullet that custom elements might mess with us?
This makes sense. I saw it in the spec but the meaning of "browsing context" eluded me.
This sounds reasonable to me. I'll do tomorrow unless you get there first. |
|
I'm curious, would it be possible to instead just stop generating (EDIT: I don't really have a good understanding of how this all fits into the bigger picture atm, so perhaps it doesn't make sense) |
This wouldn’t help with the case in #11157 (review), would it? |
This reverts commit 27a4250.
|
In that case React trips not because it rehydrates I’m not sure how React could skip over |
|
@gaearon Ah my bad, I wasn't aware that v16 tests Anyway, I don't have any opinion on this and you have a better understanding of the big picture. |
It’s still expected to be the same on the first render, is it not? Since in 15 the markup validation would fail otherwise. The only reason we have to “re-parse” the HTML is because we don’t do markup generation step on the client anymore. |
|
OK, I reverted the old fix and pushed the one that just uses |
Usually yes (components renders html, then modifies it via script), but one could imagine that someone doesn't want to wait for React to load+hydrate and makes e.g. some progressive enhancement scripts load before React so that certain parts of the page are immediately interactive. I don't dabble with SSR myself (and would never to that extent), so I'm indifferent towards it, but I would have assumed that to be valid given that I'm free to modify it. However, it's only a warning so I guess it's technically allowed and it's understandable if React draws the line there.
It's not nice, but I'm assuming a possibility would be to just skip the warning if one detects a noscript-tag (e.g. in the DOM or in the html string), "good enough". Similar to how React now normalizes the strings before warning when hydrating. An alternative I guess would be to switch it around, instead of comparing the strings you can walk the DOM and compare node by node. It's a lot more work, but it would give you the possibility to bail at noscript or even normalize nodeValue vs innerHTML. Perhaps a middle-ground could be to just generate the "comparison DOM" as you do, but step through the hierarchy and normalize all PS. I dug around the code a bit and understand the context a bit better now, so I see where I didn't make sense before, sorry 😄 |
|
Sorry, I meant the other way around, if you encounter a noscript-tag, serialize its nodes into a TextNode. |
|
My impression is that the use case of custom elements with constructor side effects inside |
There was a problem hiding this comment.
I think the most convincing argument around this is that third-party scripts in the innerHTML can do whatever they want to the DOM before this hydrates and therefore we should ignore the warning all together.
If that is needed, there is now an easy bailout mechanism with suppressHydrationWarning. That also solves any problems with custom elements, if there are any, since it'll skip this code path.
Seems better to try the more restrictive solution with an opt-out and if it's a problem we can relax it.
|
The custom element problem is something that might happen but we don't know, where as noscript will definitely happen. |
|
I had the same issue with |
|
Can you check whether it does by applying the same modification locally? |
|
@gaearon server-side rendering code is correct. It is just a false positive warning. |
|
Yes, but do you still have the false positive warning after applying this fix? |
|
I didn't try with the fix, I don't know how to apply it. I have never built React 😅. |
|
I mean you can just apply it manually in the build output in |
|
Oops, in this case it would be |
|
I applied the patch and it doesn't work. I investigated deeply and it result it was a script (Google Analytics) inserted client side just before ReactDOM.hydrate, React found the script and confused it with another. I fixed it by inserting the script after rendering. The warning could be improved, the only information I got was the prop "type" that didn't match. I think you should mention the |
|
There is a separate issue tracking making these warnings better. The plan is to batch them and show a diff view. |
|
OK nice! |
Fixes #10993.
It doesn’t seem like jsdom reproduces the original issue (?!) so instead I added a
<noscript>into our SSR fixture. I verified the warning appears without the fix, and disappears after the fix.The
<noscript>tag is special because the browser parses it as text content when scripting is enabled. However, the way we normalized HTML did not reproduce the same behavior faithfully because we didn’t include the<noscript>tag itself into that HTML. Therefore, it didn’t switch the parsing mode to treat its content as text.The fix adds a special case for
<noscript>to wrap it an extra time. I decided to special-case it because it seemed more solid than always wrapping (which is not always possible to do one level, e.g.tr > tdcase).