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] Move @mui/base from peer dependency to dependency #10215

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Sep 3, 2023

There is no singleton in @mui/base, no need to have it as a peer dependency. Along the way, I added a missing dependency for the charts.

I have noticed this issue when installing Toolpad:

Screenshot 2023-09-03 at 20 51 47

See #8590 (comment) for a previous discussion on this. I have seen the change listed in #7902 but I don't see how it could be a breaking change.


Side note:

  • import X from '@mui/base' should be banned, it pulls in everything. It should be imported from the lower level.
  • import X from '@mui/utils' should be banned in MUI X (unless there is a really good reason). It's a private npm repository for Base UI and MUI System. Developers who would look at how to add their own components should be able to do such with public packages. They might check MUI X to see how its done, so we should set the example.

@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Sep 3, 2023
@mui-bot
Copy link

mui-bot commented Sep 3, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-10215--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms -192.6 102.4 -192.6 -57.5 114.333
Sort 100k rows ms 691.7 1,459.5 1,434.1 1,211.46 296.892
Select 100k rows ms 604.1 746.1 685.5 673 49.047
Deselect 100k rows ms 136.7 230.5 177.2 184.86 37.075

Generated by 🚫 dangerJS against a69cf1e

@oliviertassinari oliviertassinari mentioned this pull request Sep 3, 2023
45 tasks
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Sep 3, 2023
@flaviendelangle
Copy link
Member

flaviendelangle commented Sep 4, 2023

@oliviertassinari how could we keep using those utilities without using @mui/utils ?
We re-implement them ?
We import them from another package ?
The MUI X components are not based on Base UI components (unlike Joy and Material), so for me they need the same utilities as Base UI to build their internals. So they very often need stuff that is exported from @mui/utils

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

My intent with adding this change only on v7 was not to make such "undecided change" in the same major, but yes, agreed, its not a breaking change. 👌

The changes look correct and make sense. 👍

As for the disallowing of imports, I'm on board with avoiding the root level imports. 👌
As for the @mui/utils, that's another story that needs discussing and I'd second the questions raised by Flavien. 🤔

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 4, 2023

how could we keep using those utilities without using @mui/utils ? We re-implement them ?

@flaviendelangle From what I recall, the last discussion with had in the core was that @mui/base should export the useful modules from @mui/utils, as part of its public API.

Why? Because if MUI X needs them, then it's likely any teams that want to extend a design system will also need them. cc @michaldudak as I might recall wrong.

The MUI X components are not based on Base UI components (unlike Joy and Material), so for me they need the same utilities as Base UI to build their internals. So they very often need stuff that is exported from @mui/utils

Yeah, the idea goes like this: MUI X extends Material UI & Joy UI, MUI X needs to have access to the same primitives as Material UI and Joy UI. So in the same way, Material UI should never import from @mui/utils.


The only limitation I can think of is that @mui/utils/composeClasses -> @mui/base/composeClasses -> @mui/x-date-picker is an extra module we could save. If it adds 20% of slowness to the build in dev mode, then maybe we might as well kill @mui/base/composeClasses and document how to import @mui/utils/composeClasses directly. But if the overhead is 2%, I think we should keep @mui/utils private as one npm package is a simpler mental model.

As for the disallowing of imports, I'm on board with avoiding the root level imports. 👌

@LukasTy I have created mui/material-ui#38806 to illustrate a bit the direction we could take.

@oliviertassinari oliviertassinari changed the title [core] Fix dependency on @mui/base [core] Move @mui/base from peer dependency to dependency Sep 4, 2023
@oliviertassinari oliviertassinari merged commit 98ebae7 into mui:master Sep 4, 2023
5 checks passed
@oliviertassinari oliviertassinari deleted the base-dependency branch September 4, 2023 22:03
@flaviendelangle
Copy link
Member

MUI X extends Material UI & Joy UI, MUI X needs to have access to the same primitives as Material UI and Joy UI.

I do not totally agree on this.

Material UI and Joy UI are built on top of Base UI
MUI X is not built on top of Base UI, it creates it's own headless components.
For me, MUI X creates both the level of abstraction of Base UI and (Material UI + Joy UI) and then uses (Material UI + Joy UI) for certain parts of its application.

For example, if I understand correctly, the only time Joy UI is using useControlled is for component not using Base UI (because Base UI does not support them yet or because those components make no sense in Base UI, I don't know).
But for MUI X, we have a lot of usages of useControlled and no evolution of Base UI will ever change that (except if we want to move the headless version of the X components in Base UI but I don't think that's the plan).

So we can re-export everything MUI X needs for @mui/utils from Base UI (with nested imports), but it won't be a few imports, we are using this library extensively.

@michaldudak
Copy link
Member

michaldudak commented Sep 8, 2023

The purpose of the @mui/utils package has been under discussion for quite some time. I created a page where we can list the possibilities and make a decision: https://www.notion.so/mui-org/mui-utils-purpose-9a9fc9da3a004864b6c4e1f4d1f24f95

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Sep 8, 2023

@michaldudak it sounds great, thanks

So we can re-export everything MUI X needs for @mui/utils from Base UI (with nested imports), but it won't be a few imports, we are using this library extensively.

@flaviendelangle True, this is my main open question: Does so many re-exports harms build time or built output size? I would want to do a quick benchmark to make sure.

It will be slower for making changes to the utils but this sounds fair in exchange of having a single consistent package.

@flaviendelangle
Copy link
Member

The set of utility methods we are using from @mui/utils is pretty stable so I don't think it would be too much of a pain on our side.
If we don't have to discuss 2 weeks to add an export when needed of course.

@oliviertassinari
Copy link
Member Author

I have created #10354 to cover the deep import aspect of the discussion.

@oliviertassinari oliviertassinari restored the base-dependency branch September 16, 2023 12:31
@oliviertassinari oliviertassinari deleted the base-dependency branch January 23, 2024 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants