Skip to content

[docs] Add emotion dependencies in codesandbox examples #22736

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

Merged
merged 3 commits into from
Sep 26, 2020
Merged

[docs] Add emotion dependencies in codesandbox examples #22736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 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
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion docs/src/modules/utils/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ function pageToTitleI18n(page, t) {
* set of packages that ship their own typings instead of using @types/ namespace
* Array because Set([iterable]) is not supported in IE11
*/
const packagesWithBundledTypes = ['date-fns'];
const packagesWithBundledTypes = ['date-fns', '@emotion/core', '@emotion/styled'];

/**
* WARNING: Always uses `latest` typings.
Expand Down Expand Up @@ -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'],
Copy link
Member

@oliviertassinari oliviertassinari Sep 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@Andarist Andarist Sep 26, 2020

Choose a reason for hiding this comment

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

Styled pkg doesnt create any singletones - it only consumes them.

Copy link
Member

@eps1lon eps1lon Sep 26, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member

@oliviertassinari oliviertassinari Sep 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@oliviertassinari oliviertassinari Sep 26, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member

@eps1lon eps1lon Sep 27, 2020

Choose a reason for hiding this comment

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

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

});

if (
Expand Down Expand Up @@ -140,6 +142,8 @@ function getDependencies(raw, options = {}) {
'@material-ui/system': getMuiPackageVersion('system', muiCommitRef),
'@material-ui/utils': getMuiPackageVersion('utils', muiCommitRef),
'@material-ui/pickers': 'next',
'@emotion/core': 'latest',
'@emotion/styled': 'latest',
};

const re = /^import\s'([^']+)'|import\s[\s\S]*?\sfrom\s+'([^']+)/gm;
Expand Down
18 changes: 18 additions & 0 deletions docs/src/modules/utils/helpers.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ const styles = theme => ({

it('should handle @ dependencies', () => {
expect(getDependencies(s1)).to.deep.equal({
'@emotion/core': 'latest',
'@emotion/styled': 'latest',
'@foo-bar/bip': 'latest',
'@material-ui/core': 'next',
'prop-types': 'latest',
Expand All @@ -46,6 +48,8 @@ const suggestions = [
`;

expect(getDependencies(source)).to.deep.equal({
'@emotion/core': 'latest',
'@emotion/styled': 'latest',
'@material-ui/core': 'next',
'@unexisting/thing': 'latest',
'autosuggest-highlight': 'latest',
Expand All @@ -58,6 +62,8 @@ const suggestions = [

it('should support next dependencies', () => {
expect(getDependencies(s1, { reactVersion: 'next' })).to.deep.equal({
'@emotion/core': 'latest',
'@emotion/styled': 'latest',
'@foo-bar/bip': 'latest',
'@material-ui/core': 'next',
'prop-types': 'latest',
Expand All @@ -78,6 +84,8 @@ import { LocalizationProvider as MuiPickersLocalizationProvider, KeyboardTimePic

expect(getDependencies(source)).to.deep.equal({
'date-fns': 'latest',
'@emotion/core': 'latest',
'@emotion/styled': 'latest',
'@material-ui/pickers': 'next',
'@material-ui/core': 'next',
'prop-types': 'latest',
Expand All @@ -88,6 +96,8 @@ import { LocalizationProvider as MuiPickersLocalizationProvider, KeyboardTimePic

it('can collect required @types packages', () => {
expect(getDependencies(s1, { codeLanguage: 'TS' })).to.deep.equal({
'@emotion/core': 'latest',
'@emotion/styled': 'latest',
'@foo-bar/bip': 'latest',
'@material-ui/core': 'next',
'prop-types': 'latest',
Expand All @@ -114,6 +124,8 @@ import {

expect(getDependencies(source)).to.deep.equal({
'date-fns': 'latest',
'@emotion/core': 'latest',
'@emotion/styled': 'latest',
'@material-ui/core': 'next',
'@material-ui/pickers': 'next',
react: 'latest',
Expand All @@ -127,6 +139,8 @@ import lab from '@material-ui/lab';
`;

expect(getDependencies(source)).to.deep.equal({
'@emotion/core': 'latest',
'@emotion/styled': 'latest',
'@material-ui/core': 'next',
'@material-ui/lab': 'next',
react: 'latest',
Expand All @@ -142,6 +156,8 @@ import { useDemoData } from '@material-ui/x-grid-data-generator';
`;

expect(getDependencies(source, { codeLanguage: 'TS' })).to.deep.equal({
'@emotion/core': 'latest',
'@emotion/styled': 'latest',
'@material-ui/core': 'next',
'@material-ui/lab': 'next',
'@material-ui/icons': 'next',
Expand Down Expand Up @@ -170,6 +186,8 @@ import * as Utils from '@material-ui/utils';
).to.deep.equal({
react: 'latest',
'react-dom': 'latest',
'@emotion/core': 'latest',
'@emotion/styled': 'latest',
'@material-ui/core':
'https://pkg.csb.dev/mui-org/material-ui/commit/2d0e8b4d/@material-ui/core',
'@material-ui/icons':
Expand Down