Allow specifying the target element for appending style tags

[bteng22]

@mxstbr As requested here's the PR. Luckily there weren't many merge conflicts to resolve and all the tests @paul-veevers wrote are passing! My team and I also tested it locally on our application and it looks like it helped solved the rehydration problem caused by multiple styled-components that others also voiced in the issue.

I don't know if I have permissions to link this PR to #1102 since it'll essentially be a duplicate, so let know if there's any due diligence needed on my end.

Thanks again for all the help 😄

[mxstbr-bot]

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

Generated by 🚫 dangerJS

[mxstbr]

CI is failing?

[bteng22]

Looking it into it now! For some reason the native tests on my machine can't run due to this Cannot find module 'ReactElementType' from 'ReactRef.js'. May be a problem with babel-plugin-flow-react-proptypes and react-native, but haven't drilled down to it.

But since my native tests failed, it skipped the bundlesize tests. Working on a fix for that now

[bteng22]

@mxstbr Is there a protocol we should follow when deciding when to raise the bundlesize threshold? Looks like the CI is failing from the bundlesize tests by 0.06KB FAIL ./dist/styled-components.min.js: 14.56kB > maxSize 14.5kB gzip

Going to take a stab at minimizing it to get it passing, but just wanted to keep you in the loop and see if there were any alternatives? Tagging @siddharthkp as this may be related?

[mxstbr]

You can increase the max size in package.json, it's kind of annoying that this makes it so much bigger though 😢

[siddharthkp]

@bteng22 I've been in this situation 😄

@mxstbr Ideally you want to keep the threshold a good 2-3kB more than the current size (which is 14.49kB right now) so that you can catch mistakes. Keeping it too close to the current size sounds like micromanaging 🙂

This PR does not contribute a lot to the size, there were couple of pull requests in the past that made a big impact

