[styled] Specify emotion & styled-components as optional peer dependencies

[mnajdova]

• I have followed (at least) the PR section of the contributing guide.

[mnajdova]

Should we specify these as regular peer dependencies here and as optional peer dependencies only in the core? Same would apply for the @material-ui/styled-engine-sc

[mnajdova]

Moving conversation from #22788 (review) here

@oliviertassinari @eps1lon

[oliviertassinari]

No objection, no strong point of view either:

Pros:

• Avoid warning with styled-components (counter-argument: nobody looks at these warnings anymore, I don't.)

Cons:

• Force to run the code to realize you forgot to install the emotion peer dependency (counter-argument: nobody looks at these warnings anymore, I don't.)

[mnajdova]

Let's go with optional everywhere for now and see how it will go then?

[eps1lon]

(counter-argument: nobody looks at these warnings anymore, I don't.)

You're also part of the problem why "nobody" looks at these. It's pretty straight forward to fix them but many (especially library authors) just ignore them right from the start because "it works on my machine". Apart from hoisted peer dependencies I've never seen a peer dep warnings that's not an actual issue and just works with the exact same declared dependencies (which rarely happens because lib consumers don't pull in dev deps).

So peer dep warnings are important. It's ok to ignore them as an app dev but as a library author you actually ship problems to your users.

These are required peer deps as far as I can tell so they shouldn't be marked as optional. Hoisting them just isn't supported in yarn v1. A proper test to verify correctness of the tree would be installing with pnpm or yarn v2 (with and without PnP).

[mnajdova]

If developers have configured correctly styled-engine-sc in their bundler as alias, the warning are not meaningful. The question is then, should we try to fix these warnings, or should we consider that they should know that once that the alias is configured they no longer need emotion?

[oliviertassinari]

@eps1lon Agree, I think that npm warnings should be avoided as much as possible.

We haven't that many great options, the one I cant think of outside the changes proposed (1.):

2. We can make emotion a direct dependency. But then, this can lead to the duplicated version installed. This would be completely fine if it wasn't for the singletons. If we use our own ThemeProvider the singleton on the theme context is solved. However, I imagine would still have issues with the cache, and the CSS injection order.
3. We can make @material-ui/styled-engine a peer dependency. But it adds to the complexity of getting started.
4. We do nothing

Happy to try 1. out.

[mnajdova]

Apologizes I am a bit confused at this moment. What exactly is the plan? Should we add these as optional or not?

[oliviertassinari]

I vote for the options in this order of preference: 1 > 3 > 4 > 2. @eps1lon what would you recommend?

[eps1lon]

But then, this can lead to the duplicated version installed. This would be completely fine if it wasn't for the singletons.

I can not stress this enough: This would not be fine. It causes bundle bundle bloat.

I'll need to think about this a bit more and will talk to some package manager folks how they would recommend we move forward. We probably should need to mark the styled-engine as peer not the underlying style library. That's not that big of an issue since we already have to install peers anyway. And with npm 7 that won't be an issue at all anyway. This PR is fine for now.

[mui-pr-bot]

No bundle size changes comparing 1240860...d774066

Generated by 🚫 dangerJS against d774066