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

Export ThemeProviderProps in @material-ui/core/styles #20380

Closed
1 task done
TidyIQ opened this issue Apr 2, 2020 · 3 comments · Fixed by #20390
Closed
1 task done

Export ThemeProviderProps in @material-ui/core/styles #20380

TidyIQ opened this issue Apr 2, 2020 · 3 comments · Fixed by #20390
Labels
new feature New feature or request package: material-ui Specific to @mui/material ready to take Help wanted. Guidance available. There is a high chance the change will be accepted typescript

Comments

@TidyIQ
Copy link
Contributor

TidyIQ commented Apr 2, 2020

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

Summary 💡

ThemeProvider is currently being exported in @material-ui/core/styles however ThemeProviderProps is NOT also exported here.

If you need to import ThemeProviderProps into your project then you have to import it from @material-ui/styles instead.

This means having to add @material-ui/styles as a dependency in your project (otherwise you will have extraneous dependencies, which is not a good practice) just to access ThemeProviderProps, which wouldn't be necessary if it were simply exported in @material-ui/core/styles.

It also has the potential to cause issues if ThemeProvider and ThemeProviderProps are modified in the future and a user upgrades @material-ui/core to the new version but doesn't also update @material-ui/styles.

Examples 🌈

Here's an example where the user is creating an AppProvider component to wrap the entire app. It includes react-redux's state provider and material-ui's theme provider.

Current implementation

import React from "react";
import { ThemeProvider } from "@material-ui/core/styles";
import { ThemeProviderProps } from "@material-ui/styles"; // Requires adding @material-ui/styles as a dependency in package.json
import { Provider as StateProvider, ProviderProps as StateProviderProps } from "react-redux";

interface AppProviderProps {
  store: StateProviderProps["store"];
  theme: ThemeProviderProps["theme"];
}

export default function AppProvider(props: AppProviderProps): React.ReactElement<AppProviderProps> {
  const { children, store, theme } = props;
  return (
    <StateProvider store={store}>
      <ThemeProvider theme={theme}>
        {children}
      </ThemeProvider>
    </StateProvider>
  );
};

Better implementation

import React from "react";
import { ThemeProvider, ThemeProviderProps } from "@material-ui/core/styles";
import { Provider as StateProvider, ProviderProps as StateProviderProps } from "react-redux";

interface AppProviderProps {
  store: StateProviderProps["store"];
  theme: ThemeProviderProps["theme"];
}

export default function AppProvider(props: AppProviderProps): React.ReactElement<AppProviderProps> {
  const { children, store, theme } = props;
  return (
    <StateProvider store={store}>
      <ThemeProvider theme={theme}>
        {children}
      </ThemeProvider>
    </StateProvider>
  );
};

Motivation 🔦

  • Eliminate the need to install unnecessary extra dependencies
  • Maintain best practices by not importing extraneous dependencies
  • Ensure no compatibility issues between ThemeProviderProps and ThemeProvider
@eps1lon eps1lon added new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted package: material-ui Specific to @mui/material typescript labels Apr 2, 2020
@eps1lon
Copy link
Member

eps1lon commented Apr 2, 2020

A PR that adds ThemeProviderProps from /core would be more than welcome.

@TomekStaszkiewicz
Copy link
Contributor

If you don't mind I would love to take care of that ;)

@TomekStaszkiewicz
Copy link
Contributor

#20390

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request package: material-ui Specific to @mui/material ready to take Help wanted. Guidance available. There is a high chance the change will be accepted typescript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants