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

preact rendering <style> tag inside <noscript>, whereas react does not #2797

Closed
jaredh159 opened this issue Oct 13, 2020 · 5 comments
Closed
Labels

Comments

@jaredh159
Copy link

I'm not sure if this matters to the library maintainers, or how much compatibility/interop with React is a goal, but I noticed a subtle difference between React and Preact that caused a fairly big blocker trying to switch a Gatsby site over to preact.

Basically, this jsx:

<noscript>
  <style dangerouslySetInnerHTML={{ __html: "body { background-color: pink }" }} />
</noscript>

Renders differently between React and preact. In react, you get an empty <noscript> tag. Preact renders out the <style> tag inside the noscript. Codepens below.

The use-case here is that I'm trying to use gatsby-background-image which adds styles to noscript like the example above. With react, nothing renders, but with preact, I get extra style tags which load in un-optimized images, destroying any performance benefit gained from switching to preact.

Reproduction

Codepen with React:
https://codepen.io/jaredh159/pen/NWrGyPq?editors=1010

Codepan with Preact:
https://codepen.io/jaredh159/pen/mdEeXJg?editors=1010

Note: you'll have to use the dev tools to inspect to see the <noscript> contents, as it's not visible, of course.

Steps to reproduce

See codepens above ^^^

Expected Behavior

I would have hoped that preact matched the behavior of react.

Actual Behavior

preact rendered the contents of the <noscript> tag, unlike React.

@marvinhagemeister
Copy link
Member

That sounds like React isn't following the HTML-Spec correctly. It states:

When scripting is disabled, not in a <head> element: transparent, but there must be no <noscript> element descendants.

Which translates as any of the usual elements is a valid descendant of a <noscript> element, even <style> tags. The rules are different when the <noscript> is inside the <head> section of the document. In the linked codesandbox that's not the case.

Looks like React has a bug there. I can't come up for valid reasons to disallow style-tags in that context as it is often used to customize the fallback experience on sites I've seen.

@jaredh159
Copy link
Author

@marvinhagemeister yeah, i understand your reasoning here. digging into it a bit more, it seems react made a conscious decision to ignore the contents of the <noscript> when they are client-rendering:

https://github.com/facebook/react/blob/e614e6965749c096c9db0e6ad2844a2803ebdcb6/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js#L594

Which kind of makes sense, if you think about it -- if react is there, doing things on the client, then the <noscript> by definition is going to be not necessary. So maybe in a sense they consider it a performance optimization? A little more background here:

facebook/react#11423

@developit
Copy link
Member

Here's a patch that makes Preact skip noscript contents on the client only:

import { options } from 'preact';
const CLIENT = typeof document !== 'undefined';
const old = options.vnode;
options.vnode = vnode => {
  if (vnode.type === 'noscript' && CLIENT) {
    vnode.props.children = null;
  }
  if (old) old(vnode);
};

@jaredh159
Copy link
Author

@developit thanks! I didn't know about the options hooks API, but i looked it over and I think I understand. I'm not exactly sure where I'd implement this, but I'm guessing if I stick it in some entry-type file to hook in before Preact does any work, that would be the ticket. I'll give it a go. Appreciate the help!

@developit
Copy link
Member

@jaredh159 yep! Just have it run before your component code.

developit added a commit that referenced this issue Jul 30, 2021
This fixes the issue from #2797, and [this issue](https://zenn.dev/d_suke/scraps/f7013c4554ac43) using Next.js's `<Image>` component with Preact.
JoviDeCroock added a commit that referenced this issue Oct 12, 2021
* [compat] Skip rendering <noscript> contents on the client

This fixes the issue from #2797, and [this issue](https://zenn.dev/d_suke/scraps/f7013c4554ac43) using Next.js's `<Image>` component with Preact.

* Update render.js

Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants