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.

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

[core] Remove withTheme from @material-ui/core #26051

Merged
merged 14 commits into from
Apr 30, 2021

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Apr 29, 2021

  • The withTheme utility has been removed from the @material-ui/core/styles package. You can use the @material-ui/styles/withTheme instead. Make sure to add a ThemeProvider at the root of your application, as the defaultTheme is no longer available. If you are using this utility together with @material-ui/core, it's recommended you use the ThemeProvider from @material-ui/core/styles instead.

     import * as React from 'react';
    -import { withTheme } from '@material-ui/core/styles';
    +import { withTheme } from '@material-ui/styles';
    +import { createTheme, ThemeProvider } from '@material-ui/core/styles';
      
    +const theme = createTheme();
     const MyComponent = withTheme(({ theme }) => <div>{props.theme.direction}</div>);
    
     function App(props) {
    -  return <MyComponent />;
    +  return <ThemeProvider theme={theme}><MyComponent {...props} /></ThemeProvider>;
     }

One of the PRs that is necessary for removing @material-ui/styles as a dependency of @material-ui/core.

@mnajdova mnajdova marked this pull request as draft April 29, 2021 12:45
@mui-pr-bot
Copy link

mui-pr-bot commented Apr 29, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 03ab006

@@ -174,65 +172,6 @@ const t4: string = createTheme().spacing(1, 2, 3, 4);
// @ts-expect-error
const t5 = createTheme().spacing(1, 2, 3, 4, 5);

// withTheme
Copy link
Member Author

Choose a reason for hiding this comment

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

There are tests for this in the @material-ui/styles package.

@mnajdova mnajdova marked this pull request as ready for review April 29, 2021 14:26
docs/src/pages/guides/migration-v4/migration-v4.md Outdated Show resolved Hide resolved
docs/src/pages/guides/migration-v4/migration-v4.md Outdated Show resolved Hide resolved
docs/src/pages/guides/migration-v4/migration-v4.md Outdated Show resolved Hide resolved
docs/src/pages/guides/migration-v4/migration-v4.md Outdated Show resolved Hide resolved
docs/src/pages/guides/migration-v4/migration-v4.md Outdated Show resolved Hide resolved
docs/src/pages/guides/migration-v4/migration-v4.md Outdated Show resolved Hide resolved
docs/src/pages/styles/basics/basics.md Outdated Show resolved Hide resolved
docs/src/pages/styles/basics/basics.md Outdated Show resolved Hide resolved
docs/src/pages/styles/basics/basics.md Outdated Show resolved Hide resolved
mnajdova and others added 10 commits April 29, 2021 16:49
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@eps1lon
Copy link
Member

eps1lon commented Apr 29, 2021

Did we intend to keep withTheme at all considering useTheme exists?

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 29, 2021

I was wondering too. It makes sense for the legacy code that can't be migrated to hooks, no preferences.

@mnajdova
Copy link
Member Author

I wasn't thinking too much about it to be honest, I am just trying to break the dependencies to @material-ui/styles at this stage.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 29, 2021

@mnajdova I think that we need to resolve the question of Sebastian. I see two cases based on the answer:

  1. If we estimate that it's still too early to drop the HOC API, then it seems that the best course is to follow what we do with useTheme().
  2. If we estimate that it's time to introduce a breaking change and drop withTheme for v5 users (it wasn't planned in [RFC] Material-UI v5 🚀 #20012 but why not), then I think that the current state of the PR is OK.

@eps1lon
Copy link
Member

eps1lon commented Apr 29, 2021

Isn't it just a prop-injector that injects useTheme? That seems trivial enough to fork if you really need it. And in turn we can cut down in shipped code.

@mnajdova
Copy link
Member Author

@mnajdova I think that we need to resolve the question of Sebastian. I see two cases based on the answer:

  • If we estimate that it's still too early to drop the HOC API, then it seems that the best course is to follow what we do with useTheme().
  • If we estimate that it's time to introduce a breaking change and drop withTheme for v5 users (it wasn't planned in [RFC] Material-UI v5 🚀 #20012 but why not), then I think that the current state of the PR is OK.

Let's go with option 2. I wouldn't drop anything from @material-ui/styles, as we are anyway going to deprecate that package, so I don't think it's worth doing too many changes there. If you disagree, we can go trough everything exported from @material-ui/styles and see whether we can drop some things.

@oliviertassinari
Copy link
Member

@mnajdova Happy with option 2. The only reason I raised 1 is because of https://styled-components.com/docs/api#withtheme and https://emotion.sh/docs/theming#withthemecomponent-reactcomponenttype-reactcomponenttype.

@mnajdova mnajdova merged commit f738e41 into mui:next Apr 30, 2021
@oliviertassinari oliviertassinari added package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. breaking change package: material-ui Specific to @mui/material and removed package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Jul 5, 2021
@mnajdova mnajdova mentioned this pull request Jul 9, 2021
2 tasks
@oliviertassinari oliviertassinari modified the milestones: v5, v5-beta Aug 17, 2021
@oliviertassinari oliviertassinari mentioned this pull request Aug 17, 2021
42 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants