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 compatibility #131

Closed
jaredh159 opened this issue Oct 13, 2020 · 7 comments
Closed

preact compatibility #131

jaredh159 opened this issue Oct 13, 2020 · 7 comments

Comments

@jaredh159
Copy link

Really appreciate this library! Thanks so much for all your work on it! 🙏

An issue was closed recently, for being stale: #127.

I realize you don't claim to support preact, but I spent a while digging into the problem with gbi and preact this morning and figured out what the issue is, so I thought I'd at least see if you had any insight on whether it's easily fixable on the gbi end, or if you had some guidance to help me possibly submit a PR if you could see a way forward to be compatible with preact.

Basically, the problem with preact boils down to this issue, which I raised on the preact project:

preactjs/preact#2797

The gist of that issue is that this code

<noscript>
  <style
    dangerouslySetInnerHTML={{
      __html: noScriptPseudoStyles,
    }}
  />
</noscript>

renders nothing inside of the <noscript> tag when run with react, but when preact runs it, the style tag is rendered. The browser then ends up reading the style tag inside the tag and actually downloads the unoptimized image from the <noscript>.

So, obviously, it's a quirky edge case difference between react and preact, and that's why I raised an issue over there as well. But, I wanted to ask if this was intended behavior or not. It's odd to me that this library is trying to render these fallback styles in a noscript tag, and I wondered if you knew that they were not rendering on the client (they might be rendering on the server I suppose...). And I also if you knew that when they were present in the DOM that the sub-optimal image format ends up getting downloaded by the browser, defeating the purpose of gbi.

Is there an opportunity to improve gbi here? Or can you maybe explain a little bit more of the reasoning behind the stuff an how you intended it to work? If you would accept a pull request for preact compatibility, could you give me any help on where to get started? Thanks again!

@jaredh159
Copy link
Author

jaredh159 commented Oct 20, 2020

well, for anyone else who stumbles on this and wants to use preact + gbi, I forked the repo, omitted rendering the noscript style tag (whatever trade-off that represents, if any, feels worth it for my project and my usage of gbi), and published a npm package for myself. if it's helpful for other folks, you can install it with:

npm install gatsby-background-image-preact

or

yarn add gatsby-background-image-preact

@ahansson89
Copy link

Thanks for debugging this @jaredh159! I had this issue on a site for a while, but was not able to find the root cause. I then decided to just live with it until someone fixed it upstream - switched over to your library fixed it. :)

@jaredh159
Copy link
Author

@ahansson89 sure thing, glad it worked for you. Wanted to mention that in my other issue over in preact, the preact author pointed out a way to workaround the issue:

preactjs/preact#2797 (comment)

I haven't gotten around to it, but I think i'll be able to implement that patch in my gatsby codebase and then go back to depending on the regular gatsby-background-image package, so I don't need to track upstream changes with my preact-version. fwiw. 👍

@timhagn
Copy link
Owner

timhagn commented Nov 4, 2020

Hi @ahansson89 & @jaredh159!

Gonna try looking into this issue & the patch from preact soon, haven't been able to do Web-Dev (was in a React Native Gig) for quite some time ; ).

Best,

Tim.

@timhagn
Copy link
Owner

timhagn commented Nov 19, 2020

Hi @ahansson89 & @jaredh159,

thanks a lot for pointing this out & linking the issue. After reading through the Specs & thinking about it a little more, there's probably never gonna be a moment, when noscript content is needed with JS activated. So I just added a !isBrowser() to the hasNoScript check, removing noscript from hydrated content. Works at my place; would you be so kind to test it on your side with the just published [email protected]? Should it work as intended, you may of course have the honor to close this issue : )!

Best,

Tim.

@jaredh159
Copy link
Author

jaredh159 commented Nov 19, 2020

Tim, thanks so much, i'll give it a shot later today and get back to you. Really appreciate your taking the time to do this.

@jaredh159
Copy link
Author

Tim, seems to work as intended. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants