Fix ComponentStyle caching strategy to take StyleSheet cache into account

[darthtrevino]

Fixes #1634, Whitespace-change free version of PR #1635

When ComponentStyle.generateAndInjectStyles is invoked, the stylesheet instance should be checked even if the style name is already defined. If the component is being rendered in a sibling iframe with a separate StyleSheetManager, this allows the second iframe to receive the injected styles.

[kitten]

@darthtrevino Changing the StyleSheet while the component is mounted — essentially hot swapping the sheet — is not something we should support, just to be clear again. However, this is a reasonably small change that I can maybe accept, but we'll have to regression test it before release. Hence a small disclaimer: Sorry in advance if this change doesn't land 😉

The current state of the code breaks an important caching optimisation, as it runs a bunch of operations that we're trying to avoid just to fix this. I'll leave some comments on the code itself since there's another way.

[darthtrevino]

I don't believe this will hot-swap stylesheets for mounted and styled components. This is kind of analogous to a Java thread-local - it's two components of the same type in totally different stylesheet contexts.

[kitten]

This test is extremely specific to your environment and additionally this conflicts with it as well:

Changing the StyleSheet while the component is mounted — essentially hot swapping the sheet — is not something we should support,

Also it's a little unnecessarily complex. Instead we can use enzyme to swap out the the stylesheet itself together with a prop, and expect to see a different class.

Edit: I'd say it's ok to not test this for now. I can write a test eventually, but one that is specific to all caching guarantees instead

[kitten]

Here you're leading the code into expensive operations that we're trying to avoid

[kitten]

This goes through all operations even if it's not necessary too

[kitten]

This is the crux of it, so instead you can replace this:

if (areStylesCacheable && isStatic && lastClassName !== undefined) {

with this:

if (areStylesCacheable && isStatic && lastClassName !== undefined && styleSheet.hasNameForId(componentId, lastClassName)) {

a lot simpler, doesn't break our caching and optimisation, and it should have the same effect 👍

so essentially this is the only change that is needed

[darthtrevino]

I'm guessing the flatten() is what you're trying to avoid?

[kitten]

@darthtrevino

I don't believe this will hot-swap stylesheets for mounted and styled components. This is kind of analogous to a Java thread-local - it's two components of the same type in totally different stylesheet contexts.

No, this code change won't. Essentially you're just invalidating the cache for a new stylesheet. Whether that's hot swapped through the context, or because you're suddenly in a new context with the same ComponentStyle doesn't matter that much, because it won't occur during normal usage.

I think it's a fair addition though, so I've left some comments above and I think we can ship this

Edit:

I'm guessing the flatten() is what you're trying to avoid?

We're avoiding flatten and stringifyRules. generateRuleHash(this.componentId + flatCSS.join('')) is also quite expensive but not as bad. styleSheet.hasNameForId(componentId, name) is cheap so since that suffices in this case, that'd be a super elegant change

Edit: also, sorry to make you jump through a couple of hoops and so on, but I'm really — still — missing a lot of context of what you're trying to achieve and are seeing as a result 😉 apologies if I come across a little rude due to that. Anyway, the change seems cool and kind of explains some changing assumption in your env compare to a typical one 👍

[darthtrevino]

No problem, I get that it's a weird use case. I put some additional context at the tail of the last PR. I guess I would call this "iframes for css-isolation in plugin-style applications"

[kitten]

@darthtrevino just seen your last changes. Haven’t given it a quick run yet but see you sure that the change I proposed here won’t be enough? #1636 (comment)

Because essentially we only have to prevent returning the lastClassName if it’s not on the StyleSheet so that one line change should be enough

[darthtrevino]

@kitten Switching to your suggestion - I think that will be fine

[kitten]

Can we make this generic as well like the test?

I.e. fix last className caching strategy to take StyleSheet cache into account

Or sth

[kitten]

@darthtrevino thanks for making this change this quickly 👏 hope it will help more ppl with similar edge cases.

I’ll give it a couple of tests tomorrow and will push a prerelease. If that looks fine I’ll publish a patch :)

[bliitzkrieg]

This fix should resolve a similar issue I am having with a different use case. I created an embedded component system that mounts a react component (bundled with styled components) on html markup on any given client. The components wrap a StyleSheetManager that use the element its mounting as the target and when I programically unmount/mount the component for advanced usage, the styles disappear.

[kitten]

@bliitzkrieg yes, that might be fixed as well by this. Quick note that what you're doing will break specificity and certain guarantees (see previous comments) 😉

[mxstbr]

Thank you so much for helping us improve styled-components! Based on our Community Guidelines every person that has a PR of any kind merged is offered an invitation to the styled-components organization—that is you right now!

Should you accept, you'll get write access to the main repository. (and a fancy styled-components logo on your GitHub profile!) You'll be able to label and close issues, merge pull requests, create new branches, etc. We want you to help us out with those things, so don't be scared to do them! Make sure to read our contribution and community guidelines, which explains all of this in more detail. Of course, if you have any questions just let me know!

[darthtrevino]

Sounds great - thanks Max!