I sound like a broken record but add the github token so that you can see the difference in each pull request (or give me access for a few hours and i'll do it for you!)

[bteng22]

Thanks for the input @mxstbr & @siddharthkp 👍 I raised the threshold to 14.7kB. It's a little more on the conservative side and will be about 1.4kB more than the current thresholds in the tests. Looks like that was enough to get the checks to pass 🎆

[mxstbr]

Looks awesome, thanks so much @bteng22! One point of discussion is, do we want to rename the StyleSheetManager to just StyleSheet?

<StyleSheetManager target={document.getElementById()}>

</StyleSheetManager>

// vs.

<StyleSheet target={document.getElementById()}>

</StyleSheet>

The second one seems more obvious to users, though it might be confusing since we also have ServerStyleSheet. How do we make the difference between those clear, and also what happens when you try to server-side render an app that has a StyleSheet? Users would have to use refs, right?

Also, what happens when you nest two StyleSheetManagers? Which one gets applied? Can we add some tests for all of this behavior?

This also needs a bunch of documentation over at https://github.com/styled-components/styled-components-website, mind submitting a PR there?

[bteng22]

No problem @mxstbr. Just trying to get this in prod ASAP 😄

Hmm maybe StyleSheetProvider? It looks like it's essentially providing a StyleSheet instance to StyledComponents through it's context, whether explicitly through a sheet prop or implicitly from the given target. I'm game for either suggestion, but changing the name would be a breaking change for any teams using the StyleSheetManager. Is this something we want to address now or can we shelve it for later?

In regards to your other questions, we're going to investigate a little further, but I just wanted to clarify a couple questions:

*When server-side rendering with an app that has a StyleSheet, should we prevent the use of the target prop since we do not have a DOM? or are you implying there is a way to get a pointer to a ref of one of the components?

My team and I tested it locally by keeping the SSR rendering the same as the documentation with a ServerStyleSheet instance and then on the client-side we wrapped our application with a StyleSheetManager with a specific target prop pointing to our DOM element. This approach seemed to be working well so far.

Also, is there a better channel we could reach you or any of the moderators? We might have some quick questions that could be better clarified if we PM'd you directly?

[mxstbr]

but changing the name would be a breaking change for any teams using the StyleSheetManager

Wait, what would people be using it for right now? Why do we export that?!

I tested it locally by keeping the SSR rendering the same as the documentation with a ServerStyleSheet instance and then on the client-side we wrapped our application with a StyleSheetManager with a specific target prop pointing to our DOM element.

The issue is that that only works for top-level StyleSheetManagers, what happens when one uses a StyleSheetManager far down the component tree? That'll just throw an error, maybe not if you use refs?

Also, is there a better channel we could reach you or any of the moderators?

https://spectrum.chat/styled-components

[kitten]

@mxstbr

Wait, what would people be using it for right now? Why do we export that?!

It can be used when collectStyles is a bad option, i.e. to clean up code in SSR. It shouldn't be a major problem to rename though.

I would hold off from renaming it to just StyleSheet however, as the confusion is simply to great when a component acts as a stylesheet while merely providing one.

[mxstbr]

Sounds good to me, let's leave it as-is then.

[bteng22]

Some things to note:

• Opened a PR for documentation here Documentation for appending styles to target styled-components-website#178
• The proposed documentation also potentially outlines how to handle SSRing with targeted styles. In short, the implementation is the same as the Server Side Rendering section, but we just need to inject the CSS under the specific target on the markup.
• We pushed a commit with a couple tests for multiple instances of StyleSheetManager when they're targeting the same DOM element and for when they're nested. Let me know if they're valuable or need to be reworked
• Still investigating your point regarding deeply nested StyleSheetManagers. At the moment, it looks like any StyledComponents outside the manager will get set on the document.head, while everything inside the manager will get applied to the style tag underneath the target if specified

[bteng22]

Here's an example we got working of nested StyleSheetManagers:

<StyleSheetManager> // passing in ServerStyleSheet on server-side and target on client-side
  <StyledCompA/>
  <Frame>
    <div ref={(el) => {this.setState({ targetRef: el })}}> 
      {this.state.targetRef && <StyleSheetManager target={this.state.targetRef}>
        <StyledCompB />
      </StyleSheetManager>}
    </div>
  </Frame>
</StyleSheetManager>

To your point @mxstbr , users would have to use refs (specifically on an HTMLElement and not a React Element), for the StyleSheetManager to access the given target. Unfortunately, this doesn't lend itself to collecting styles on the server-side and will only generate them on the client-side during runtime (because the ref callback occurs on mount).

Some other scenarios we explored with our app running its own instance of styled components:

• running another application with it's own instance of styled-components rendered through an iframe
• running another application rendered on the same page (as a sibling) with separate styled-components instance and a target. In our case, a separate application rendering a header and the content body of a page

In summary, using targets seems to work easily for pulling in external apps that are also using styled-components with a target, but requires a bit more work on the client-side if one is nesting StyleSheetManagers within a single application.

[mxstbr]

But the problem is that main app now stops loading other styled components properly after that external widget loads its own

That sounds like a bug, any ideas what's up with that?

[Fer0x]

Also keyframes doesn't work when used inside <StyleSheetManager target={someIframe} />.

[bteng22]

@ivanakimov hmm definitely sounds like a bug—are you using the same StyledComponents both inside and outside of your target container? If you could code up a small example on a gist to reproduce I could take a better look

@Fer0x I believe keyframes are global and as a result won't be appended to the target container within StyleSheetManager at least within this PR. Maybe we can open another issue to tackle that?

[jd327]

@bteng22 yes I've tried the same StyledComponents for both repos (and even using StyleSheetManager with a target for the main app) -- still definitely an issue with the main component not loading styles after another app loads its own.

[kitten]

@ivanakimov so, that sounds like either something’s wrong with this branch, or sth is wrong with your setup and you’re using two packages, maybe 😬

Can you make sure that your project is using a single version of styled-components? (Webpack can help ensure this)

And would you have time to help us and the PR submitter @bteng22 with a reproduction please? :)

[jd327]

@philpl I was using the same tgz for both repos when doing the tests. And yep, this issue is definitely a blocker for us, so I'm going to be diving in deeper to see if I can either setup a basic example or see what the problem is.

[kitten]

@ivanakimov could it be that it was installed in both repos but isn’t deduced? If those are sufficiently separate, the npm resolution algorithm will search the closest node_modules folder upwards, so if you install it in both they might still be duplicated even if they’re identical.

It unfortunately depends a lot on the folder structure and build pipeline :/ a quick fix if they’re nested in each other would be to use peer dependency in the lower project

[jd327]

@philpl hmm, well they're completely different repos in different folders with their own node_modules (and the repos are not nested). but i'll try to rm node_modules and do a fresh install and see if that helps.

[jd327]

Well, reinstalling node_modules didn't help, and I made sure both projects use the same version.

I've spent 2 hours today trying to build a basic example to reproduce the bug, but no luck. I've tried to recreate our conditions (with react router, with blueprintjs, with injectGlobal, with the same class names, with loading components dynamically, with separating into 2 separate apps), but can't trigger the issue.

I can clearly see it happening on our own project and the fact that styles do not load anymore right after loading the external widget, but the two repos are so big, I am not sure how else to try and trigger it in a reproducible way.

On the other hand, I guess this is good news for the PR... If there's any other suggestions on what I could try, I'm all ears.

[nmackey]

I know I'm a little late to the game but couldn't we use this support both an insertion point in the head and specifying a target element for appending style tags? JSS supports both by allowing the user to specify a string or an element. If it is a string then it looks for a comment inside the head otherwise if it is an element it uses that. - https://github.com/cssinjs/jss/blob/8252b25dfa7ebdb9ab9112c6760279cd79e742d2/src/renderers/DomRenderer.js

[rpellerin]

Any updates on this?

[tomaswitek]

Hi guys, we've run into the same issue having multiple CRA apps on one page.
What's the status here?
It looks like there have been already 2 attemps to solve it:

1. Allow specifying the target element for appending style tags #1102
2. this PR Allow specifying the target element for appending style tags #1324

How far is this one?
Can we help somehow @bteng22 @mxstbr ?

[petermikitsh]

Hello, I wanted to comment and say this PR is critical for web applications that are developed by distributed teams, that are sharing the DOM, but are using multiple instances of styled-components.

In an enterprise environment such as this, I have experienced collisions / missing style rules that break the way our application renders. Having a singleton / global space where styled-components mutates the singleton style node creates some problems in distributed environments with multiple instances.

Being able to target specific elements would fix this problem. Thanks.

[AndersGerner]

Has anybody found any "quick fix" / "Hacks" for fixing this ?
We have 2 big client project shipping next week, and we just encounted this issue as well. We are create single react Components as webparts in Sharepoint (Not custom webparts, just showed in a content-editor) and we have the exact same problem.

Right now we are trying to use: http://cssinjs.org/styled-jss/ in order to solve this.
We have previously seen that Material-UI has the same limit, that it can only be instanced once in a site, otherwise it might have some weird quirks.
The JSS-Insertion-Point solved that for us back then. It just seems like a BIG task.

[hco]

@AndersGerner We are currently injecting StyledComponents from one place to windows.styled and using it from there everywhere else with a little help from webpack Externals. Maybe that helps you.

[AndersGerner]

@hco sounds interesting and like a solution that might work for us. Thanks for letting me know!
Can you share a bit more about this “little help” from webpack externals? I have just switch from Gulp to Webpack, so aren’t an expert.. yet ;)
Maybe you can share some of your webpack configuration so I can understand it better? :)

[marionebl]

I am interested in this feature to land and would like to help this along.

Reading the comments so far I am not sure I understand what prevents this PR from landing (apart from the merge conflicts).

Could the maintainers chime in on this? @mxstbr @geelen

[roflc0pter]

Pleaaaase merge this!

[mxstbr]

Haven't really looked at this patch too much to be honest since I don't directly need it in production right now.

A thorough code review and test in production would be appreciated and would help us ship this

[marionebl]

Picking this up, will work on the following topics:

1. Solve merge conflicts
2. Make the test suite pass
3. Do a scoped publish for real-world testing on some projects

Work happends in #1491

[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!

[kitten]

@bteng22 Hiya, thanks to you as well! The changes have been merged with modifications by @marionebl in #1491