mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.64k stars 32.22k forks source link

@material-ui/styles vs @material-ui/core/styles #17854

Closed mwskwong closed 4 years ago

mwskwong commented 5 years ago

Is it more preferable to use @material-ui/core/styles instead of @material-ui/styles if material-ui is used?

Also according to the Minimizing Bundle Size Guide, anything below 2nd level import is considered private and can cause module duplication in your bundle. So should I import makeStyles like this to minimize bundle size?

import makeStyles from '@material-ui/core/styles/makeStyles';
oliviertassinari commented 5 years ago

If you follow the documentation, you should be fine. To answer your question, in your case, the best approach is:

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

(the example you have shared imports 3 levels deep, it's not OK)

Hum, in the documentation we say:

Anything below is considered private and can cause module duplication in your bundle.

It seems that it has confused you. Below is used with the "deeper" semantic. I think that we should update the wording.

kurtwilbies commented 5 years ago

The docs are very confusing. What are the best practices?

oliviertassinari commented 5 years ago

@kurtwilbies what do you mean?

oliviertassinari commented 5 years ago

@matthewkwong2 What do you think of this diff?

diff --git a/docs/src/pages/guides/minimizing-bundle-size/minimizing-bundle-size.md b/docs/src/pages/guides/minimizing-bundle-size/minimizing-bundle-size.md
index 0cf8c2443..c615f5586 100644
--- a/docs/src/pages/guides/minimizing-bundle-size/minimizing-bundle-size.md
+++ b/docs/src/pages/guides/minimizing-bundle-size/minimizing-bundle-size.md
@@ -49,8 +49,8 @@ Head to [Option 2](#option-2) for the approach that yields the best DX and UX.

 While importing directly in this manner doesn't use the exports in [`@material-ui/core/index.js`](https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/index.js), this file can serve as a handy reference as to which modules are public.

-Be aware that we only support first and second level imports. Anything below is considered
-private and can cause module duplication in your bundle.
+Be aware that we only support first and second level imports. Anything deeper is considered
+private and can cause issues, like module duplication in your bundle.

Would it remove part of the confusion?

kurtwilbies commented 5 years ago

I meant use examples, because this issue depends on a deep knowledge of the source code.

coglite commented 5 years ago

best solution would be to remove material ui styles all together , react-jss 10 is released

oliviertassinari commented 5 years ago

@Coglite #17391

shiraze commented 5 years ago

I had to change to change all my references from: import MuiThemeProvider from "@material-ui/core/styles/MuiThemeProvider"; to: import { MuiThemeProvider } from "@material-ui/core/styles";

when upgrading from @material-ui/core@4.5.0 to @material-ui/core@4.5.1, otherwise the MuiThemeProvide module could not be found.

oliviertassinari commented 5 years ago

@shiraze Thanks for the feedback! This looks expected, imports deeper than 2 levels deep are private. @material-ui/core/styles/MuiThemeProvider is a private module.

petersendidit commented 5 years ago

Might be good to create a codemod that could be run to catch and fix improper imports.

oliviertassinari commented 5 years ago

There is a codemod and and eslint rule https://github.com/mui-org/material-ui/tree/master/packages/eslint-plugin-material-ui#restricted-path-imports, https://github.com/mui-org/material-ui/tree/master/packages/material-ui-codemod#optimal-imports.

pschlette commented 5 years ago

Is it problematic to import types from deep import paths? I'd like to remove my explicit dependency on @material-ui/styles, but I'm not sure where to import e.g. the CSSProperties type from, if not @material-ui/core/styles/withStyles.

eps1lon commented 5 years ago

@pschlette Could you explain what you need this type for? I'd hope everything is covered by *Styles.

josephstgh commented 5 years ago

Hi, I have the following.

import { CSSProperties } from '@material-ui/styles`

interface Common {
  appbarSpace: CSSProperties;
}

const getAppStyle = (theme: Theme) => {
  return {
    appbarSpace: 50,
  }
}

So after upgrading to 4.5.1, I shifted most of the component from @material-ui/styles to @material-ui/core/styles. But there's no export of CSSProperties now, so it complains about that.

Any idea?

eps1lon commented 5 years ago

@josephstgh It looks like your Common interface is unused. Could you expand your example so that Common and getAppStyle is used?

josephstgh commented 5 years ago

Hi sorry, let me try.

I have a place where Theme is defined.

interface Theme {
  customPalette: ReturnType<typeof getAppThemeColor>;
  commonStyle: () => Common;
}

const appTheme = (options: ThemeOptions) => {
  const theme = createMuiTheme({ ... // commented off });
  theme.commonStyle = () => getAppStyle(theme);
  return theme;
}

So in my other component where I can make use of the commonStyle.

const useStyles = makeStyles((theme: Theme) => ({
  smallPadding: {
    ...theme.commonStyle().appbarSpace,
    // some other properties
  }
});

Full disclosure, this is actually part of the workaround because jssExtend() plugin does not work for my case. Hence, the use of spreading to extend the style.

pschlette commented 5 years ago

@eps1lon Using the CSSProperties type seemed like a reasonable way to annotate variables holding some shared styles, like this (simplified a bit):

const useStyles = makeStyles(() => {
  // Annotate this with `CSSProperties` to avoid type widening and see type errors here
  const sharedStyles: CSSProperties = { display: "inline-flex" };

  return createStyles({
    class1: {
      ...sharedStyles,
      color: "red"
    },
    class2: {
      ...sharedStyles,
      color: "green"
    }
}));

Sorry - I'm not sure what you meant by *Styles?

croraf commented 5 years ago

I was really in favor to use @material-ui/styles and make it as a peer dependency of @material-ui/core.

I think this is something suggested here also: https://www.styled-components.com/docs/faqs#i-am-a-library-author-should-i-bundle-styledcomponents-with-my-library

coglite commented 5 years ago

@oliviertassinari I can rewrite the implementation to use react-jss in a couple hours, but there is 1 thing i am unclear on how to solve, which is the use of the '$(pseudoClass)' variables scattered throughout the styles. For example, the styles on the Button use '$disabled' below:

export const styles = theme => ({
  /* Styles applied to the root element. */
  root: {
    ...theme.typography.button,
    boxSizing: 'border-box',
    minWidth: 64,
    padding: '6px 16px',
    borderRadius: theme.shape.borderRadius,
    color: theme.palette.text.primary,
    transition: theme.transitions.create(['background-color', 'box-shadow', 'border'], {
      duration: theme.transitions.duration.short,
    }),
    '&:hover': {
      textDecoration: 'none',
      backgroundColor: fade(theme.palette.text.primary, theme.palette.action.hoverOpacity),
      // Reset on touch devices, it doesn't add specificity
      '@media (hover: none)': {
        backgroundColor: 'transparent',
      },
      '&$disabled': {
        backgroundColor: 'transparent',
      },
    },
    '&$disabled': {
      color: theme.palette.action.disabled,
    },
  },
//...rest

I understand what it does just not how it interops with jss-nested and its effect on rule generation. https://github.com/mui-org/material-ui/blob/master/packages/material-ui-styles/src/createGenerateClassName/createGenerateClassName.js

any guidance? or maybe like a pseudo code walk through of the logic. something like if stylesheet starts with Mui -> apply pseudoSelector logic else apply jss-nested logic