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

[material] Remove dependency to @mui/base #42907

Merged
merged 25 commits into from
Jul 12, 2024

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova added the core Infrastructure work going on behind the scenes label Jul 11, 2024
@mui-bot
Copy link

mui-bot commented Jul 11, 2024

Netlify deploy preview

https://deploy-preview-42907--material-ui.netlify.app/

@material-ui/utils: parsed: +152.12% , gzip: +70.16%
@material-ui/lab: parsed: +4.00% , gzip: +4.80%
@mui/joy: parsed: +0.20% , gzip: +0.24%
@mui/joy/Tooltip: parsed: +1.00% , gzip: +0.87%
@mui/joy/Autocomplete: parsed: +0.59% , gzip: +0.55%
@mui/joy/Select: parsed: +0.77% , gzip: +0.61%
@material-ui/unstyled: parsed: +0.65% , gzip: +0.09%
SpeedDialAction: parsed: -0.18% 😍, gzip: -0.31% 😍
@material-ui/core: parsed: -0.02% 😍, gzip: -0.08% 😍

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against cc5aea6

@mnajdova mnajdova marked this pull request as ready for review July 11, 2024 12:01
@DiegoAndai DiegoAndai mentioned this pull request Jul 11, 2024
@aarongarciah
Copy link
Member

@mnajdova today I merged #37301, which contains changes in mui-base. Can you update this PR to include them?

@mnajdova
Copy link
Member Author

@mnajdova today I merged #37301, which contains changes in mui-base. Can you update this PR to include them?

Thanks for the heads up! I will update it tomorrow and have this ready for review :)

@mnajdova mnajdova added the needs cherry-pick The PR should be cherry-picked to master after merge label Jul 12, 2024
@mnajdova mnajdova requested review from aarongarciah and removed request for michaldudak July 12, 2024 07:26
Copy link
Member

@aarongarciah aarongarciah left a comment

Choose a reason for hiding this comment

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

Looks good

docs/pages/material-ui/api/popper.json Show resolved Hide resolved
@@ -27,6 +27,9 @@ export const projectSettings: ProjectSettings = {
getHookInfo: getBaseUiHookInfo,
translationLanguages: LANGUAGES,
skipComponent: () => false,
skipHook: (filename) => {
return filename.match(/(useSlotProps)/) !== null;
Copy link
Member Author

Choose a reason for hiding this comment

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

This hook was anyway not documented, so I've added this option to skip, otherwise the docgen generation failed. It's an optional prop it won't break other ts projects.

@mnajdova
Copy link
Member Author

mnajdova commented Jul 12, 2024

@aarongarciah this is ready for final review. Changes after the approval:

  • moved the utils used in MUI X to @mui/utils to avoid having to add @mui/material as a dependency to some packages that are Material UI agnostic (charts for example)
  • fixed the docs generation script, as Base UI is not just re-exporting some hooks from @mui/utils (useSlotProps)

Copy link
Member

@aarongarciah aarongarciah left a comment

Choose a reason for hiding this comment

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

🏎️ 🚀

@mnajdova
Copy link
Member Author

Thanks for the review @aarongarciah. Next steps:

  • cherry-pick on master
  • create PR on MUI X with the updates
  • after we ensure everything works, mark the @mui/base package as private and stop releasing it

I will move with the next steps.

@mnajdova mnajdova merged commit 3bbff37 into mui:next Jul 12, 2024
23 checks passed
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

What's the motivation for this change? It seems that the PR description is missing context.

@@ -27,6 +27,7 @@ module.exports = [
{ pathname: '/material-ui/api/checkbox' },
{ pathname: '/material-ui/api/chip' },
{ pathname: '/material-ui/api/circular-progress' },
{ pathname: '/material-ui/api/click-away-listener' },
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused about this. Would it mean we will have:

SCR-20240713-bcxd

@@ -125,6 +128,7 @@ module.exports = [
{ pathname: '/material-ui/api/tab-panel' },
{ pathname: '/material-ui/api/tabs' },
{ pathname: '/material-ui/api/tab-scroll-button' },
{ pathname: '/material-ui/api/textarea-autosize' },
Copy link
Member

Choose a reason for hiding this comment

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

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 21, 2024

From the exposure to this change and dive discussed in mui/mui-x#13823 can we revert this PR and go in the opposite direction?

I also got confused in #42604 (comment), we shouldn't have 3 versions at any point in time. As a side note, we need the git history of those components, it's missing after this PR.

It seems that having those modules in @mui/base, hosted in http://github.com/mui/material-ui would already to be a clear step forward, with no real downsides.
Now, I'm truly advocating to have those hosted in http://github.com/mui/base-ui and exposed from @base_ui/react as unstable/legacy.

@mnajdova
Copy link
Member Author

mnajdova commented Jul 22, 2024

From the exposure to this change and dive discussed in mui/mui-x#13823 can we revert this PR and go in the opposite direction?

What exactly is the opposite direction?

I also got confused in #42604 (comment), we shouldn't have 3 versions at any point in time.

Right, I would assume by moving the component to Mateiral UI, we won't need to do any updates on @mui/base, the initial plan is to no longer release this package, mark it as private, and eventually remove the code once we have @base_ui.

It seems that having those modules in @mui/base, hosted in http://github.com/mui/material-ui would already to be a clear step forward, with no real downsides.

The motivation for the change is that we will no longer need to make updates on @mui/base, not about where the components are being documented.

Now, I'm truly advocating to have those hosted in http://github.com/mui/base-ui and exposed from @base_ui/react as unstable/legacy.

I am not sure which if any of these components will live in Base UI, and what would be the form of these. What is the value of adding legacy stuff in Base UI, if these are only used in Material UI and Base UI do not plan to support these or plans to change the API around them. I will let @colmtuite and @michaldudak answer on this, but in my opinion this was not an option, hence I went with moving them to Material UI.

@mnajdova
Copy link
Member Author

As a side note, we need the git history of those components, it's missing after this PR.

I can fix this in #43028, I've applied the changes needed for the NoSsr, but I will wait with updating all components until we have a decision on how to move forward.

@colmtuite
Copy link
Contributor

I would assume by moving the component to Mateiral UI, we won't need to do any updates on @mui/base, the initial plan is to no longer release this package, mark it as private, and eventually remove the code once we have @base_ui.

This makes sense to me, and is what the majority of the team have been planning for at least the past few months. I figure we would just take each util/package, figure out the most appropriate place for it to belong, and move it there. Then that team are responsible for it. Sometimes, the most appropriate place will be Base UI, sometimes, Material UI, sometimes X, and potentially sometimes a shared utils lib.

But corrupting the new Base UI lib by prematurely abstracting legacy utils/components to it—when we don't know if they belong there long-term—is the worst path we could take. It's much worse than duplicating things in multiple locations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes needs cherry-pick The PR should be cherry-picked to master after merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants