[docs] Add emotion dependencies in codesandbox examples #22736
Conversation
mnajdova
commented
Sep 25, 2020
mnajdova
commented
- I have followed (at least) the PR section of the contributing guide.
|
No bundle size changes comparing 2596203...18b0668 |
|
@mnajdova Tests are failing (which is intended). |
|
@eps1lon fixed :) |
| @@ -77,6 +77,8 @@ function includePeerDependencies(deps, versions) { | |||
| Object.assign(deps, { | |||
| 'react-dom': versions['react-dom'], | |||
| react: versions.react, | |||
| '@emotion/core': versions['@emotion/core'], | |||
| '@emotion/styled': versions['@emotion/styled'], | |||
There was a problem hiding this comment.
I would like to follow-up on #22435 (comment).
@Andarist Does @emotion/styled have any singleton? Would it be safe if we make it a dependency of @material-ui/style-engine? We don't depend on the theme provider of emotion/sc, we use our own. My hope is that we can only retain @emotion/core as a peer dependency.
There was a problem hiding this comment.
Styled pkg doesnt create any singletones - it only consumes them.
There was a problem hiding this comment.
Even if it doesn't have a singleton we should always be aware of potential duplicate bundling. Style engines shouldn't be direct dependencies if we want users to customize it. Otherwise they might use a different version than we do.
There was a problem hiding this comment.
Otherwise they might use a different version than we do.
True, version can be different. Would it lead to issues?
Maybe we should be greedy like we were with @material-ui/style-engine (using a dependency over a peer dependency because simpler but makes using styled-components harder)? We could start with the approach that can yield the simplest installation steps (@emotion/styled as a dependency) and based on the pain we hear from developers change the tradeoff.
There was a problem hiding this comment.
Another element, it seems that in the demos, we will do:
import styled from '@material-ui/core/styled';or something similar so we can benefit from auto-switch between em/sc, the theme provider of MUI, and the default theme's values.
There was a problem hiding this comment.
True, version can be different. Would it lead to issues?
It would lead to multiple versions in the bundle:
Even if it doesn't have a singleton we should always be aware of potential duplicate bundling
|
I'm merging so we can include it in the incoming release:
|
