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鈥檒l 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

Open
1 task done
o-alexandrov opened this issue Sep 8, 2020 · 28 comments
Open
1 task done

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

o-alexandrov opened this issue Sep 8, 2020 · 28 comments
Labels
discussion docs Improvements or additions to the documentation RFC Request For Comments waiting for 馃憤 Waiting for upvotes

Comments

@o-alexandrov
Copy link

o-alexandrov commented Sep 8, 2020

  • I have searched the issues of this repository and believe that this is not a duplicate.

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:

  • improves consistency
    • because a similar decision was made here
      • material-ui doesn't import named exports from react, why should imports from material-ui be different?
  • improves DX
    • as it removes the need to manually clean up unused imports (when there is a linter rule in development workflow).
    • as it removes the need to manually add new imports from Mui
      • as IDEs most of the time fail to auto import from libraries already present in the module
    • as it is cleaner, since less individual imports are in the module
      • it is easier to understand where the variable is coming from while looking at the code, as you know the variable in question belongs to Mui
        • for example: Mui.Button is a pure Mui button, whereas Button is part of your project's modifications that fell outside of what you can do with theme's customizations
  • 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.

Cons:

  • the amount of code writing (less lines, but a bit more actual text) is more as there'd be a prefix everywhere: Mui.
    • but the clear indication where these variables are coming from is far more important

Example project

For your convenience I created a simple example that showcases:

  • tree-shaking of material-ui works when importing all

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

@o-alexandrov o-alexandrov added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 8, 2020
@eps1lon
Copy link
Member

eps1lon commented Sep 8, 2020

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
Copy link
Member

oliviertassinari commented Sep 8, 2020

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
Copy link
Author

o-alexandrov commented Sep 8, 2020

@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
Copy link
Author

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

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 8, 2020

@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
Copy link
Author

o-alexandrov commented Sep 8, 2020

@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

This comment was marked as resolved.

@oliviertassinari oliviertassinari removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 8, 2020
@oliviertassinari

This comment was marked as resolved.

@o-alexandrov
Copy link
Author

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:

  • the side effects should introduce the exact same problems with the named imports as all exported modules are parsed

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Sep 8, 2020
@o-alexandrov
Copy link
Author

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

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

  • it is cleaner, since less individual imports are in the module
    • it is easier to understand where the variable is coming from while looking at the code, as you know the variable in question belongs to Mui
  • and not only it improves DX when removing unused imports
    • it also improves DX when importing more API from Mui, as IDEs sometimes fail to auto import from libraries already present in the module

@eps1lon
Copy link
Member

eps1lon commented Sep 8, 2020

@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
Copy link
Author

@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鈥檚 what I referred to in the links above and that鈥檚 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
Copy link
Author

@eps1lon regarding:

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

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

  • if you say it makes named imports better, then why? What鈥檚 your point?

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 8, 2020

@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
Copy link
Author

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

  • tree-shaking of material-ui works when importing all

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
Copy link
Author

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
Copy link
Member

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

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 11, 2020

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
Copy link
Member

oliviertassinari commented Sep 12, 2020

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
Copy link
Author

I am not surprised pretty much none of the responders there read this GitHub issue 馃槄

I read all of the responses there:

  • people ask: would it hurt bundle size
    • even though with absolutely no modifications to webpack's bundling optimization defaults, tree-shaking works the same for both types of imports
  • some agree and pretty positive about it
  • some don't like it due to legacy projects

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
Copy link
Author

Stats based on your tweet:

  • 16 different users answered (not including Sebastian and Olivier)
    • 7 undecided: @sandeep0695, @TandonKrishna12, @AndaristRake, @erikras, @thegoldenshun (mentioned only with icons, so I'd say more positive than negative), @arunkumar413, @MelamedYoav
    • 6 like it: @ee0pdt (likes it, but held with a legacy project), @garbage_value, @alchemist_ubi, @Lani78, @ModularCoder, @kandelborg
    • 3 don't like it: @pflevy,@MelloGarrett, @dfmartin

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
Copy link
Member

oliviertassinari commented Sep 12, 2020

@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 a虁 14 07 10

@oliviertassinari oliviertassinari changed the title [docs] use import * as Mui from 'material-ui' [RFC] Use import * as Mui from 'material-ui' in the demos Sep 12, 2020
@o-alexandrov
Copy link
Author

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.

@oliviertassinari oliviertassinari added the waiting for 馃憤 Waiting for upvotes label Nov 19, 2020
@troglotit
Copy link

troglotit commented Feb 4, 2021

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
Copy link

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.

@oliviertassinari oliviertassinari added the RFC Request For Comments label Aug 21, 2022
@zemd
Copy link

zemd commented Jan 27, 2023

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
Copy link
Contributor

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
Copy link

dburles commented Jul 20, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion docs Improvements or additions to the documentation RFC Request For Comments waiting for 馃憤 Waiting for upvotes
Projects
None yet
Development

No branches or pull requests

8 participants