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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[icons-material] Add types to ESM icons #38742

Closed
wants to merge 1 commit into from

Conversation

Methuselah96
Copy link
Contributor

Context

I am encountering this issue. I have narrowed it down to an issue with esbuild that only occurs in this case when CJS and ESM are mixed. In order to workaround this I would like to switch to using the icons in @mui/icons-material/esm, but they have no types.

Changes

Add types to the icons in @mui/icons-material/esm directory so they can be imported in a TypeScript project.

@mui-bot
Copy link

mui-bot commented Aug 31, 2023

Netlify deploy preview

https://deploy-preview-38742--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 79873ce

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 31, 2023

I don't think we can merge this, direct imports from /esm are not supported. It's a private API.

Developers are strongly discouraged to import from any of the other bundles directly.

https://mui.com/material-ui/guides/minimizing-bundle-size/#available-bundles

Would the proposal there solve the issue?

Instead, use these bundles at the bundler level with e.g Webpack's resolve.alias:

@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Aug 31, 2023

I see, thanks for the reply. It seems odd that you can get away with @mui/material being ESM by default, but not @mui/icons-material. I look forward to @mui/icons-material being ESM by default as well.

Aliasing is fine, but that means I need to alias for Vite, Webpack, Jest, and anything else that needs to know how to resolve stuff. It's a bit of a pain.

I will patch the package.json of @mui/icons-material for now as demonstrated here: #35233 (comment).

@Methuselah96 Methuselah96 deleted the icons-material-types branch August 31, 2023 21:50
@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Aug 31, 2023

Turns out neither of those are options for me because @devexpress/dx-react-grid-material-ui is depending on @mui/icons-material/esm, so trying to alias or patch the exports causes those to not work because they then get resolved to @mui/icons-material/esm/esm.

@Methuselah96
Copy link
Contributor Author

@oliviertassinari Any suggestions on how to proceed?

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 31, 2023

@devexpress/dx-react-grid-material-ui is depending on @mui/icons-material/esm

They might get surprises, we might rename the esm folder from one release to another 😅.

@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Sep 1, 2023

I worked around this by adding more export conditions. Looking forward to MUI exposing ESM that can be picked up automatically by tools.

@Janpot
Copy link
Member

Janpot commented Sep 1, 2023

Turns out neither of those are options for me because @devexpress/dx-react-grid-material-ui is depending on @mui/icons-material/esm, so trying to alias or patch the exports causes those to not work because they then get resolved to @mui/icons-material/esm/esm.

@Methuselah96 Does it work when you use a negative lookahead to exclude the esm folder from being aliased?

resolve: {
  alias: [
    {
      find: /^@mui\/icons-material\/(?!esm\/)([^/]*)/,
      replacement: '@mui/icons-material/esm/$1',
    },
  ],
},

(/^@mui\/icons-material\/([^/]*)$/ should also work 🤔 )

@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Sep 1, 2023

@Janpot Probably, thanks for the tip, but I have multiple tools that need to know about this alias, so I didn't want to have to declare it each one. I ended up patching the package.json of @mui/icons-material to include an exports field that knows about @mui/icons-material/esm as the most straightforward solution.

@BennyAlex
Copy link

Please add the esm option otherwise vite fails when using mui with
#32727

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: icons Specific to @mui/icons
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants