Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 10 commits into from Oct 5, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c2d31cc
add emotion peer dependencies
mnajdova Sep 25, 2020
5ae933f
fixed types & tests
mnajdova Sep 25, 2020
18b0668
prettier
mnajdova Sep 25, 2020
f0ef95c
Merge branch 'next' of https://github.com/mui-org/material-ui into next
mnajdova Sep 28, 2020
c7bebb8
Merge branch 'next' of https://github.com/mui-org/material-ui into next
mnajdova Sep 28, 2020
92b2d6e
Merge branch 'next' of https://github.com/mui-org/material-ui into next
mnajdova Sep 28, 2020
13da531
Merge branch 'next' of https://github.com/mui-org/material-ui into next
mnajdova Sep 29, 2020
cf5d9a5
Merge branch 'next' of https://github.com/mui-org/material-ui into next
mnajdova Sep 29, 2020
4729ff5
specify emotion & styled-components as optional peer dependencies
mnajdova Sep 29, 2020
d774066
Update packages/material-ui-styled-engine-sc/package.json
mnajdova Sep 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/material-ui-styled-engine/package.json
View file
Open in desktop
Expand Up @@ -41,6 +41,14 @@
"@emotion/core": "^10.0.27",
"@emotion/styled": "^10.0.27"
},
"peerDependenciesMeta": {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving conversation from #22788 (review) here

image

@oliviertassinari @eps1lon

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.):

  1. 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.
  2. We can make @material-ui/styled-engine a peer dependency. But it adds to the complexity of getting started.
  3. We do nothing

Happy to try 1. out.

Copy link
Member Author

@mnajdova mnajdova Sep 30, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"@emotion/core": {
"optional": true
},
"@emotion/styled": {
"optional": true
}
},
"sideEffects": false,
"publishConfig": {
"access": "public"
Expand Down
6 changes: 6 additions & 0 deletions packages/material-ui/package.json
View file
Open in desktop
Expand Up @@ -48,6 +48,12 @@
"peerDependenciesMeta": {
"@types/react": {
"optional": true
},
"@emotion/core": {
"optional": true
},
"@emotion/styled": {
"optional": true
}
},
"dependencies": {
Expand Down