Emotion: Hashed className key collisions

[coreyward]

Current behavior:

We encountered an issue recently where a single page of a moderately sized blog was rendering a component incorrectly. A quick investigation discovered that the generated class name assigned (css-r1ywwf) was identical to another, correctly-styled component on the page, and the styles associated with the duplicate class were consistent with the correctly-styled component.

This led to some spelunking into how Emotion generates the hash, which lead me to the @emotion/hash package, which has a nearly unintelligible (bit shifting, hexadecimal literals, and single-character variables oh my!) hashing function I hadn't heard of, so I did what I get paid to do… I Google'd it, which led me to a write-up by the author of MurmurHash that discusses a flaw that can result in the algorithm having a significantly reduced potential output pool. This leads to increased collisions (“a better than 50% chance of finding a collision in a group of 13115”).

Okay, so under the assumption that this is the source of the class-name collision that we are experiencing, I went to try to pull some more details from the buggy page to see if I can create a reproduction, and that's when I discovered the registered and inserted lists in the Emotion Context, which I presume are handling collisions. And this is also when I notice that the div that is in my DOM with the incorrect class name is actually being passed the correct className prop, which is generated by a pretty vanilla css prop if I use React Devtools to inspect it. So the actual DOM node seems to disagree with React Devtools.

It gets weirder, though. I'm using Gatsby, and I noticed that the issue doesn't present itself if you navigate to the page from another page on the site, only if you load the page directly. I assumed this would be related to server-side rendering, so I checked the static HTML and the correct class and styles are used, so the incorrect class is being assigned as part the rehydration mount, not during SSR or during client-side render from raw data.

Short summary:

• A single Emotion-generated class name is being applied to multiple components that are not using the same styles
• The correct class name is passed to the React element, but the incorrect value is rendered to the DOM
• The issue appears to only present when rehydrating server-side rendered content
• I'm stumped, but anticipate someone with more knowledge of the Emotion internals can pinpoint the issue pretty quickly.
• 😬🙏

Environment information:

• react version: 16.8.6
• emotion version:
  
  
  
  • @emotion/[email protected]
  
  
  • @emotion/[email protected]
  
  
  • @emotion/[email protected]
  
  

[Andarist]

Thanks for the report and the initial investigation! It's really hard for me to pinpoint the exact problem without having a runnable repro case. Could you maybe try to distill down your project to just those 2 affected components to see if it still behaves the same way? Or could you maybe share access to your project with me?

[coreyward]

Yeah, I'll see if I can extract the relevant bits while retaining the behavior, but it's a bit of a tricky waltz of components and styles, so I'm not super confident it'll work.

[Andarist]

Well - maybe consider sharing access to your project with me, I can sign NDA or similar if that would be necessary.

[Andarist]

@coreyward have you been able to repro this in isolation?

[coreyward]

I haven’t had a chance to try to extract it out, honestly.
On Nov 3, 2019, 4:19 PM -0600, Mateusz Burzyński [e-mail], wrote:

@coreyward have you been able to repro this in isolation?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

[snirp]

Not sure if this is same exact error, but my Gatsby build also seems to have hash collisions between emotion classes. Specifically I defined an emotion component named Content in three different components.

I noticed that the wrong styles were being applied in the production builds. This was not an issue in development.

My code is on a private repo, but I think I can make a simple public proof of concept if that helps at all.

edit: the collisions are not limited to emotion components with the same name. Weird stuff happens all over.

[Andarist]

That would help tremendously.

[snirp]

Ok, hold on. For now it seems to make all the difference to wrap my page components in a <div> instead of a <> (React fragment).

[snirp]

Here is a proof of concept. It is derived from the starter template and wraps two page components inside a Layout component (in gatsby-browser.js).

https://github.com/snirp/emotion-issue-1552

[snirp]

Notice that the result is different for gatsby develop and gatsby serve. The develop build correctly applies the Red component in the layout and the Big component in each of the pages. The production build appears to have a collision, where the class of the Big component is applied instead of the Red component.

Hope this makes sense. Perhaps best observed by running both versions.

I created an index and page-2 page, to test my earlier observation that wrapping in a React Fragment could be a cause but this seems to make no difference.

[Andarist]

Thank you 🙏 I’ll try to look into as soon as I can

[Andarist]

Your issue is different - you have set up your Gatsby project wrong way. As per documentation - https://www.gatsbyjs.org/docs/api-files-gatsby-browser/ - when implementing gatsby-browser.js you most likely should also implement gatsby-ssr.js. You have to consistently export the same wrapPageElement from them.

I don't quite understand why you were not warned by React about SSR/client mismatch though - this is weird (?).

[snirp]

Ah ok, that could explain. Thanks for going above and beyond and helping me out and sorry for messing this issue up.

[Andarist]

No repro case was provided, so I'm going to close this one - but if you find the time to repro this I will gladly take a look at the problem.

[hawkett]

We've also encountered this problem in a large(ish) codebase. I haven't been able to reproduce in a small demo repository, but have some details of the implementation, and what we've done to fix.

We define a component that looks similar to this:

export const Glyph = styled(BaseGlyph)<{ name: string }>`
  &::after {
    content: '${props => getChar(props.name)}';
  }
`;

There's ~200 variations of name that produce different glyphs from a custom font (where characters like 'a', 'b' etc. render different glyphs) and name is a human readable form that maps to the font character in getChar. It is used like <Glyph name="MeaningfulSymbolName" />. BaseGlyph defines a few styles like font and color, but is not especially complicated.

