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

[ClassNameGenerator] Prevent all base imports #31297

Merged
merged 9 commits into from Mar 7, 2022

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Mar 4, 2022

Fix #30011 (comment)

BREAKING CHANGE

unstable_ClassNameGenerator has been moved from utils to className folder to prevent all MUI Base module imports. Please update the import in your project if you use this module:

- import { unstable_ClassNameGenerator } from '@mui/material/utils';
+ import { unstable_ClassNameGenerator } from '@mui/material/className';

Note: the usage does not change.

Root cause

import { unstable_ClassNameGenerator as ClassNameGenerator } from "@mui/material/utils";
// `@mui/material/utils` > createSvgIcon > SvgIcon > @mui/base
// at this point the whole '@mui/base' has been parsed
// `badgeClasses` is parsed before the below `configure` fn, that's why the prefix is `Mui` even though the App has not been parsed yet.

ClassNameGenerator.configure((componentName) =>
  componentName.replace("Mui", "Foo")
);

import App from './App';

<App />

To fix this, the import must be specific and must not include any base components. So I move unstable_ClassNameGenerator to its own folder:

import { unstable_ClassNameGenerator as ClassNameGenerator } from "@mui/material/className";

In the future, if we want to do anything related to className we can put them in this folder. If we continue to use utils folder, there might be a regression in the future if we forgot about this.

Note

Not sure why CodeSandbox does not work but it works locally (by pulling from CodeSandbox built)

Further questions

  • Should it throw an error for the current import from @mui/material/utils so it leads people to import from @mui/material/className instead?

  • I have followed (at least) the PR section of the contributing guide.

@mui-bot
Copy link

mui-bot commented Mar 4, 2022

Details of bundle changes

@material-ui/core: parsed: +0.10% , gzip: +0.16%

Generated by 🚫 dangerJS against c8fa5db

@siriwatknp siriwatknp force-pushed the fix/classname-generator-base branch from 6772fa5 to 3579db6 Compare March 4, 2022 03:12
@siriwatknp siriwatknp marked this pull request as ready for review March 4, 2022 03:28
@siriwatknp siriwatknp requested a review from mnajdova March 4, 2022 03:28
@mnajdova
Copy link
Member

mnajdova commented Mar 4, 2022

Should it throw an error for the current import from @mui/material/utils so it leads people to import from @mui/material/className instead?

Yes, it is unstable API so we can play around. My proposal would be to show a meaningful message containing the new import people should make.

@siriwatknp
Copy link
Member Author

siriwatknp commented Mar 7, 2022

@mnajdova decided to do the same with base & joy (moved to folder className) so all the packages are consistent. Also, warning was added if importing from utils folder.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Please add a breaking change sectio in the PR description, so that we don't forget to include it in the changelog of the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

createGenerateClassName in Material UI v5
3 participants