Render <noscript> contents to string

[matthewwithanm]

This is the fix for #1252; it uses renderComponentToStaticMarkup on the children so that the server and browser both treat them the same (as text content). In browsers that have JS disabled…well, React won't be loaded so it can't be upset that the contents are actual elements 😝

Let me know if any improvements can be made. I didn't see a map utility for the children list that produced an array so I used ReactChildren.forEach. I think the tests cover everything, but if not, let me know!

[sophiebits]

Can you make sure that this passes in all browsers? You can run grunt test --debug then go to http://127.0.0.1:9999/test/ to run the tests. (Because of #1314 some of the Immutable tests may be failing already, but you can ignore those.)

[sophiebits]

(If it isn't consistent, I'm not super concerned and I think we could probably drop this test.)

[matthewwithanm]

Hm…it seems like <noscript> is a different beast in IE8. For example, trying to set its innerHTML just causes an error. This is a problem: if you write a component that uses <noscript> (in order to leverage server side rendering), it will error when React tries to mount it.

I'm not exactly sure how best to get around this. I supposed we could sidestep the issue by telling it not to render any contents when mounting into the DOM, but I'm not sure how best to recognize that. Thoughts?

[matthewwithanm]

Wait, what am I saying? Then it wouldn't match what the server sent down!

[sophiebits]

You mean modifying an existing noscript gives problems in IE8? If so, you could cache the rendered HTML in componentWillMount, store it in state, and just use that on subsequent renders.

[matthewwithanm]

No, unfortunately. It doesn't seem like the problem is just modifying a noscript but even creating one in the first place. It doesn't seem to be possible to create a noscript element with contents in that browser.

var div = document.createElement('div');
div.innerHTML = '<noscript>hi</noscript>';
console.log(div.innerHTML); // undefined

Also, modification via appendChild

var noscript = document.createElement('noscript');
var span = document.createElement('span');
span.innerHTML = 'hi';
noscript.appendChild(span);
// Error: "Unexpected call to method or property access."

[sophiebits]

I think this looks good, though I'd probably call it ReactDOMNoScript.

[matthewwithanm]

Ah, okay. I took a guess based on the capitalization of "Textarea" but I can fix it.

[sophiebits]

Yeah... maybe you could argue that textarea is one word though. I don't feel strongly.

[matthewwithanm]

I pushed the capitalization change. It does seem a little inconsistent IMO, but I figure this way you guys can just decide whether you want that particular commit or not (:

[matthewwithanm]

To summarize what I told @spicyj on IRC:

One test is currently failing in IE8. It's not a problem with the PR per se, but with how IE8 handles <noscript>s—namely, it doesn't allow you to create them with contents or add contents to an existing element. Therefore, the failing test is really just showing something that doesn't have coverage in React's current suite.

As far as workarounds go, I can only think of one approach that would make IE8 happy: 1) always render <noscript> without content in the browser and 2) exclude them from reconciliation/invariant checking. (This would only work because rendering <noscript> contents on the browser side is pointless anyway.)

EDIT: The bug now has its own issue #1341

[sophiebits]

Going to close this out for now because there's not much improvement we can make here immediately. Thanks for sending this in though! I think we may be able to make this really work in the future after some upcoming refactors of the core.

[matthewwithanm]

That's too bad /:

The current state is broken. This is at least unbroken in everything but IE8.

[sophiebits]

As a workaround, you can write

<div dangerouslySetInnerHTML={{__html: '<noscript>...</noscript>'}} />

and React shouldn't get confused even if the noscript disappears.

[andygeers]

I'd love it if somebody could revisit this issue - it's a real pain to have to use the workaround