Ignore <noscript> content on the client and don't warn about mismatches

[stephen-last]

React 16.0.0 with SSR & lazysizes 4.0.1.

Trying to use the "the noscript pattern" to lazy load images with lazysizes but I'm seeing this:

Warning: Expected server HTML to contain a matching <img> in <noscript>.

Image component render method:

render () {
    const { cdn, url, width, height } = this.props

    if (!url) return null

    const noScriptImgProps = {
      src: `${cdn}${url}`,
      className: classNames('product-image'),
      width,
      height
    }

    const imgProps = {
      'data-src': `${cdn}${url}`,
      className: classNames('product-image', 'lazyload'),
      width,
      height
    }

    return (
      <span>
        <noscript>
          <img {...noScriptImgProps} />
        </noscript>
        <img {...imgProps} />
      </span>
    )
  }

Does React have an issue with noscript tags..?

[Imundy]

I think that warning should be a result of the client failing to match the server side rendered content when it tries to hydrate HTML.

[stephen-last]

Ok... I've tried removing lazysizes and just adding a simple <noscript> via a wrapper component:

function NoScript (props) {
  const staticMarkup = ReactDOMServer.renderToStaticMarkup(props.children)
  return <noscript dangerouslySetInnerHTML={{ __html: staticMarkup }} />
}

return (
      <span>
        <NoScript>
          <p>.</p>
        </NoScript>
        <img {...props} />
      </span>
    )

Now I get:

Warning: Prop dangerouslySetInnerHTML did not match. Server: "&lt;p&gt;.&lt;/p&gt;" Client: "<p>.</p>"

So the server has encoded HTML, the client doesn't.

Why would that be..?

[gaearon]

Warning: Prop dangerouslySetInnerHTML did not match. Server: "&lt;p&gt;.&lt;/p&gt;" Client: "<p>.</p>"

This is a bug in 16.0.0: #11103. It was fixed in master via #11119 and will be released in the next update.

[stephen-last]

@gaearon Thanks for the update, and fix... I'll ignore the warning for now.

Any idea when you're likely to release the next update..?

[gaearon]

This or next week. But I've said this a few times in the past...

[gaearon]

React 16.1.0-beta has been released. Please update react, react-dom, and react-test-renderer (if you use it) to this version and let us know if it solved the issue! We’d appreciate if you could test before Monday when we plan to get 16.1.0 out.

[stephen-last]

@gaearon Testing now...

With 16.1.0-beta installed for both react and react-dom the NoScript functional component mentioned previsously now works, that is, I no longer see this:

Warning: Prop dangerouslySetInnerHTML did not match. Server: "&lt;p&gt;.&lt;/p&gt;" Client: "<p>.</p>"

However, and this maybe a separate thing, I still seem to have an issue using <noscript> directly. If I use:

      <span>
        <noscript>
          <img {...props} />
        </noscript>
        <img {...props} />
      </span>

I see this:

Warning: Expected server HTML to contain a matching <img> in <noscript>.

The HTML from the SSR (via view -> source)...

<span>
  <noscript>
    <img src="https://d3iq5gspowosu2.cloudfront.net/dev/gifts/images/categories/483/1-300x300-beatrix-potter.jpg" class="product-image"/>
  </noscript>
  <img src="https://d3iq5gspowosu2.cloudfront.net/dev/gifts/images/categories/483/1-300x300-beatrix-potter.jpg" class="product-image"/>
</span>

The HTML in Chrome DevTools...

[gaearon]

Can you please provide a reproducing project?

[stephen-last]

@gaearon Ok... I'm a github rookie, but how is this..?

https://github.com/stephen-last/react-16-noscript

It's a basic app using SSR with a <noscript> tag.

The only difference I can see between the server generated HTML and that shown in the DevTools, is that the img tag is closed with /> in the page source, but DevTools shows >. I guess that could just be how DevTools shows it though.

[gaearon]

OK, I understand why this happens now. But it’s not clear to me what’s the best fix.

As a workaround you can replace this:

        <noscript>
          <img src='https://placeimg.com/300/200/animals' width='300' height='200' />
        </noscript>

with this:

        <noscript
          dangerouslySetInnerHTML={{
            __html: "<img src='https://placeimg.com/300/200/animals' width='300' height='200' />"
          }}
        />

I verified it works and doesn’t warn.

The reason you see the mismatch is because the browser is parsing the noscript content as text, but React attempts to hydrate it as a real tree. Maybe React shouldn’t attempt to hydrate anything in noscript.

[gaearon]

This seems like an earlier manifestation of this issue: #1252. Seems like in past version it was just completely broken instead of warning, but now it produces a false positive warning.

[gaearon]

Spoke briefly with @sebmarkbage.

It's not clear how this should work.

We are currently generating elements on the client with createElement. But noscript content is not meant to be an element; it's text content.

