[typescript] Re-export Theme type in @material-ui/styles

[dangcuuson]

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Context 🔦

Currently, to use makeStyles with Theme, I'll have to use 2 imports:

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

const useStyles = makeStyles((theme: Theme) => ({
  // ...
}));

I'd be great if I we can update the styles package so that it re-export the Theme type from core, because I tend to group similar components of material-ui into 1 import statement:

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

styles already uses Theme type from core, so I believe making a re-exports should not be a big problem.

Thank you.

[oliviertassinari]

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

[dangcuuson]

Thanks.

So it means I don't really need @material-ui/styles if I already have @material-ui/core?

[oliviertassinari]

We reexport the styles modules with a default theme from the core. No, you most likely don't need it.

[fzaninotto]

If I'm not mistaken, we need @material-ui/styles for ThemeProvider, which is not exported in @material-ui/core/styles. Besides, the documentation for makeStyles mentions @material-ui/styles... I'm confused.

[oliviertassinari]

Does https://next.material-ui.com/customization/default-theme/#material-ui-core-styles-vs-material-ui-styles help?

[fzaninotto]

No, It's confusing me even more... I understand that @material-ui/core/styles/makeStyles is NOT the same thing as @material-ui/styles/makeStyles. So I don't know which one I should use... Is there one for when we use a ThemeProvider and one for when we don't use it?

[oliviertassinari]

The only difference is about the default theme. core removes the need for the user to use a theme provider at the expense of +3 kB gzipped. In the case of react-admin, you don't need to change anything. Import from core.

[fzaninotto]

But then I still need to import ThemeProvider from styles, am I right?

[oliviertassinari]

Why do you need a custom theme for? Imagine Material-UI was using styled-components, you would import the theme provider from styled-components.

[fzaninotto]

react-admin users typically want to set the theme of their admin. So we pass that theme to ThemeProvider, see https://github.com/marmelab/react-admin/blob/32e22a97a8ca754b3096842bc447b0d2ae3f8d4c/packages/ra-ui-materialui/src/layout/Layout.js#L190-L192

[oliviertassinari]

The code looks good to me. We have kept the MuiThemeProvider for backward compatibility but we don't document it anywhere.

[fzaninotto]

OK, so you confirm that I DO need to import both packages?

// for when I want to use styles
import { makeStyles } from '@material-ui/core/styles';

// for when I want to define the theme
import { ThemeProvider } from '@material-ui/styles';

Since you reexport makeStyles and Theme in core, I don't understand why you don't reexport ThemeProvider, too. I don't manage to understand where you drew the boundary between each package.

[oliviertassinari]

Yes. The boundary is drawn around material design. styles is agnostic to the specification.

[fzaninotto]

I understand the rationale of moving the styles to a standalone package to make things easy for those who just want styles and no mui components, but this shouldn't impact core users IMHO.

Would you consider re-exporting ThemeProvider in core? Not only would it simplify the styles for core users (import all style-related helpers and components from core/styles, no need to think about it), it would also simplify my task documenting custom themes in react-admin, and therefore explaining a boundary that I don't really understand ;)

[oliviertassinari]

We have kept the MuiThemeProvider for backward compatibility but we don't document it anywhere.

You can rely on it. It's still unclear what we will do about it.

[fzaninotto]

Thanks for your help 👍

[fzaninotto]

hmm it seems that TypeScript doesn't see this exported MuiThemeProvider:

src/auth/Login.tsx:12:5 - error TS2305: Module '"../../../../node_modules/@material-ui/core/styles"' has no exported member 'MuiThemeProvider'.

12     MuiThemeProvider,
       ~~~~~~~~~~~~~~~~


Found 1 error.

The related code:

import {
    MuiThemeProvider,
    createMuiTheme,
    withStyles,
    createStyles,
    WithStyles,
    Theme,
} from '@material-ui/core/styles';

[eps1lon]

We should just re-export the ThemeProvider from @material-ui/styles in @material-ui/core/styles. At this point I don't think we should actually advertise @material-ui/styles if people are using @material-ui/core. At least I don't see the benefit of importing from @material-ui/styles. If there is one we should document it.

[oliviertassinari]

@fzaninotto Yes, the TypeScript definitions should be aligned with the implementation.

Isolation of the styling solution of the core components in a dedicated package. Remove the MuiThemeProvider component:

https://next.material-ui.com/guides/migration-v3/#styles

@eps1lon What do you think of removing this point? My concern n°1 would be around what happens if we move to styled-components, do we still keep MuiThemeProvider?

[eps1lon]

We should just drop the breaking change. I can see no benefit and from my experience the whole @material-ui/styles vs. @material-ui/core/styles usage has only caused confusion.

I'm not sure some vaguely planned feature is a good reason to introduce a breaking change.

[oliviertassinari]

@eps1lon Sounds good 👍