-
-
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] icons exports structure for Material Symbols #42704
Comments
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. |
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.
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. |
Regarding 1.
To note that currently it's already possible to enforce this with 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. |
@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. |
Thank you for the ESLint rule, I’ll try it out. 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 |
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 #43412 |
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 For instance I could achieve a similar behavior in |
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. |
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
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). |
@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 |
@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.
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.
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'm not following how this works. It looks like it's even slower: barrel that imports other barrels? |
It should be
You always use a barrel file for
I've never encountered a website as a user, and as a developer where I've seen/used more than 1 icon variant. The proposal of this issue is to have a separate barrel file per use case; i.e. you have
|
@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 #22529 (comment). 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
Right, this will be an interesting DX tradeoff decision to make 😁.
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. |
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. 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). |
@o-alexandrov Agree, it's the underlying goal.
I don't understand this. Let's say developers shouldn't be able to import from 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:
|
@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%). |
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:
|
No matter if we implement this. I think that the answer should be no, and we can't with Material Symbols.
I would expect that they are all named in a global namespace.
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. |
I started to think, keeping 1 file that re-exports icons with nested imports from mui or another package with icons is a good enough workaround until bundlers get faster which would probably happen next year. |
Reopening as we need to solve import paths for Material Symbols #32846 |
What's the problem?
This RFC looks at 2 problems with icons:
Sharp
,Rounded
,Outlined
,TwoTone
,Filled
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:
Related issues
Search keywords: material icons, symbols, icons v2
The text was updated successfully, but these errors were encountered: