mui / material-ui

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

[RFC] Restructure icons exports #42704

Open o-alexandrov opened 5 months ago

o-alexandrov commented 5 months ago

What's the problem?

This RFC looks at 2 problems with icons:

  1. for MUI's users, there's no out-of-the-box option to enforce icons' variant: Sharp, Rounded, Outlined, TwoTone, Filled
  2. too many icons; it affects performance (see related issues)

What are the requirements?

MUI's user should be able to enforce icons' variants and the number of icons shouldn't affect DX.

Proposed solution

We can tackle both problems by allowing MUI's users to have more consistent interfaces by enforcing icons' variants using ESLint's no-restricted-imports.

File structure for Material Icons:

File structure for Material Symbols:

|-- index.ts // exports everything as it is now to support the current usage
|-- /variants
   |-- /sharp // exports only Sharp variant, available as "@mui/icons-material/sharp"
      |-- /100 // exports only 100 weight, available as "@mui/icons-material/sharp/100"
         |-- /neg25 // exports only -25 grade, available as "@mui/icons-material/sharp/100/neg25"
         |-- /0 // exports only 0 grade, available as "@mui/icons-material/sharp/100/0"
         |-- /200 // exports only 200 grade, available as "@mui/icons-material/sharp/100/200"
      |-- ...other weights in separate directories 200, 300, 400, 500, 600, 700
   |-- /rounded // exports only Rounded variant, available as "@mui/icons-material/rounded"
      |-- /100 // exports only 100 weight, available as "@mui/icons-material/rounded/100"
         |-- /neg25 // exports only -25 grade, available as "@mui/icons-material/rounded/100/neg25"
         |-- /0 // exports only 0 grade, available as "@mui/icons-material/rounded/100/0"
         |-- /200 // exports only 200 grade, available as "@mui/icons-material/rounded/100/200"
      |-- ...other weights in separate directories 200, 300, 400, 500, 600, 700
   |-- /outlined // exports only Outlined variant, available as "@mui/icons-material/outlined"
      |-- /100 // exports only 100 weight, available as "@mui/icons-material/outlined/100"
         |-- /neg25 // exports only -25 grade, available as "@mui/icons-material/outlined/100/neg25"
         |-- /0 // exports only 0 grade, available as "@mui/icons-material/outlined/100/0"
         |-- /200 // exports only 200 grade, available as "@mui/icons-material/outlined/100/200"
      |-- ...other weights in separate directories 200, 300, 400, 500, 600, 700

Related issues

Search keywords: material icons, symbols, icons v2

TheOneTheOnlyJJ commented 5 months ago

Not long ago I made a suggestion that would exapnd the icons' package features and improve DX when requiring specific icons at runtime (eg. based on user input).

See the issue at #42450.

Now, thinking of it in the context of performance, splitting the icons into separate packages based on style could allow tree shakers to reduce the bundled size significantly if icons of one particular style are used. A separate import could be used for when all the icons of all styles are required.

My issue was requesting the ability to get the icons at runtime based on their style(s) and tag(s)/synonym(s) (see why I need that feature in mui/mui-x#13206). It's a shame it got closed just because there's already a hacky way to import icons at runtime in userland. The current way of doing it is also not optimised for a particular style. I'm glad this RFC has been opened, so we can have a proper discussion on the topic.

Given the very likely adoption of Material Symbols (#32846) in MUI v7, I believe an expansion of the icons' package API (like my suggestion or similar) should be considered by the team. A well thought-out API expansion in this area could bring both extended functionality and improved performance, thus covering extra use-cases (like mine from mui/mui-x#13206) and improving performance, as mentioned here.

TheOneTheOnlyJJ commented 5 months ago

I'll copy & paste the proposed API changes I made in the issues mentioned in the above reply so that they can be discussed further right here.

I would like to add an explicit example of how the utility function from the icons package I was talking about could look like:

import { getIcons } from '@mui/icons-material';
import SvgIcon from '@mui/material/SvgIcon';

// Get all icons in all styles
let allIcons: Array<SvgIcon> = getIcons();

// Get all outlined icons
let allOutlinedIcons: Array<SvgIcon> = getIcons({ styles: [ 'outlined' ] });

// Get only filled and outlined icons
let filledAndOutlinedIcons: Array<SvgIcon> = getIcons({ styles: [ 'filled', 'outlined' ] });

// Get only filled icons with the "home" tag
// Should only return these icons: https://mui.com/material-ui/material-icons/?query=home
let filledHomeIcons: Array<SvgIcon> = getIcons({ styles: [ 'filled' ], tags: { anyOf: [ 'home' ] } });

// Get all icons with the "home" tag, in all styles
let allHomeIcons: Array<SvgIcon> = getIcons({ tags: { anyOf: [ 'home' ] } });

// Get all icons that have either the "home" or the "building" tags, in all styles
let allHomeOrBuildingIcons: Array<SvgIcon> = getIcons({ tags: { anyOf: [ 'home', 'building' ] } });

// Get all icons that have both the "home" and "building" tags, in the sharp style only
let allSharpHomeAndBuildingIcons: Array<SvgIcon> = getIcons({ styles: [ 'sharp' ], tags: { allOf: [ 'home', 'building' ] } });

// Do not support allOf and anyOf at the same time
let willThrowException = getIcons({ tags: { anyOf: [ 'home' ], allOf: [ 'building' ] } });

The styles argument is optional and, if missing, every style will be included in the output. It is a string array that can have any of the following options filled, outlined, rounded, two tone and sharp. These could additionally also be exported as enums to be used in code conveniently.

The tags argument is also optional, and its filtering will apply together with the style filter. It is an object that must have either the anyOf or the allOf keys that map to string arrays. They should not be both supported at the same time, resulting in an exception being thrown. Using the argument with anyOf option will return all the icons that have at least one of the tags in the list. Using it with the allOf option will return only the icons that have all the tags from the given tag list.

With this kind of API, the Icon Picker component can pass its state as arguments to the getIcons function and fulfil my requirement. When the user would change the style option(s) or change the search word, the state would update and call the function again, and the Icon Picker would rerender to only display the new icons returned by getIcons. Having both the anyOf and the allOf tag selectors will enable this component to potentially use Chip-based sorting as well as the already discussed Radio Button/Tree View variants.

Additionally, a function that allows getting all tags for a specific icon (maybe call it getIconTags) should also be available for flexibility (to show the user the keywords/tags a specific icon can be found by), but this would require an internal ID for every icon to be passed as an argument to that function. I am not that accustomed to the internals of the icons package, so I do not know how difficult such an addition would be.

If this icons-related reply is out of topic here, feel free to move (or copy) this issue to the corresponding repository (material-ui).

Of course, this proposed API should be expanded taking into account the separation of the icons' packages based on style, to reduce the final build size through tree shaking when not all styles are required.

Janpot commented 1 month ago

Regarding 1.

We can tackle both problems by allowing MUI's users to have more consistent interfaces by enforcing icons' variants using ESLint's no-restricted-imports.

To note that currently it's already possible to enforce this with no-restricted-imports:

To allow "filled" only (playground):

/* eslint no-restricted-imports: ["error", { 
  patterns: [
    {
      regex: '@mui/icons-material$',
      importNamePattern: ".*(Outlined|Rounded|TwoTone|Sharp)",
      message: "Only filled icons are allowed."
    }, 
    {
      group: ['@mui/icons-material/*Outlined', '@mui/icons-material/*Rounded', '@mui/icons-material/*TwoToned', '@mui/icons-material/*Sharp'],
      message: "Only filled icons are allowed."
    }
  ]
}]
*/

To allow "outlined" only (playground):

/* eslint no-restricted-imports: ["error", { 
  patterns: [
    {
      regex: '@mui/icons-material$',
      allowImportNamePattern: ".*Outlined",
      message: "Only outlined icons are allowed."
    }, 
    {
      group: ['@mui/icons-material/*', '!@mui/icons-material/*Outlined'],
      message: "Only outlined icons are allowed."
    }
  ]
}]
*/

Regarding 2.

I don't think this would sufficiently address the performance issues. Avoiding importing from the barrel file is the solution we propose. Next.js does this for you automatically if you want to keep using the top-level import. There exist solutions for other runtime. This could be a matter of documenting better. Which bundler are you using?

Would this sufficiently address your concerns? If so, I propose we close this RFC as wontfix.

Janpot commented 1 month ago

@TheOneTheOnlyJJ I don't think your proposed API has the potential to improve performance. This can only be implemented by importing all icons in a single file and returning them conditionally in a function, at which point we're at the same level of a barrel file. It would in theory be able to somewhat simplify the implementation of the icons search page, but that's not a use-case we're optimizing for.

o-alexandrov commented 1 month ago

Thank you for the ESLint rule, I’ll try it out. Regarding avoiding importing from barrel files is not an option for the projects I worked on as it worsens DX.

Especially, since vite is soon going to introduce rolldown as a replacement for esbuild and rollup, the decrease by 5 (the user will start to transform ~80% less icons from the current barrel files) and 63 times (for the upcoming symbols) the number of icons is good enough performance boost

o-alexandrov commented 1 month ago

I don't think this would sufficiently address the performance issues.

Just doesn’t make sense, why 5 times or 80% less for current icons and ~98% for the upcoming symbols is insufficient for you?

Look how happy everyone is for the 20% performance boost here https://github.com/mui/material-ui/pull/43412

Janpot commented 1 month ago

Just doesn’t make sense, why 5 times or 80% less for current icons and ~98% for the upcoming symbols is insufficient for you?

The cut-off point you choose at how much we should optimize this is at 2500 exports. Why 2500 and not 1000? Or less? The number is arbitrary and it doesn't allow for scaling up. i.e. if the numebr of icons doubles, the performance halves again. Are we going to keep updating the package layout until it matches the 2500 limit again?

Instead I believe this is an issue with the runtime. Next.js sufficiently addressed the problem with the optimizePackageImports setting. Why couldn't such an optimization exist in vite? IMO It would be a better feature to advocate for, why settle for a 80% improvement if you can go for a 99.9%?

For instance I could achieve a similar behavior in vite with vite-plugin-import or vite-plugin-barrel

o-alexandrov commented 1 month ago

DX - users don’t need yet another plugin if you decrease the icons by 80% in the barrel file.

Pigment CSS requires a plugin, now you want to force users to have one for icons.

There’s a reason why users prefer no configuration frameworks. You seem not to grasp that.

Janpot commented 1 month ago

DX - users don’t need yet another plugin if you decrease the icons by 80% in the barrel file.

Pigment CSS requires a plugin, now you want to force users to have one for icons.

We're not forcing the use of a plugin, best is just to use subpath imports. We're considering removing the barrel file altogether in v7 and make it impossible to use the library in a way that's degrading performance. Sometimes it's just about taking footguns away.

It's not decided yet whether you will need the pigment plugin to use @mui/material when it's built on top of pigment css. But highly likely you won't need one (except if you want to use pigment css in your own code ofcourse). We will make sure to communicate clearly about this over the coming months. Potentially you will need a runtime that can understand CSS imports in node modules.

There’s a reason why users prefer no configuration frameworks. You seem not to grasp that.

I fully agree this should be zero config. It is zero-config in Next.js, and I believe it could be made so in vite as well.


Another solution for you could be to build your own barrel file which re-exports exactly those icons you wish to support in your application (e.g. only the rounded ones).

o-alexandrov commented 1 month ago

@Janpot if you get rid of barrel files, you’ll lose:

I offered you a solution by 5 times decreasing the number of modules to transpile and 63 times for newer icons. @Janpot ‘s "solution” is to remove the feature.

@oliviertassinari in this PR I saw you mentioned you are working on material symbols. Are you removing barrel files from icons? If you are not removing barrel files, please consider to structure directories as suggested in this issue to avoid bloated barrel files

oliviertassinari commented 1 month ago

@o-alexandrov I haven't looked closely at how we can support Material Symbol. From what I understand, in the SVG version, we have:

So total 144,018 icons, to put in comparison with the current legacy version:

So total 10,705 icons. It's x10 more SVG icons, so there can't be a barrel import for all of the icons since it's already too slow.

consistency. If you get rid of barrel files for icons, you should get rid of them everywhere

I don't necessarily think this is true. There can be exceptions, it's painful but there can be. At least, we never use the barrel import in any of the demos in the docs, either for components or else. I personally avoid barrel imports as a developer because it means that I can't easily move my code around, I have to group imports, and because I find it a bit harder to read, it can't scan down a list of imports, the rhythm changes when there is a group.

ability to autocomplete in IDE when partially typing

I don't necessarily think this is true. Shouldn't IDE be able to understand the exports fields of package.json? If we name those icons with something like an "Icon" suffix, maybe that should do it.

I offered you a solution by 5 times decreasing the number of modules to transpile and 63 times for newer icons.

I'm not following how this works. It looks like it's even slower: barrel that imports other barrels?

o-alexandrov commented 1 month ago

If we name those icons with something like an "Icon" suffix, maybe that should do it.

It should be prefix not suffix for the autocompletion to trigger from first letters. If you use suffix (ex. AccountIcon); then, after typing Icon, you cannot type anything else, because it will stop matching.

At least, we never use the barrel import in any of the demos in the docs, either for components or else.

You always use a barrel file for import * as React in demos (ex. Button). There's also your comment in favor of barrel imports from 2020.

I'm not following how this works. It looks like it's even slower: barrel that imports other barrels?

I've never encountered a website as a user, and as a developer where I've seen/used more than 1 icon variant. Designers I've worked with always pick either Outlined, Rounded, etc. for the whole project; same applies to weight.

The proposal of this issue is to have a separate barrel file per use case; i.e. you have 2,141 icons per barrel file for existing icons, and 3,429 per barrel file for symbols (new) icons.

oliviertassinari commented 1 month ago

You always use a barrel file for import * as React in demos (ex. Button). There's also your comment in favor of barrel imports from 2020.

@o-alexandrov Right, to be clear, the API that I personally don't have a great experience using is:

import { moduleName } from "package-name";

Base UI uses a different API, this one indeed https://github.com/mui/material-ui/issues/22529#issuecomment-691478260. I personally find the DX with this enjoyable. However, I'm not aware there they will promote the barrel index from the root of the npm package, it's per component (component in the UI sense, not the React one): https://base-ui.com/components/react-alert-dialog/. I imagine that Material UI will move to replicate this since we are downstream.

import * as MaterialUI from "@mui/material"; // works but please no, performance
import { Dialog } from "@mui/material"; // works but please no, performance
import Dialog from "@mui/material/Dialog"; // nice
import DialogTitle from "@mui/material/DialogTitle"; // nice, but on its way out, will be Dialog.Title

It should be prefix not suffix for the autocompletion to trigger from first letters. If you use suffix (ex. AccountIcon); then, after typing Icon, you cannot type anything else, because it will stop matching.

Right, this will be an interesting DX tradeoff decision to make 😁.

I've never encountered a website as a user, and as a developer where I've seen/used more than 1 icon variant. Designers I've worked with always pick either Outlined, Rounded, etc. for the whole project; same applies to weight.

Right ok, so not barrel index for all the icons, but 3 barrel one for each variant of Material Symbols. This still sounds like they will be big barrel index files, too large to have a great DX, so still footguns? As for the lower level barrels, I don't know, it feels like it creates too many ways to import the same modules.

o-alexandrov commented 1 month ago

imho, performance shouldn't be prioritized, DX should. The simple restructure of icons leads to good enough results 80% & ~97% number of icons in barrel files decrease. Next.js will probably finish Turbopack at some point for production bundles, whereas other solutions, such as Vite, tend to switch to other Rust-based bundling solutions; ex. VoidZero w/ Rolldown for Vite

import * as muiIcons from "@mui/icons-material-outline"; // perfect; no need to configure a linter; can be added to project's IDE code snippets; ex. `.vscode/*.code-snippets`
import * as muiIcons from "@mui/icons-material/outline"; // good; can be added to project's IDE code snippets; ex. `.vscode/*.code-snippets`

import IconAccount from "@mui/icons-material/IconAccountOutline"; // bad

it will become a random mess (if used without a linter):

// example file header.ts
import IconAdd from "@mui/icons-material-symbols/IconAddOutline300"; // bad
// example file footer.ts
import IconAdd from "@mui/icons-material-symbols/IconAddOutline400"; // bad; oops imported a different weight in this file

It'll be much easier to mess up and miss such instances in the Symbols (with a lot more options). So you will force the community to use a linter

oliviertassinari commented 1 month ago

performance shouldn't be prioritized, DX should

@o-alexandrov Agree, it's the underlying goal.

icons leads to good enough results 80% & ~97% number of icons in barrel files

I don't understand this. Let's say developers shouldn't be able to import from @mui/icons-material but @mui/icons-material/sharp-100, this barrel file still has 6,000 imports, no? It won't be fast.

In any cases, the API direction of removing barrel import from the root and having sub barrel index could make sense. The challenge is to properly name them, while having the best DX. Like maybe it should be:

Or another layout that:

o-alexandrov commented 1 month ago

@oliviertassinari in my calculation, I assumed filled would be in a separate barrel. If you put filled and unfilled in the same barrel, then yes, the reduction would be smaller; for existing icons would be 60% whereas for symbols it’d be ~95% (100% - (3429 2 / 144,018) 100%).

Janpot commented 1 month ago

I offered you a solution by 5 times decreasing the number of modules to transpile and 63 times for newer icons. @Janpot ‘s "solution” is to remove the feature.

I appreciate the constructive feedback, but slight nuance, I'm not proposing any "solutions" here. I'm merely rejecting the idea that we should restructure the package for performance reasons.

I could get behind a restructure for improved DX. Some questions I have about the exact implementation for your proposal:

oliviertassinari commented 1 month ago

Do we keep the top-level barrel file?

No matter if we implement this. I think that the answer should be no, and we can't with Material Symbols.

Since under this proposal, multiple components with the exact same name are exported from the same package. Is there any risk in confusing intellisense/auto-import in vscode?

I would expect that they are all named in a global namespace.

Across our own docs, examples excluded, I see us using at least three different variants

The dark mode outline doesn't count, it's the filled={true} equivalent with Material Symbols. For the other two, we might want to normalizes them.

One thing that I expect is that the weight of the icons depends on the size we display the at, so a variation is expected to be used.