One possible solution is to treat noscript like textarea and only support text content in it. That would be consistent with how browser treats it, but would disallow the pattern in the OP post.

We could implement support for setting text content to an element that can resolve to that content. Since components can return strings now. Like <noscript><MyThing /></noscript> where MyThing generates a string. But it's not clear how useful this is. If we do this though, we should probably support it for textarea as well. And perhaps even for attributes like <a title={MyThing />} />.

[stephen-last]

@gaearon Thanks for your time investigating this, and for the workaround suggestion.

For now I'll keep a global component in my app for these tags:

function NoScript (props) {
  const staticMarkup = ReactDOMServer.renderToStaticMarkup(props.children)
  return <noscript dangerouslySetInnerHTML={{ __html: staticMarkup }} />
}

So I can use <NoScript> rather than <noscript>.

It makes sense to me to treat it as the browser does, like you suggest.

[gaearon]

After more discussion we settled on warning if <noscript> contains a child that isn’t a string since it’s not really supported.

[sebmarkbage]

@stephen-last It might be better to use <noscript>{staticMarkup}</noscript>. That way we have the opportunity to escape it if needed. Less dangerous.

[sebmarkbage]

I guess in browsers where scripts are disabled this wouldn't be parsed as text content. So it's legit to put elements in there.

The issue is what happens on the client. Maybe client rendering should just ignore the content completely (and ignore hydration errors)?

I don't quite fully understand the use cases where the content is used during a script enabled render. Are there such use cases?

[stephen-last]

@sebmarkbage When I use <noscript>{staticMarkup}</noscript> I get:

Warning: Text content did not match. Server: "&lt;img src=&quot;...&quot; class=&quot;product-image&quot; width=&quot;50&quot;/&gt;" Client: "<img src="..." class="product-image" width="50"/>"

Using <noscript dangerouslySetInnerHTML={{ __html: staticMarkup }} /> produces no warnings with 16.1.0-beta.

On the general <noscript> issue, after thinking about it over the weekend, I agree with @sebmarkbage. On the server React should allow any valid HTML or text. On the client React should ignore the contents completely, as that content only exists for when React doesn't - in that way React has nothing to do with contents of <noscript>.

I can't think of a use case for React, or any script, to use the content of <noscript> tags on the client.

[damianstasik]

@gaearon the <noscript> spec doesn't say anything about supporting string only, could you explain a little bit more why are you going in the string-only direction?

[gaearon]

I think we should just do this:

Maybe client rendering should just ignore the content completely (and ignore hydration errors)?

[Ephem]

I've been tracking this issue since #1252 and #6204 so I'll chime in with our current use-case and some thoughts (long post but some background is necessary I think).

We use this mainly for A/B-testing for cases where we want to determine the bucket on the client and render a component only after that. Often we still want to provide a default variant (in a noscript-tag) for people without JS, or as graceful degradation in cases where JS has crashed.

Providing a noscript-fallback for conditional functionality that is determined on the client might be a bit niche but shouldn’t be that uncommon either. It could be as easy as <noscript><DefaultComponent /></noscript> but to make this work fully we have had to:

On the server:

1. Render default component with renderToStaticMarkup (including using a couple of Providers to make Redux and react-router work)
2. Remove all nested noscript-tags in the resulting string (quite common for us since we also use noscript-fallbacks for lazy-loaded images)
3. Use dangerouslySetInnerHTML

On the client:

1. Alias react-dom/server to noop
2. Instead of renderToStaticMarkup - Read existing textContent/innerHTML of the noscript-tag
3. Use dangerouslySetInnerHTML with the above
4. (If JS crashes before bucket has been set - Catch error and replace all relevant noscript-tags with divs and set their innerHTML with the innerHTML of the noscript-tag)

TL;DR
This leads me to two reflections:

1. The proposed solution to ignore the content of noscript-tags on the client is perfect (as long as we still keep the existing content in them).
2. While it's really really awesome that React 16 now handles <noscript><img /></noscript>, if we want a really great story around noscript, I’d say automatically removing nested noscript-tags as per suggestion in Nested <noscript> renders invalid HTML #6204 is key. Would this be doable in the new codebase? Either way it should be a separate issue.

[Ephem]

About the issue at hand though, I’ve been trying to start digging into and investigating the codebase. Supressing the hydration warning does not seem that hard at a glance, but I can’t quite figure out where the best place would be to ignore the subtree of noscripts?

I guess this should live in react-dom, but we would also like to make sure the reconciler doesn’t waste time on these subtrees, is there currently a valid place in the reconciler API where we could achieve this?

[Ephem]

Oh, and just to be clear, what I'm saying is that there is a valid usecase for using noscript content even when JS is turned on, manually simulating "noscript". We currently do this both for graceful degradation and in cases where we choose not to load the React-application for performance reasons on older devices.

One should only ever have to use the content that was provided from the server in these cases though, which is why I think the proposed solution is a good one.

[Ephem]

