[styles] Remove the old styles modules by oliviertassinari · Pull Request #14560 · mui/material-ui

oliviertassinari · 2019-02-16T21:12:48Z

moved to #14767.

The whole community is using hoist-non-react-statics, let's go for it. We should be able to remove the yarn version lock at some point. *This change is taken from #14560 as an effort not to merge too big pull requests.*

mui-pr-bot · 2019-03-05T23:34:39Z

@material-ui/core: parsed: -2.51% 😍, gzip: +0.12%
@material-ui/lab: parsed: -7.40% 😍, gzip: -1.07% 😍
@material-ui/styles: parsed: +2.57% , gzip: +2.73%

Details of bundle changes.

Comparing: 7a49f80...cea8cb8

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-2.51%	+0.12% 🔺	371,832	362,506	91,939	92,050
@material-ui/lab	-7.40%	-1.07%	184,281	170,653	50,584	50,044
@material-ui/styles	+2.57% 🔺	+2.73% 🔺	55,687	57,118	15,825	16,257
@material-ui/system	-0.12%	+0.36% 🔺	17,062	17,041	4,482	4,498
Button	-8.35%	+1.09% 🔺	99,409	91,106	26,607	26,898
Modal	-8.86%	+1.69% 🔺	98,984	90,213	26,263	26,707
Paper	+Infinity% 🔺	+Infinity% 🔺	0	63,293	0	19,264
Paper.cjs	+Infinity% 🔺	+Infinity% 🔺	0	68,801	0	20,153
Popper	+Infinity% 🔺	+Infinity% 🔺	0	30,456	0	10,534
colorManipulator	0.00%	+0.15% 🔺	3,232	3,232	1,296	1,298
createMuiTheme	+Infinity% 🔺	+Infinity% 🔺	0	17,284	0	5,700
docs.landing	0.00%	0.00%	51,891	51,891	11,291	11,291
docs.main	-7.81%	-6.40%	678,326	625,369	206,184	192,994
useMediaQuery	+Infinity% 🔺	+Infinity% 🔺	0	2,486	0	1,050
packages/material-ui/build/umd/material-ui.production.min.js	-3.53%	+0.16% 🔺	322,466	311,092	85,044	85,183
@material-ui/core/Paper	-100.00%	-100.00%	76,648	0	19,308	0
@material-ui/core/Paper.esm	-100.00%	-100.00%	71,599	0	18,783	0
@material-ui/core/Popper	-100.00%	-100.00%	30,462	0	10,582	0
@material-ui/core/styles/createMuiTheme	-100.00%	-100.00%	17,286	0	5,717	0
@material-ui/core/useMediaQuery	-100.00%	-100.00%	2,486	0	1,049	0

Generated by 🚫 dangerJS against cea8cb8

eps1lon

Main objection is about bundle names. Other things are mainly about clarifying the @material-ui/core/styles vs. @material-ui/styles difference

eps1lon · 2019-03-06T09:29:16Z

 import { create } from 'jss';
-import { withStyles, jssPreset } from '@material-ui/core/styles';
-import { StylesProvider } from '@material-ui/styles';
+import { withStyles } from '@material-ui/core/styles';


I thought the old module was removed and we use

- import { withStyles } from '@material-ui/core/styles'; + import { withStyles } from '@material-ui/styles';

You made me think that I have forgotten the hook demos. I have done the following change:

-import { makeStyles } from '@material-ui/styles'; +import { makeStyles } from '@material-ui/core/styles';

So people can copy & paste the demos directly.

eps1lon · 2019-03-06T09:33:06Z


  return (props = {}) => {
-    const theme = listenToTheme ? React.useContext(ThemeContext) || defaultTheme : defaultTheme;
+    const theme = (listenToTheme ? useTheme() : null) || defaultTheme;


I would go with the pattern suggested from the react issue and call a custom hook unconditionally that was created in the outer scope. That whole statement looks really scary.

eps1lon · 2019-03-06T09:38:08Z

    return <Component ref={innerRef || ref} classes={classes} {...more} />;
  });

+  if (process.env.NODE_ENV === 'test' && !ponyfillGlobal.disableShallowSupport) {


What's this for?

This removes the need for migrating all the tests away from the shallow API. We can consider killing this code branch once we have removed all the shallow tests :). It has a second advantage, it makes the class names deterministic in the test environment. Related to #14358.

it makes the class names deterministic in the test environment

That is a lot of redundant code for behavior that is only required for a single testing pattern that is already solvable. Emotion and styled-components agree with me here. I hope this gets removed once we figure out shallow testing.

Yes, I think that this whole code branch should get removed. Now the tradeoff was:
remove install() step > this extra logic, (> as more important)

eps1lon · 2019-03-06T09:43:40Z

-  withTheme,
-  WithTheme,
-} from './styles';
+export { createMuiTheme, Theme, withStyles, WithStyles, withTheme, WithTheme } from './styles';


This doesn't match packages/material-ui/src/index.js

eps1lon · 2019-03-06T09:44:24Z

-}
-
-export default ponyfillGlobal.__MUI_STYLES__.MuiThemeProvider;
+export default ThemeProvider;


If it still exists we should include the types.

It's only here until I figure out how to handle the CDN demo.

eps1lon · 2019-03-06T09:48:48Z

    {
      // vs https://bundlephobia.com/result?p=react-popper
-      name: '@material-ui/core/Popper',
+      name: 'Popper',


diffs are tracked via name. Should've named this id instead to make its purpose clear.

We can introduce a human label in another PR.

We shouldn't have any collision (or few exceptions) between the modules we are exporting. How is this an issue?

oliviertassinari added scope: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. breaking change Introduces changes that are not backward compatible. labels Feb 16, 2019

oliviertassinari changed the title ~~[styles] Remove the old styles modules~~ WIP [styles] Remove the old styles modules Feb 16, 2019

eps1lon mentioned this pull request Feb 16, 2019

RFC: Material-UI v4 breaking changes #13663

Closed

56 tasks

oliviertassinari force-pushed the remove-old-styles-modules branch 3 times, most recently from a4ef194 to 5375f96 Compare February 16, 2019 22:50

This was referenced Feb 17, 2019

[styles] Change the withTheme API #14565

Merged

[System] TypeError: Object(...) is not a function #14577

Closed

oliviertassinari self-assigned this Feb 19, 2019

This was referenced Feb 19, 2019

[styles] Warn if missing ThemeProvider #14581

Merged

[useMediaQuery] Remove unstable prefix #14593

Merged

v4: issue with @types/jss #14608

Closed

[styles] Upgrade JSS to 10.0.0-alpha.7 #14090

Merged

oliviertassinari force-pushed the remove-old-styles-modules branch from 5375f96 to be4b7c6 Compare February 22, 2019 17:21

oliviertassinari mentioned this pull request Feb 24, 2019

styles: BUG encountered - makeInstall and withStyles. #14645

Closed

oliviertassinari force-pushed the remove-old-styles-modules branch 3 times, most recently from 4d5fe17 to ceb8276 Compare February 27, 2019 15:24

oliviertassinari changed the title ~~WIP [styles] Remove the old styles modules~~ [styles] Remove the old styles modules Feb 27, 2019

oliviertassinari force-pushed the remove-old-styles-modules branch 9 times, most recently from 77971a7 to a82d8dc Compare February 27, 2019 23:38

oliviertassinari mentioned this pull request Mar 2, 2019

Improve test coverage #14708

Closed

oliviertassinari mentioned this pull request Mar 3, 2019

[core] Replace hoistMuiStatics usages #14722

Merged

oliviertassinari mentioned this pull request Mar 5, 2019

[styles] Improve ref forwarding #13676

Merged

oliviertassinari force-pushed the remove-old-styles-modules branch 3 times, most recently from fd6b76b to 0fe2fdf Compare March 5, 2019 23:30

oliviertassinari force-pushed the remove-old-styles-modules branch 3 times, most recently from 56e6093 to 2564e45 Compare March 6, 2019 00:39

[styles] Remove the old styles modules

08ac17f

oliviertassinari force-pushed the remove-old-styles-modules branch from 2564e45 to 08ac17f Compare March 6, 2019 00:48

oliviertassinari requested review from eps1lon and joshwooding March 6, 2019 01:01

oliviertassinari added the PR: good for merge label Mar 6, 2019

eps1lon requested changes Mar 6, 2019

View reviewed changes

oliviertassinari removed the PR: good for merge label Mar 6, 2019

reviews

cea8cb8

oliviertassinari force-pushed the remove-old-styles-modules branch from 1f2791f to cea8cb8 Compare March 6, 2019 12:43

oliviertassinari merged commit 22c8147 into mui:next Mar 6, 2019

oliviertassinari deleted the remove-old-styles-modules branch March 6, 2019 13:18

oliviertassinari removed breaking change Introduces changes that are not backward compatible. scope: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Mar 6, 2019

zannager added the scope: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. label Mar 2, 2023

Uh oh!

Uh oh!

Conversation

oliviertassinari commented Feb 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mui-pr-bot commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eps1lon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oliviertassinari commented Feb 16, 2019 •

edited

Loading

mui-pr-bot commented Mar 5, 2019 •

edited

Loading

oliviertassinari Mar 6, 2019 •

edited

Loading

oliviertassinari Mar 6, 2019 •

edited

Loading