-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[RFC] Use import * as Mui from 'material-ui'
in the demos
#22529
Comments
We ship a bundle with actual ES modules but React doesn't. Import style is personal preference. We won't enforce a particular style. |
@o-alexandrov Note that we might revisit how we import React if Facebook adds support for tree-shaking in the future. Until they do, a single import is simple and avoids back and forth with the top of the file, we can ask the Python's community why it's the convention there. |
@oliviertassinari you contradict yourself:
then why don't you: import * as Mui from "@material-ui/core" instead of recommending named imports (based on your own statement it is more complicated), if both lead to the exact same bundle size? |
@eps1lon you also contradict your own statement in this comment and also Olivier comment |
@o-alexandrov while your proposal make the import tree-shaked in production, bundlers don't tree-shake in dev mode, I don't think that it's an option for the demos of the documentation. Developers with less experience will blame Material-UI for being bloated once they look at the bundle size in production. It also doesn't account for all the new components coming (doesn't scale). |
@oliviertassinari what makes you think it will break tree-shaking? But you don't need to trust me, please review the following as tree-shaking for
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
I don't know about what's coming, but if you are talking about modules with side effects:
|
Other arguments to consider and will copy them to Motivation above:
|
React's exports and Material-UI's exports are not the same. React does not ship ES modules. Material-UI does. Opinions I hold about importing React do not automatically apply to Material-UI. |
@eps1lon please be informed the links to your comments above lead to your phrase:
That’s what I referred to in the links above and that’s what I described as contradiction when here you say:
But the statement itself is incorrect as by making choices you enforce a particular style anyway. |
@eps1lon regarding:
I don’t know why you bring it up, especially twice.
|
@o-alexandrov So if we take this demo: https://material-ui.com/components/buttons/#contained-buttons, it would become: import React from 'react';
import * as mui from '@material-ui/core';
const useStyles = mui.makeStyles((theme) => ({
root: {
'& > *': {
margin: theme.spacing(1),
},
},
}));
export default function ContainedButtons() {
const classes = useStyles();
return (
<div className={classes.root}>
<mui.Button variant="contained">Default</mui.Button>
<mui.Button variant="contained" color="primary">
Primary
</mui.Button>
<mui.Button variant="contained" color="secondary">
Secondary
</mui.Button>
<mui.Button variant="contained" disabled>
Disabled
</mui.Button>
<mui.Button variant="contained" color="primary" href="#contained-buttons">
Link
</mui.Button>
</div>
);
} and we would still have tree-shaking working. Maybe it's worth asking on Twitter how the community feels about using the approach by default for the demos. 🤔 |
@oliviertassinari I apologize, I don't know whether you were asking or making a statement, so for your convenience I created a simple example that showcases:
Basically I put your example above into a new CRA project. Please follow commits as it's easier to understand what has been changed in a pure CRA project. And here is a screenshot of the webpack-bundle-analyzer. We can clearly see that only the button and what's necessary for it is imported (meaning tree-shaking is working): |
Could you kindly open the issue, as it seems you support further discussion on this topic?
|
Let's see if it resonates with developers: https://twitter.com/olivtassinari/status/1304385408525045760, we can re-close the issue if not. |
Another pros I didn't thought about before. The mui.* pattern makes it easier to identify what's used in the codebase. Say you have multiple custom buttons, searching for We never destruture the theme object in the codebase for this very reason. We always do It's important for refactoring. |
It doesn't seem to resonate a lot with the community. It seems that they are mostly used to the following approach: import { Button, makeStyles } from '@material-ui/core'; On Vue land, they seem to basically make all the components global: import * as mui from '@material-ui/core';
Object.keys(mui).forEach(key => {
window[key] = mui[key];
}); https://vuetifyjs.com/en/getting-started/quick-start/#vue-cli-install And then, some have a smart loader to have tree-shaking. |
I am not surprised pretty much none of the responders there read this GitHub issue 😅 I read all of the responses there:
Personally, I don't think v5 should be held by old inconvenient patterns and fear. You can set a new trend with I believe whatever you choose will be the most common pattern, so it's your call, as simple as that. |
Stats based on your tweet:
Then, here is @eps1lon (I really hope you'd read the description completely), who seems to not like it due to:
And seems like @oliviertassinari also likes the pattern. |
@o-alexandrov I will personally snooze the matter for the next 2+ months. I don't think that we need to rush any changes. We have until v5 land to make bold changes (if we see them appropriate). Regarding the esthetic, Ant Design and react-bootstrap: does that too, e.g. |
import * as Mui from 'material-ui'
import * as Mui from 'material-ui'
in the demos
I totally understand decisions like this in a popular project are hard to make. From here, please do as you see best for the community, you know what's best much better than I do. |
My experience report as per https://twitter.com/troglotit/status/1356496801067589633 is that it's a bit different paradigm, bit it shines when you structure your app using namespaces/modules
compared to
Some can say that one-letter variables are bad, but when for the whole app only components-module is named "C" it's like using "i" in for-loops. And IntelliSense with namespaces/modules is so satisfying Then I free myself from problem of either |
This is old but I just stumbled on it while considering refactoring to I'm drawn to the simplification and clarity it would bring to the code base. The main drawback I can see currently is that it seems incompatible with option 2 of minimizing bundle size in regards to the dev environment, and would result in increased startup times. |
just for the record, personally I don't like imports like |
Importing everything from an index module is a big anti-pattern; you can learn why in detail here: https://jaydenseric.com/blog/optimal-javascript-module-design |
I bet those that thumbed down this comment either haven't read the article, or cannot comprehend it. |
I can say that we won't change the import in the demos anytime soon. At least I don't see the benefit that outweigh the effort. |
This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Note @o-alexandrov How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey. |
Summary 💡
Instead of recommending to use named exports, how about recommending importing all modules?
Examples 🌈
Before:
After:
Motivation 🔦
Pros:
material-ui
doesn't import named exports fromreact
, why should imports frommaterial-ui
be different?Mui
Mui
Mui.Button
is a pureMui
button, whereasButton
is part of your project's modifications that fell outside of what you can do with theme's customizationsmaterial-ui
and useful for beginners, as developers are reminded on what they use as they use values fromMui.
Cons:
Mui.
Example project
For your convenience I created a simple example that showcases:
material-ui
works when importing allBasically I put @oliviertassinari example function into a new CRA project. Please follow commits as it's easier to understand what has been changed in a pure CRA project.
And here is a screenshot of the webpack-bundle-analyzer. We can clearly see that only the button and what's necessary for it is imported (to see it for yourself, run
yarn build
):The text was updated successfully, but these errors were encountered: