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

noscript contents cause invariant violation #1252

Closed
matthewwithanm opened this issue Mar 12, 2014 · 18 comments
Closed

noscript contents cause invariant violation #1252

matthewwithanm opened this issue Mar 12, 2014 · 18 comments

Comments

@matthewwithanm
Copy link
Contributor

When using server rendering, putting an <img> in a <noscript> seems to invariably cause an invariant violation (it can't find the image).

I believe this is because, to the JS enabled browser, the noscript content looks like CDATA.

This can be worked around by using dangerouslySetInnerHTML to actually set the contents to an HTML string, however, you can't nest components with this approach.

@matthewwithanm
Copy link
Contributor Author

Now that we have renderComponentToStaticMarkup, maybe the best solution for this is just to do:

<noscript dangerouslySetInnerHTML={{__html: React.renderComponentToStaticMarkup(c)}} />

where c is the component you want to render for people without JS.

@sophiebits
Copy link
Contributor

We should just make noscript do this automatically, sort of like our existing wrappers for input, select, and textarea. Want to take a crack at it?

@matthewwithanm
Copy link
Contributor Author

Sure. I'll try to get a PR in soon.

@andygeers
Copy link

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

@syranide
Copy link
Contributor

syranide commented Feb 5, 2015

Related #1905 ... should we even set the content of <noscript> client-side? (Or technically even render it when React is able to) It's a perf hit and I can't think of a reason you would ever care about the contents of the <noscript> when client-side?

PS. I feel like there's something dirty about renderToStaticMarkup happening behind the scenes. Wouldn't it make more sense to just disallow children and if people want the implicit behavior it should be wrapped up in a custom component?

@matthewwithanm
Copy link
Contributor Author

Yeah, I think I suggested ignoring <noscript> contents on the client in #1253, but it would cause checksum issues with SSR if I'm not mistaken.

As for rendering to a string, it may be dirty but, in my opinion, it's the acceptable kind of dirtiness since it protects the user from the inherent complexity of dealing with a wonky browser bug. I guess you could argue that <noscript> should have a CDATA child (that just happens to be a string of HTML) and not actual DOM children, so it's outside the purview of React, but that seems to make it largely useless without that custom component.

@syranide
Copy link
Contributor

syranide commented Feb 5, 2015

@matthewwithanm Oh right, yeah, reusing client-side would require a special flag internally which would use server-rendering rules when constructing the initial markup.

@defrex
Copy link

defrex commented Feb 28, 2015

For future searchers, here is this workaround wrapped up into a component. In this case set up as an ES6 CommonJS module that relies on the constant process.browser. Adapt as needed.

const React = require('react/addons');

module.exports = React.createClass({
  displayName: 'NoScript',
  mixins: [React.PureRenderMixin],
  render() {
    if (process.browser)
      return <noscript/>;
    else
      return <noscript dangerouslySetInnerHTML={
        {__html: React.renderToStaticMarkup(this.props.children)}
      }/>;
  }
});

@Jpunt
Copy link

Jpunt commented Dec 9, 2015

I'd like to use this workaround, but it seems to be deprecated:

Warning: React.renderToStaticMarkup is deprecated. Please use ReactDOMServer.renderToStaticMarkup from require('react-dom/server') instead.

I'm guessing that ReactDOMServer is not available on the client. So, now what?

@benkeen
Copy link
Contributor

benkeen commented Feb 4, 2016

@Jpunt me too. Being able to convert a React component to static markup on the client came in handy sometimes.

@jimfb
Copy link
Contributor

jimfb commented Feb 4, 2016

@Jpunt @benkeen

ReactDOMServer.renderToStaticMarkup() is available on the client side if you include <script src="https://fb.me/react-dom-server-0.14.7.js"></script>

@benkeen
Copy link
Contributor

benkeen commented Feb 4, 2016

@jimfb you da MAN. Thanks, that's very helpful!

@srcspider
Copy link

noscript is a very critical component even for a javascript-dependent application. If anything even the utility it provides (eg. "you need javascript to use this page") is even more essential to javascript-only pages then normal ones.

@Jpunt @jimfb ( /cc @benkeen )

Maybe there are uses for rendering static markup on the client but noscript is not one of them.

If react is running then the noscript is clearly useless now since javascript is enabled in the browser and hence the contents of the noscript may as well not exist. The only reason a noscript tag even makes sense to generate (instead of just returning null) is to avoid an invariant error because react expects it from the server side.

var React = require('react');

var is = {
    browser: require('lib/is/browser')
};

var ReactDOM;
if ( ! is.browser()) {
    ReactDOM = require('react-dom/server')
}

var NoScript = React.createClass({

    mixins: [ React.PureRenderMixin ],

    render: function () {
        if (is.browser()) {
            return <noscript/>;
        }
        else { // on server
            return <noscript dangerouslySetInnerHTML={
                { __html: ReactDOM.renderToStaticMarkup(this.props.children) }
            } />;
        }
    }

});

module.exports = NoScript;

You'll want to alias the react server code to resolve to empty module since it's unlikely you're bundler will figure it out on it's own; not with out some other rules in place anyway. If you use webpack add the following to your configuration:

resolve: {
    alias: {
        "react-dom/server": "empty-module"
    }
},

And make sure you have empty-module on your load paths. I recommend you do npm i -D empty-module, since a dependency is clearer (and cleaner) then an empty file in the project.

@wchargin
Copy link

@srcspider: Could you explain why you choose to render an empty noscript on the client instead of the same noscript with dangerouslySetInnerHTML? When I try to do this I get a server-side rendering mismatch

 (client) t data-reactid="25"></noscript><div clas
 (server) t data-reactid="25">&lt;div&gt;&lt;secti

which totally makes sense.

Rendering the static HTML in all cases works for me, and should be fine performance-wise with the pure-render decorator.

@BabakMN
Copy link

BabakMN commented Sep 11, 2016

I think this issue should be highlighted somewhere on the documentation.

The following seems to work well for me both on client and server (thanks to people above):

Noscript.js

import React from 'react';
import ReactDOM from 'react-dom/server';


export default function Noscript(props) {

    // https://github.com/facebook/react/issues/1252

    const staticMarkup = ReactDOM.renderToStaticMarkup(
        props.children
    );

    return <noscript dangerouslySetInnerHTML={{ __html: staticMarkup }} />;

}

Usage:

import Noscript from './Noscript.js';
<Noscript>
   <div className="noJavascript">
      <div>
         <p>
         Your browser has disabled Javascript. Please note this website requires Javsacript to function correctly.
         </p>
      </div>
   </div>
</Noscript>

@wchargin
Copy link

@BabakMN This is a good first-order approximation (and I use it), but it has the severe limitation that you cannot nest Noscripts; see #6204. This destroys local reasoning. If this approach is highlighted in the documentation, then either #6204 must be either fixed or the documentation must warn aggressively about this.

@BabakMN
Copy link

BabakMN commented Sep 11, 2016

@wchargin I just dug into #6204, very interesting. I agree with you that squashing the <noscript> tags into one is the better behaviour.

@gaearon
Copy link
Collaborator

gaearon commented Nov 3, 2017

I believe React 16 doesn’t crash on this anymore, even though it produces a false positive warning.
We’ll track the warning in #11423.

@gaearon gaearon closed this as completed Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests