mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.97k stars 32.28k forks source link

[RFC] Use `import * as Mui from 'material-ui'` in the demos #22529

Open o-alexandrov opened 4 years ago

o-alexandrov commented 4 years ago

Summary 💡

Instead of recommending to use named exports, how about recommending importing all modules?

Examples 🌈

Before:

import { Button, Table, ... } from "@material-ui/core"

After:

import * as Mui from "@material-ui/core"

Motivation 🔦

Pros:

Cons:

Example project

For your convenience I created a simple example that showcases:

Basically 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):

Screen Shot 2020-09-08 at 4 21 39 PM
eps1lon commented 4 years ago

material-ui doesn't import named exports from React, why should imports from material-ui be different?

We ship a bundle with actual ES modules but React doesn't. Import style is personal preference. We won't enforce a particular style.

oliviertassinari commented 4 years ago

The reason why it will improve consistency is because a similar decision was made here.

@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.

o-alexandrov commented 4 years ago

@oliviertassinari you contradict yourself:

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.

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?

o-alexandrov commented 4 years ago

@eps1lon you also contradict your own statement in this comment and also Olivier comment

oliviertassinari commented 4 years ago

@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).

o-alexandrov commented 4 years ago

@oliviertassinari what makes you think it will break tree-shaking? I am using import * as in every webpack environment and nowhere tree-shaking was ever broken.

But you don't need to trust me, please review the following as tree-shaking for import * as has been supported by webpack for a long time:

o-alexandrov commented 4 years ago

also updated Motivation part of the Issue description, so benefits are clearly highlighted

oliviertassinari commented 4 years ago

I am using import * as in every webpack environment and nowhere tree-shaking was ever broken.

@o-alexandrov Interesting, I didn't know! I wonder about the esthetic.

o-alexandrov commented 4 years ago

It also doesn't account for all the new components coming (doesn't scale).

I don't know about what's coming, but if you are talking about modules with side effects:

o-alexandrov commented 4 years ago

@o-alexandrov Interesting. I wonder about the esthetic.

Other arguments to consider and will copy them to Motivation above:

eps1lon commented 4 years ago

@eps1lon you also contradict your own statement in this comment and also Olivier comment

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.

o-alexandrov commented 4 years ago

@eps1lon please be informed the links to your comments above lead to your phrase:

So we do want to influence them. If we want to follow then we do what we're told. If we want to lead we're doing what we think they want. What you're describing is incoherent.

That’s what I referred to in the links above and that’s what I described as contradiction when here you say:

We won't enforce a particular style.

But the statement itself is incorrect as by making choices you enforce a particular style anyway. This issue asks to change your opinion on what you enforce.

o-alexandrov commented 4 years ago

@eps1lon regarding:

React's exports and Material-UI's exports are not the same. React does not ship ES modules.

I don’t know why you bring it up, especially twice. What does it change in terms of the importing style?

oliviertassinari commented 4 years ago

@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. 🤔

o-alexandrov commented 4 years ago

@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):

Screen Shot 2020-09-08 at 4 21 39 PM
o-alexandrov commented 4 years ago

Could you kindly open the issue, as it seems you support further discussion on this topic? I also added another note to the Pros in the Description of the issue above:

it's better marketing for material-ui and useful for beginners, as developers are reminded on what they use as they use values from Mui.

oliviertassinari commented 4 years ago

Let's see if it resonates with developers: https://twitter.com/olivtassinari/status/1304385408525045760, we can re-close the issue if not.

oliviertassinari commented 4 years ago

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 mui.Button will give you more accurate results.

We never destruture the theme object in the codebase for this very reason. We always do theme.palette.divider, not const { divider } = theme.palette.

It's important for refactoring.

oliviertassinari commented 4 years ago

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.

o-alexandrov commented 4 years ago

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 mui.*

I believe whatever you choose will be the most common pattern, so it's your call, as simple as that.

o-alexandrov commented 4 years ago

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:

Explicit imports make it easier to spot what is used

And seems like @oliviertassinari also likes the pattern.

oliviertassinari commented 4 years ago

@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.

Capture d’écran 2020-09-12 à 14 07 10
o-alexandrov commented 4 years ago

I totally understand decisions like this in a popular project are hard to make. I am already happy you were open to reconsider.

From here, please do as you see best for the community, you know what's best much better than I do.

troglotit commented 3 years ago

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

import * as MUI from '@material-ui/core'
import * as C from '../components'
// components/index.js
export * from 'Button.js'

compared to

import { Button as MuiButton } from '@material-ui/core'
import { Button as MyButton } from '../components/Button.js' // or import Button from '../components/Button.js'

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 a) name-clashes of auto-import (it just seems bizarre that only yesterday we stopped using global scoping, but somehow it's ok for auto-imports) b) coming up with "MyButton" name when it's just a regular button in my app code.

BryanAPEX commented 2 years ago

This is old but I just stumbled on it while considering refactoring to import * as MUI from '@mui/material' as part of a migration to version 5. Has there been any continued discussion on this anywhere?

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.

zemd commented 1 year ago

just for the record, personally I don't like imports like import { Button, Table, ... } from "@material-ui/core", so I always use direct imports. on the flip side, I think aesthetically import * as mui is not 100% nice, but it makes sense and I like the idea using <mui.Button/> especially in cases when multiple button implementations exist and I have to rename it to import Button as MuiButton from

jaydenseric commented 1 year ago

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

dburles commented 1 year ago

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.