A very naive way to solve this issue:

  // In client/ReactDOM.js

  shouldSetTextContent(type: string, props: Props): boolean {
    return (
      type === 'textarea' ||
      type === 'noscript' || // <-- This is new
      typeof props.children === 'string' ||
      typeof props.children === 'number' ||
      (typeof props.dangerouslySetInnerHTML === 'object' &&
        props.dangerouslySetInnerHTML !== null &&
        typeof props.dangerouslySetInnerHTML.__html === 'string')
    );
  },

This solves the hydration warning, makes the reconciler ignore everything inside <noscript> but keeps the content in the DOM (since it's already there) and all current tests are passing.

Not sure if there are any side effects to this, I did notice the React devtools acting a bit weird. It didn't show any content in the <noscript> (since React doesn't really know about it) and said its children looked like this:

Not sure if anything there is indicative of something undesirable or not.

I'm new to the codebase so I thought I'd mention the fix here for feedback since I might be way off, but would be happy to submit a PR for discussion if it seems feasible of course.

[muhammadInam]

hey @Ephem, are you still working on the fix? If not, then I would like to give it a go. Let me know, thanks

[Ephem]

Hey @muhammadInam! Since I am unsure if the direction of the fix I suggested is good I'm waiting for feedback before doing a PR, maybe @gaearon who's been active in the thread could chip in? (Only my last post is relevant in regards to a fix, rest is just background and context around how we use noscript and a bit wall-of-texty, sorry..)

Basically I think my questions are:

• Is shouldSetTextContent too "late" a place to solve this optimally since the reconciler already knows about the children, props etc?
• Saying that <noscript> only contains text is kind of true on the client and solves the problem, but does it have any bad consequences/side-effects?

If this is an obviously bad direction I'd be happy to let it go and let you work on it @muhammadInam, if it's not terrible at first glance I'd be happy to open a PR and continue discussions there. :)

[asimqt]

I tried with an iframe, the below approach doesn't work.

<noscript
          dangerouslySetInnerHTML={{
            __html: "<iframe src='https://www.googletagmanager.com/ns.html?id=1234'></iframe>"
          }}
        />

It's still coming as a string. For image,p etc have no problem. (Please forget the mismatch between src and all. Not working in both scenarios). And I found some other issues too if I used string literals. <iframe src="https://www.googletagmanager.com/ns.html?id=${val}></iframe>

[benschac]

cc: @gaearon happy to try grabbing this one too if Y'all still need someone to work on it.

[gaearon]

Sounds good. I haven’t looked into this again yet. A proof of concept would help the conversation.

[benschac]

wonderful. Putting this in my queue. ty for the quick response @gaearon

[benschac]

This is most likely the wrong place to ask this question. I couldn't get on IRC or discord. I'm getting the error:

error Received malformed response from registry. The registry may be down.

when running yarn in root.

I'm running node v8.11.4 yarn version v0.17.9 I went through the docs and watched the youtube video.

Can anyone point me in the right direction to getting the project running on my machine? I'm on OSX.

[gaearon]

Yarn 0.17 sounds super old. Isn't current version 1.x something?

[babipal]

I am also using the noscript tag in a project in a similar fashion as @stephen-last. I have a long page of images, most of which are lazy loaded. For the ones that are lazy loaded, there is a coresponding noscript tag containing an image tag with the target source, this is the Google recommended way to provide SEO content for lazy loaded images.
When React Hydration occurs, it looks like the img tag within the noscript tag gets 'executed', and the image is getting downloaded in the browser. If I look at the network tab in Chrome, I can see all of the noscript wrapped images have been requested. If I block React Hydration, and only rely on the SSR HTML, the noscript wrapped images do not get downloaded.
React components that contain noscript tags should not attach the child contents as actual DOM nodes.

[tomstuart]

Yes, the mere construction of the <img> DOM node causes the browser to pre-emptively download the src image. The download starts before the node has even been attached to the document, and therefore irrespective of whether it will eventually become a child of a <noscript> element.

[Ephem]

By adding a case to the SSR fixture I verified today that:

<noscript dangerouslySetInnerHTML={{ __html: "<img src='...' /> }} />

still works in master, but that:

<noscript>
  <img src='...' />
</noscript>

is still broken in the way described, that is, it unnecessarily downloads the image.

I also verified that the solution I proposed in the comment above (adding type === 'noscript' to shouldSetTextContent) does indeed make the second case work as expected.

I would still be happy to send a PR for this, including the updated fixture. I might just go ahead and do that this weekend to have a better place to discuss the proposed solution. :)

[gaearon]

Thanks @Ephem for fixing this. I think the fix makes sense but the issue itself is a bit of a mind-bender so if somebody still has problems in the next version (probably 16.4.3) please raise a new issue and reference this one.

[ptdede]

After update to react 16.5.0 the warning is gone and <noscript> content rendered even the child is not string. Thanks for the fix guys.