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

[styles] Change material-ui/styles folder structure #14868

Merged
merged 4 commits into from
Mar 13, 2019
Merged

[styles] Change material-ui/styles folder structure #14868

merged 4 commits into from
Mar 13, 2019

Conversation

VincentLanglet
Copy link
Contributor

@oliviertassinari oliviertassinari added new feature New feature or request package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Mar 13, 2019
@oliviertassinari oliviertassinari changed the title Change material-ui/styles folder structure [styles] Change material-ui/styles folder structure Mar 13, 2019
oliviertassinari
oliviertassinari reviewed Mar 13, 2019
View reviewed changes
packages/material-ui-styles/src/ThemeProvider/ThemeProvider.js
@@ -2,8 +2,8 @@ import React from 'react';
import PropTypes from 'prop-types';
import warning from 'warning';
import { exactProp } from '@material-ui/utils';
import ThemeContext from './ThemeContext';
import useTheme from './useTheme';
import ThemeContext from '../useTheme/ThemeContext';
Copy link
Member

@oliviertassinari oliviertassinari Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's the most controversial change I can find in the pull request. I do prefer hosting ThemeContext in the consumer than in the provider. People might not use the provider, but will always use the consumer. So, why not 👍.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to make a choice. I decided this way.

ThemeProvider was already importing useTheme
But useTheme wasn't importaing ThemeProvider
So I wanted to avoid to create something similar to a circular reference

@mui-pr-bot
Copy link

mui-pr-bot commented Mar 13, 2019 •
edited
Loading

@material-ui/core: parsed: -0.45% 😍, gzip: -0.40% 😍
@material-ui/lab: parsed: -0.23% 😍, gzip: -0.50% 😍
@material-ui/styles: parsed: -6.10% 😍, gzip: -4.48% 😍

Details of bundle changes.

Comparing: 2f6a982...23667ff

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.45% -0.40% 362,768 361,144 92,403 92,032
@material-ui/core/Paper -0.50% -0.97% 68,878 68,536 20,172 19,977
@material-ui/core/Paper.esm -2.55% -1.94% 63,340 61,725 19,264 18,890
@material-ui/core/Popper 0.00% +0.04% 🔺 30,458 30,458 10,566 10,570
@material-ui/core/styles/createMuiTheme 0.00% -0.05% 17,284 17,284 5,703 5,700
@material-ui/core/useMediaQuery 0.00% +0.19% 🔺 2,471 2,471 1,036 1,038
@material-ui/lab -0.23% -0.50% 170,726 170,337 50,110 49,859
@material-ui/styles -6.10% -4.48% 57,220 53,729 16,216 15,489
@material-ui/system 0.00% -0.02% 17,041 17,041 4,489 4,488
Button -1.77% -1.37% 91,218 89,602 26,957 26,587
Modal -1.79% -1.50% 90,263 88,646 26,728 26,327
colorManipulator 0.00% 0.00% 3,232 3,232 1,298 1,298
docs.landing 0.00% 0.00% 51,908 51,908 11,302 11,302
docs.main +0.47% 🔺 +0.28% 🔺 648,034 651,057 201,660 202,216
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 311,223 311,223 85,394 85,394

Generated by 🚫 dangerJS against 23667ff

@oliviertassinari
Copy link
Member

Wait what? 😆

Capture d’écran 2019-03-13 à 13 30 59

@VincentLanglet VincentLanglet mentioned this pull request Mar 13, 2019
Closed
2 tasks
@VincentLanglet
Copy link
Contributor Author

Will have conflict with #14856

@VincentLanglet VincentLanglet mentioned this pull request Mar 13, 2019
Merged
1 task
eps1lon
eps1lon approved these changes Mar 13, 2019
View reviewed changes
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Webpack didn't pick up the esm build before. Now it does which is why it displays these savings.

It's an issue with the webpack config we use for the snapshots. We have to target the cjs barrel1 which previously pointed directly to the cjs files. Now it points to the sub modules and webpack can work its magic. It can now pick up the package.json for every sub module and chooses the esm files.

The savings for @material-ui/styles are only virtual. Not quite sure why the other packages display the savings. It's very hard to debug with path imports.

@oliviertassinari oliviertassinari merged commit bdb4baa into mui:next Mar 13, 2019
@oliviertassinari
Copy link
Member

I'm curious to see the impact on this tool https://bundlephobia.com/result?p=@material-ui/styles.

@VincentLanglet Well done :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

@eps1lon eps1lon eps1lon approved these changes

Assignees
No one assigned
Labels
new feature New feature or request package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@VincentLanglet @mui-pr-bot @oliviertassinari @eps1lon