Per-component classnames

[geelen]

Super hacky proof of concept, but something's been bugging me for a while about Styled Components — we don't have any classnames that are always going to be attached on a component. This is the key problem with supporting #142 but it also causes a few DX annoyances, prevents an optimisation, and presents one nasty af potential bug that nobody has hit yet...

One annoyance is that, in dev tools, you don't have a single class hook to tweak settings if you have several instances of a component with different props. <Button primary> and <Button> will generate totally different classnames, so you can't jump in to dev tools and tweak all the buttons. If they share a classname, you can.

It also means that #160 is harder than it should be. If there's a unique class for every Styled Component then in dev you can make that whatever you like. 5 internet high fives for the person who implements BEM-style .app_components_Logo_Inner classnames using the directory structure 👍 , ping @markdalgleish.

The performance optimisation has to do with #134. If we had a single shared classname for all instances of a SC then we could potentially lift all the static styles into them. Then, when props change, we're injecting just the styles that depend on props. I'm not convinced this will be a massive gain but it certainly would map better to how people think about their styles.

And finally the nasty af bug. Consider this setup:

const A = styled.div`
  color: blue;
`

injectGlobal`
  /* contrived example, but all you need is a rule with equal specificity
     so source order becomes important */
  .override {
    color: red;
  }
`

const B = styled.div`
  color: ${props => props.selected ? 'blue' : 'green'};
`

<A className="override"/> // red, since .override appears later
<B className="override"/> // green, since it is rendered afterwards
<B selected className="override"/> // red, not blue !!

The problem arises from the internal cache about rendered styles. If two components end up producing the exact same CSS, we'll only inject a single class. Which means in cases where source-order is important (two rules applying with equal specificity) the result can be unpredictable. But, without a legitimate per-component identifier, the alternative (render the same selector twice) is actually kinda worse, since it would break components you're not looking at. In the above example, rendering a single <B selected> would change all the <A className="override"> on the page to blue!! 😱

So, it's something I've wanted for a while, but the problem is there's no deterministic information from which to derive a per-component identifier. This PR uses a simple counter, so the 5th time you call styled.div you'll get a classname based on a hash of div and 5 (which is .sc-ioLzpW). If you have two components with displayName = 'Inner', the first will get a classname of .Inner-jaWsLN and the second .Inner-dBMIyz. Which works really really well, replaces #160 and made #142 a one-line change 😎

Note the empty .Inner-jaWsLN tag in dev tools. Makes it really to tweak all instances at once 👍

The only issue is SSR, which we're about to land support for in #214. On the server, components for different pages might all be loaded, and so relying on order-of-instantiation will cause a checksum mismatch on the client. Not good.

So I've built this with a potential babel transform in mind, which would go through all StyledComponents instances and generates a deterministic identifier, much the way babel-plugin-styled-components-named generates displayNames. If that bundle is sent down to the client the counter won't be used and the checksum will match.

(Note by @mxstbr: a first experimental version of this babel transform exists now! See babel-plugin-styled-components)

I don't really have any time before CSS Conf AU next week to go any further with this, but as a proof of concept it really does prove the concept. We can bikeshed the generated classnames but I'm pretty convinced per-component classnames are the way to go. Anyone keen to take this and run with it?

[mxstbr-bot]

	Warnings
⚠️	❗ Big PR
⚠️	There are library changes, but not tests. That's OK as long as you're refactoring existing code

Generated by 🚫 dangerJS

[chiefjester]

@geelen couldn't we potentially create a unique identifier since we're creating a React Component anyways? (at StyledComponent.js) I'm thinking of adding uuid in the component creation instead of adding babel-transform after everything is rendered.

Nevermind babel-transform is for SSR.

Re: performance. A fast fix for the performance is the ephermal helper for now. But I do agree this is the better solution.

[geelen]

Using something like a UUID would be like Math.random though, right? Which would definitely not match between client and server, breaking the checksum stuff...

[mxstbr]

I've written a first babel plugin for this! https://github.com/styled-components/babel-plugin-styled-components

Go to test/fixtures/add-identifier to see what it does – essentially, it goes through and adds an incremental numeric .identifier to each styled component. This isn't perfect yet, but I think it's a good first start?

[mxstbr]

For some reason this is throwing lots of these errors: Error: Expected '.b { color: black; }' to equal '.a { color: black; }' where the class is just off-by-one. Is that a problem in the code or is that a sideeffect of the changes and the class names need to be updated?

[geelen]

Nah I didn't even run the tests while i was sketching this out, not surprised I broke everything :)

[mxstbr]

So I think the last thing for this to be ready is that extending/inheritance is completely broken for some reason unknown to me. The tests fail in very weird ways, not applying the classes how you'd expect them to. Any ideas what's up with that?

[mxstbr]

In more happy news, this is working perfectly with the babel plugin! 🎉

Code:

DevTools:

😍

[chiefjester]

@mxstbr babel-plugin = works with SSR?

[mxstbr]

Yes, and better debugging (because it adds the displayNames), but it's not mandatory at all

[clempat]

Do you mean by "not applying the classes how you'd expect them to" that the class name is always one extra time incremented.
I guess it is because the nameGenerator (const classNames = () => String.fromCodePoint(97 + index++)) passed to ComponentStyle is run twice.

First in StyledComponent.js line 115

Then in componentWillMount line 65

Or I misunderstand something.

And for the first extending error I guess it is because both are with the same identifier and as no style. they should have the same hash. So only one .a {} added which ended as .b{} because increment in willmount.

So It looks all normal...
Either avoiding regenerate the name multiple time (which should not be a problem using the generateAlphabeticName) or changing the predictable name generator for test ?

[mxstbr]

No, I'm aware of that issue and fixed all tests that broke because of that. The issue is with the extending.test.js file – if you run the tests on this branch, you'll see that they fail because the generated CSS is completely wrong! (not just the class names incremented by one)

[ticky]

Tasty, tasty sugar! 😃

[ticky]

This is really, really cool! I think it’d definitely fill the gap I’ve found which led to #291! 👍

[geelen]

Ok, in the interests of getting this shipped, I've pulled this back to a single extra method withConfig. No classes support for the moment (reverted in 5fd2de5) but displayName and componentId are in there. This isn't designed to be user-facing, but to be hooked in to by the babel transform:

const X = styled.div`
  ...
`

/* transformed to */
const X = styled.div.withConfig({ displayName: 'X', componentId: 'X_a1b2c3d4' })`
  ...
`

This PR can now be merged and will close #142 and #160 and enable deterministic SSR, so it'll be good to get this shipped. I had hoped to solve classes and extends with the same API but this takes the babel-targeted properties and removes them from the equation. Let's move discussion on the others to #291.

[chiefjester]

@geelen / @mxstbr do we need to change anything on this #214 in respect to this PR? If not, can we merged it? So that someone can add the rehydration part of SSR.

[geelen]

Ok, kicking of the v2 branch with this.