Nested <noscript> renders invalid HTML

[echenley]

While it doesn't really make sense to nest <noscript />, there are instances where it can happen unintentionally: mui/material-ui#3586

In this case, React renders invalid HTML: http://codepen.io/anon/pen/BKjVpZ?editors=1010

In a setup with HMR, this ends up causing invariant violations. It might be worth adding a warning for this case.

React Version: 0.14.7 (should be fixed in 15.0 due to the changes to <noscript />)

[gaearon]

Thank you for reporting! This appears to be fixed in 15.0: http://codepen.io/anon/pen/ONMeor?editors=1010.

To me, this does not look like a high-priority issue so I would suggest to work around it by using an empty <div> until you can switch to 15. I will leave it open for other team members to comment on.

[jimfb]

Confirmed, fixed: http://jsfiddle.net/smw2cLe1/

[wchargin]

This may be a dumb question, but…what does "fixed" mean? i.e., I'm of the opinion that the expected output for the static markup for the component

const c = (
    <noscript>
        <span />
        <noscript>
            <div />
        </noscript>
        <p>sneaky</p>
    </noscript>
);

should be

<noscript>
    <span></span>
    <div></div>
    <p>sneaky</p>
</noscript>

with all nested noscripts squashed out, and that in particular, the rendering

<noscript>
    <span></span>
    <noscript>
        <div></div>
    </noscript>
    <p>sneaky</p>
</noscript>

is incorrect because the first (indented) </noscript> ends the whole thing and the sneaky text appears.

The argument for this is that it enables local reasoning about code. Otherwise, I have to audit any components that might appear in any noscript I create, and make sure that they themselves don't have noscripts, and also that my component will never appear in a noscript, or the whole app would break. It seems like a no-brainer to me that it should be React's job to handle this, not the author's.

Under this definition of "fixed," the code is not fixed as of 15.3.1:

$ babel-node
> const ReactDOMServer = require("react-dom/server");
'use strict'
> const React = require("react");
'use strict'
> const c = <noscript><span /><noscript><div /></noscript><p>sneaky</p></noscript>;
'use strict'
> ReactDOMServer.renderToStaticMarkup(c);
'<noscript><span></span><noscript><div></div></noscript><p>sneaky</p></noscript>'
> ReactDOMServer.renderToString(c);
'<noscript data-reactroot="" data-reactid="1" data-react-checksum="1778990683"><span data-reactid="2"></span><noscript data-reactid="3"><div data-reactid="4"></div></noscript><p data-reactid="5">sneaky</p></noscript>'
> 

(The fiddles above also demonstrate this IMO-incorrect behavior, which is why I was confused that this is marked "fixed.")

[brigand]

Shouldn't this warn like other invalid nesting situations?

[wchargin]

Given that no one else has chimed in…in my opinion, no, this should not warn like other invalid nesting situations.

In my understanding (please correct if wrong), the current invalid nesting situations happen when you forget a tbody, or put a p inside a ul, or something. These situations at worst cause the browser to reflow (not great) and React to have to re-render the entire subtree (not great). This isn't good, but—crucially—there is no bad behavior visible to the end user as a result, and so it makes sense to warn.

On the other hand, nesting noscripts causes React to emit invalid HTML that can severely mess up a site (primarily, by splicing in arbitrary closing tags from the noscript, and also of course by displaying some of the noscripted content). That calls for more than a warning, as it has an effect that's very visible to users.

Yes, in HTML it is invalid to treat nested noscripts as if they were elements. But React is not HTML. A component is supposed to be an atomic unit; it's critical that I be able to drop in a <div><Foo /></div> without wondering whether that will break my entire layout. Again, this is all about enabling local reasoning of code, which is trivial in plain HTML (because the code's all in one place) but undecidable in React (because components can come from anywhere and be embedded anywhere). So, in my opinion, React should interpret <noscript> as "a component whose contents should only be displayed when Javascript is disabled." This requires properly handling the case of nested noscripts.

[sophiebits]

No, in HTML the browser changes the parse tree when it encounters certain types of invalid nesting which causes React to get confused at nodes in the wrong place. It sounds like the precise mechanics of how noscript parsing works are slightly different, but it would have the same effect of messing the hierarchy up.

[wchargin]

@spicyj: Okay—that makes sense that the parse tree changing confuses React.

But if React never emits nested noscripts (as I'm suggesting) then there's nothing to be confused by, right? (And if it does then it gets confused and the page looks totally wrong.)

[sophiebits]

Maybe. I could go either way. It could also be surprising to ask to render a <noscript> tag and have one not appear. Plus we don't have an easy way in the code to make this happen right now even if we wanted to.

I am happy to make validateDOMNesting warn about nested noscripts if it doesn't already – send a PR?