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

Randomly generated classNames bust cache on hosting services like Netlify (and maybe browsers too...?) #154

Open
f-kottek opened this issue Apr 20, 2021 · 10 comments
Labels
bug Something isn't working not stale This issue has gone quiet but will be kept open

Comments

@f-kottek
Copy link

f-kottek commented Apr 20, 2021

Summary

When you are using this plugin with a service like Netlify, you end up generating a lot more files to upload at each build, than you need to. Pages that don't change at all, but include <BgImage />-components will get a new hashed className on each rebuild. This will invalidate Netlifys cache and the file will be uploaded as a new one. This will on some scale be detrimental to build times and because of the cache-busting might even lead to worse page speed scores.
(And aren't we all after the perfect 100/100/100 with Gatsby? 🥇)

Basic example

This will be rendered by the component:

<div 
   class="arbitrary-class gbi-1355162470-9c1tdoNQuJjhFPBaGNHdzZ"
   style="...some style"
>
{ ... some stuff }
</div>   

on the next rebuild, the hash-value will change -> problem
I just added the "arbitrary-class" for demonstration purposes.

Motivation

  • possibly better lighthouse scores
  • definitely faster build times (depending on the scope/size of your project)
  • more efficient caching

Possible Solution

I propose a prop/flag that lets you opt out of hashed classes. I know the reasons why the hash-value is there, but it currently generates some unwanted and detrimental behaviors. I guess if you're using this flag, you can get away with setting individual classNames for every instance of the component, but that's something that you need to be aware of. Maybe therefore name the prop something like "dangerouslyOptOutOfHashes" to remind yourself of setting a distinct className? 🤔

@timhagn
Copy link
Owner

timhagn commented Apr 21, 2021

Hi @f-kottek,

that's a strange behavior. As in lib/StyleUtils.js:L33 the additionalClass should only get added if no className is defined:

  // Should an element already exist or have no className, add randomized class.
  const additionalClass = elementExists || !className ? randomClass : ``;

gbi had a flag like this way back, but this mechanism usually worked. Would you provide a repro case?

Best,

Tim.

@f-kottek
Copy link
Author

f-kottek commented Apr 21, 2021

Hey @timhagn,

it's a private repo for a company-website, so I can't share the whole thing unfortunately.
Here's a small insight tho:
This is part of a component (shortened) that uses gbi:

const { speechBubble } = useStaticQuery(graphql`
    { ...dataQuery  }
  `)

  const speech_bubble_image = getImage(speechBubble)

  return (
    <BgImage
      style={{ backgroundPosition: "bottom" }}
      image={speech_bubble_image}
      className="unique-class-thats-unused"
    >
... some stuff 
</BgImage> 
)

The instance of BgImage is the only one in the component and on the page. The class name is arbitrary, as in the first example and only for demo purposes,

The page works fine, the image is there and in the background etc. -> all good.
Here is the rendered HTML tho, including the hashed additional class:

<div class="unique-class-thats-unused gbi-1355162470-4RWoXwJNbK3unvurphB7rQ" style="...style stuff;"><style>
          .unique-class-thats-unused.gbi-1355162470-4RWoXwJNbK3unvurphB7rQ:before,
          .unique-class-thats-unused.gbi-1355162470-4RWoXwJNbK3unvurphB7rQ:after
 { .... other stuff }
 </div>

some info from our package.json that might help/be important:

    "gbimage-bridge": "^0.1.2",
    "gatsby-plugin-image": "^1.2.1",
    "gatsby": "^3.2.1",

Edit: not using this plugin anymore

@timhagn
Copy link
Owner

timhagn commented Apr 21, 2021

Hi @f-kottek,

now I got you. It is a bug & I even already know where & why it happens (see lib/StyleUtils.js. elementExists is set to true (class in cache) after the first state update (e.g. base64 image) -.-
Don't know how to fix it without breaking multiple image capabilities right now, but as a quickfix you could change line 26 in the above mentioned file in your node_modules/gatsby-background-image and fixate it with patch-package to this:

  var elementExists = false; //(0, _ClassCache.inComponentClassCache)(className);

Will look into it further as soon as I find the time (just closing shop for the day and am in Austria ; ).

Grüße aus Wien : )!

Best,

Tim.

EDIT: add correct patch-package link

@timhagn timhagn added bug Something isn't working not stale This issue has gone quiet but will be kept open labels Apr 21, 2021
@f-kottek
Copy link
Author

f-kottek commented Apr 22, 2021

Hey @timhagn,

it works, thanks!
(also thank you for pointing out patch-package, I didn't know such a thing existed, but this is definitely very nice to know! 👍🏻)

@mwskwong
Copy link

I believe my issue might be related to this.
For me, I got a Prop `dangerouslySetInnerHTML` did not match warning after enabling DEV_SSR (i.e. use SSR on DEV build). I believe this is due to the hash is being regenerated unnecessarily.

@OloZ17
Copy link

OloZ17 commented Jul 2, 2021

The same here. I got a Prop dangerouslySetInnerHTML did not match warning. DEV_SSR was enabled by default.
I put in gatsby-config.js :

flags: {
    DEV_SSR: false,
  },

to solve the problem.

@mwskwong
Copy link

The same here. I got a Prop dangerouslySetInnerHTML did not match warning. DEV_SSR was enabled by default.
I put in gatsby-config.js :

flags: {
    DEV_SSR: false,
  },

to solve the problem.

This technically doesn't solve your problem. You just hide it. AT the end of the day, you have to build the project and do SSR. The same problem will still be hit when doing rehydration on the client-side, and it has a major impact on performance even though the warning message is hidden.

@dmorda
Copy link

dmorda commented Jun 13, 2022

I've recently noticed an issue with several of my Gatsby sites that use this plugin in combination with gbimage-bridge and gatsby-plugin-image. Since upgrading to React 18, production builds display console errors indicating a Minified React error. The errors have to do with hydration and the UI rendered being different than what's on the server.

Screen Shot 2022-06-13 at 5 50 51 PM

I chased this down a bit and think it's related to this issue in that the component is automatically generating a random hash for classnames and also generating a new one on each refresh of the browser. So you don't actually need to rebuild the project and deploy it, if you refresh your browser you will see different classnames.

For example, here's an abbreviated version of what the wrapper tag looked like when I first visited the page.

<section class="gbi-1463115190-9fBPJeQinocWrPea1rg1J4">
[...]
</section>

And then after I refreshed, here's an updated version of the tag.

<section class="gbi-1463115190-oEMGZHRCVbr2cn7uL5ZHLz">
[...]
</section>

I reverted back to React 17 and didn't see these hydration errors in production builds, so it's only after upgrading to React 18 that they seem to appear and there's no errors displayed when running in development mode. I also tried the same thing in React 17 and the classnames do change on every page refresh.

I've created a repo to easily reproduce the problem.

git clone https://github.com/dmorda/bgimage-issue.git
npm install --legacy-peer-deps
npm run build && npm run serve

Once it's up and running, open a browser at http://localhost:9000 and view the console errors.

@dmorda
Copy link

dmorda commented Aug 17, 2022

Any progress on this one?

@DamageSami
Copy link

I can confirm problems relating to this issue too. With React 18, I get the Minified React error #418 on the built production version of the site, and in dev with DEV_SSR: true I get a Warning: Prop 'dangerouslySetInnerHTML' did not match. and Uncaught Error: Hydration failed because the initial UI does not match what was rendered on the server..

After reverting back to React 17, I see only the Warning: Prop 'dangerouslySetInnerHTML' did not match.. Does reverting back to React 17 help in any other way than that you just do not get the error message in the console?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working not stale This issue has gone quiet but will be kept open
Projects
None yet
Development

No branches or pull requests

6 participants