The reason that we do it this way (rather than rendering the character directly) is that we previously did not have a font for these glyphs and rendered them as separate svg, so they had React components that matched the same component <Glyph name="..." /> signature. This method using css content allowed us to switch to an emotion-only font implementation without changing the component API. There are certainly other possible implementations that may not have resulted in a name collision.

The result of this implementation is needing ~200 different hashes for styles - each style string maybe one or two hundred characters long and of the same length, but varying by a single character.

We render many thousands of Glyphs in a single page, and the result of the issue is that one (or more) of the glyphs are consistently replaced by some other glyph because of the name collision. The easiest way to conceptualize the impact is to imagine we are rendering a long and complex page of mathematical formula, with the occasional symbol substituted for some other arbitrary symbol.

The issue was resolved in this instance by supplying a custom label name:

export const Glyph = styled(BaseGlyph)<{ name: string }>`
  &::after {
    content: '${props => getChar(props.name)}';
  }
  ${props => `label:Glyph-${props.name}`};
`;

this generated: css-ppg6yw-Glyph-SymbolName1 & css-c954va-Glyph-SymbolName2, whereas both were generated as css-112tzn2 previously. As a side note, maybe related, we do have autoLabel enabled, but for about 80% of our components the component name is not appended (as in this example).

The following also caused emotion to generate distinct class names:

export const Glyph = styled(BaseGlyph)<{ name: string }>`
  &::after {
    content: '${props => getChar(props.name)}';
  }
  /* ${props => props.name} */
`;

So just making the css string vary on name works in this particular instance, but I'm guessing that this just changes the potential for collision, whereas the label solution removes any possibility.

A couple of questions:

• I haven't had a chance to review whether Emotion does attempt to handle collisions as surmised by the OP - do you see this collision as a bug, or should the expectation be that css names might collide on occasion?
• If we should expect the occasional collision pretty much anywhere in our document, is it advised to ensure no collision by adding a custom label to every component, composing the values of each component parameter?

[Andarist]

I haven't had a chance to review whether Emotion does attempt to handle collisions as surmised by the OP - do you see this collision as a bug, or should the expectation be that css names might collide on occasion?

We don't handle collisions - this would slow the runtime. The expectation is that collisions should be extremely rare. This is how all hash-based CSS-in-JS solutions work (to the best of my knowledge).

If we should expect the occasional collision pretty much anywhere in our document, is it advised to ensure no collision by adding a custom label to every component, composing the values of each component parameter?

Hard to tell - I'm not sure if the collision chance is related to the input length. Theoretically, it may always happen for any 2 given strings.

I haven't been able to reproduce in a small demo repository

If you were able to pinpoint the problem to a particular component - wouldn't it be possible to just try to render all of its variations and check the generated hashes? Hashing algorithm is pure, for the same input it should yield the same output. This would help a lot in analyzing the problem further - if we could get exact inputs that have caused the problem.

we do have autoLabel enabled, but for about 80% of our components the component name is not appended (as in this example).

Could you create a repro case for this? Seems like something that we should fix.

The following also caused emotion to generate distinct class names:

Interesting - this probably should not work because it's put in a comment and our Babel plugin should just remove this comment. Are you sure you have configured Babel, right?

[hawkett]

re. demo repository, unfortunately I don't have time at the moment. Hopefully the information provided is more useful than not. It sounds like you're saying that collision handling won't be implemented in any case - what next steps do you see if two colliding strings were identified in an algorithm known to have collisions?

re. collision checking, it seems the consequences of a collision (css silently applied incorrectly), would be easily more important than any performance hit. P1: do it right, P2: do it fast. The argument that 'everyone implements it like that' doesn't seem convincing.

re. performance, apart from a map lookup, the collision handling code path should only be incurred if there is actually a collision, which (as you note) should be rare. At first glance it seems the performance hit would be minimal, but perhaps I have misunderstood the problem. Even a warning that a collision did occur would be valuable, regardless of whether it was actually resolved by Emotion. It could then be ignored or fixed (with a custom label) at the user's discretion. Warning for conflicts could be an optional configuration so there is no performance hit at all, turned on in development and off in production.

re. autoLabel demo - unfortunately not at the moment, but hopefully the information is of some value.

re. Babel - I'm using the Gatsby default config. Is there a specific configuration you think might be at fault here?

[coreyward]

We don't handle collisions - this would slow the runtime. The expectation is that collisions should be extremely rare. This is how all hash-based CSS-in-JS solutions work (to the best of my knowledge).

You have two people reporting collisions being detected and causing problems in production. The only reasonable takeaway seems to be that collisions are not sufficiently rare to rely on the current hashing algorithm. Perhaps a few more bytes on the hash would be sufficient, but as it ships today collisions are genuinely problematic.

This might be somewhat tolerable if Emotion were shipping a default hashing algorithm with clear instructions on overriding it. That is not the case. Users of the library are being asked to either tolerate significant bugs in production code or not use Emotion. That’s a really awful ultimatum.

[pawelngei]

Let's make that three people with collisions on prod.

[Andarist]

Those collisions should be stable (as the hashing algorithm is stable) - if you provide a runnable repro case for this I will try to evaluate the problem.

[pawelngei]

I can confirm that they are stable on a given commit, not affected by pre-hashed Emotion component names. I'll see what I can do for reproduction, since our code isn't open.