[ESM] @mui/icons-material move to pure ESM

macrozone commented 1 year ago

Steps to reproduce 🕹

I try to convert a NPM module to pure ESM (with typescript).

It occasionally uses icons from @mui/icons-material

We usually had imports like this:

import DeleteIcon from '@mui/icons-material/Delete'

this no longer works, DeleteIcon is not an Component, but an object with the default property.

this however works:

import {default as DeleteIcon} from '@mui/icons-material/Delete'

and this as well (probably the recommended way anyway)

import {Delete as DeleteIcon} from '@mui/icons-material'

Current behavior 😯

import DeleteIcon from '@mui/icons-material/Delete'

DeleteIcon is not an Component, but an object with the default property.

Expected behavior 🤔

import DeleteIcon from '@mui/icons-material/Delete'

DeleteIcon is a Component and works out of the box

Context 🔦

I maintain https://github.com/react-page/react-page and have a problem there currently with latest nextjs in combination with some libraries and therefore try to move everything to pure ESM.

the ESM transition is pretty rough and combined with typescript its surprisingly complicated. Its unclear whether the above problem is related to the config or to @mui/icons-material, or both.

I see that not all npm packages have this problem though.

Your environment 🌎

npx @mui/envinfo

``` System: OS: macOS 13.0 Binaries: Node: 14.21.1 - ~/.nvm/versions/node/v14.21.1/bin/node Yarn: 1.22.19 - /usr/local/bin/yarn npm: 6.14.17 - ~/.nvm/versions/node/v14.21.1/bin/npm Browsers: Chrome: 107.0.5304.110 Edge: Not Found Firefox: 99.0.1 Safari: 16.1 ```

michaldudak commented 1 year ago

We plan to properly support ES modules (file extensions in imports, package.json exports, etc.) in the upcoming version (v6) of Material UI. Changing anything in this area before then is likely a no-go, as the changes could break existing applications.

acomanescu commented 1 year ago

@michaldudak What about adding a package.json file inside esm folder with only:

{
    "type": "module"
}

I've done this for a package and I was able to make it work without using the exports inside the main package.json file.

michaldudak commented 1 year ago

I suppose we could do this. @macrozone, would this alone help in your case? It won't make the output valid for browsers or Node (as the import statements lack extensions), but tools with less strict rules should be happy.

cc @mui/code-infra for thoughts.

Janpot commented 1 year ago

I may be mistaken, but I believe Next.js during SSR would try to load it untranspiled as ESM on node.js and fail on the missing file extensions and missing package.json exports. I think this would break a lot of existing code. I also remember this breaking with React 17. (It's been a while since I've looked into https://github.com/mui/material-ui/issues/30671 and my memory is a bit cloudy, so treat this comment mainly as potential scenarios to test.)

Extensions are relatively easy to support
Breaking React compatibility may be a bigger problem I suppose (Didn't we recently have a conversation to bring back support for React 16?). Although I assume the overlap between users that have react@<18 users and next@>=12 is quite small. We may get away with this.
I also expect trouble with some dependencies like react-transition-group as there will be an ESM module trying to import a non-ESM package. So these need to be converted to ESM as well.

@arobert93 I can't seem to load @untitled-ui/icons-react in Next.js btw, see https://stackblitz.com/edit/nextjs-t7g2gq?file=pages%2Findex.js

acomanescu commented 1 year ago

@Janpot For some reason you need to run yarn add @babel/runtime. I'll try to investigate it tomorrow, but it works on in my local projects without installing this package.

Janpot commented 1 year ago

@arobert93 Still can't load it when imported as

import Activity from '@untitled-ui/icons-react/buid/esm/Activity';

https://stackblitz.com/edit/nextjs-n1kkod?file=pages%2Findex.js

I believe the reason is that "type": "module" makes it to load as native ESM on node.js, and node.js is refusing to expose anything from the package that is not described in package.json exports.

acomanescu commented 1 year ago

@Janpot there's a typo in your import path:

Replace:

import Activity from '@untitled-ui/icons-react/buid/esm/Activity';

With:

import Activity from '@untitled-ui/icons-react/build/esm/Activity';

Janpot commented 1 year ago

🤷 I copied that straight from the README

edit:

Ok, looking over the docs again

Like in CommonJS, module files within packages can be accessed by appending a path to the package name unless the package's package.json contains an "exports" field, in which case files within packages can only be accessed via the paths defined in "exports".

It seems I was conflating not having "exports" with not having a subpath defined in "exports". Personally, it's still a change I'd make over a major, but it could be ok.

4-1-8 commented 1 year ago

Mmm, we have this specific issue, and because the v6 with ESM support is TBA, we will have to leave MUI for our project. Bummer.

libasoles commented 1 year ago

@Janpot I'm curious how you solved this. My issue is: either it works in Next.js or in Jest. I can't have both working at a time.

My stack involves Next.js 13, typescript, ts-jest and jest. Commonjs modules vs ES6 is a challenging thing to setup with this stack man.

Janpot commented 1 year ago

Not sure whether it will help you, but in vite I've had some success with

    resolve: {
      alias: [
        {
          // FIXME(https://github.com/mui/material-ui/issues/35233)
          find: /^@mui\/icons-material\/(?!esm\/)([^/]*)/,
          replacement: '@mui/icons-material/esm/$1',
        },
      ],
    },

Which forces the bundler to import the ESM target for import AddIcon from '@mui/icons-material/Add'.

edit: I changed the regex from /^@mui\/icons-material\/([^/]*)/ to /^@mui\/icons-material\/(?!esm\/)([^/]*)/ to account for packages like @devexpress/dx-react-grid-material-ui that import straight from the /esm/ folder.

literallylara commented 1 year ago

Here is a workaround for esbuild based on @Janpot's answer:

// FIXME(https://github.com/mui/material-ui/issues/35233)
const fixMuiIconsMaterialImports = {
    name: 'fix @mui/icons-material imports',
    setup(build) {
        build.onResolve(
            { filter: /^@mui\/icons-material\/([^/]*)$/ },
            async args => {
                const name = args.path.split('/').pop()

                const result = await build.resolve(
                    `@mui/icons-material/esm/${name}.js`,
                    {
                        kind: 'import-statement',
                        resolveDir: args.resolveDir,
                    },
                )

                if (result.errors.length > 0) {
                    return { errors: result.errors }
                }

                return { path: result.path }
            },
        )
    },
}

Just add it to your build's plugins array:

{
    ...
    plugins: [fixMuiIconsMaterialImports],
    ...
}

Janpot commented 1 year ago

We ran into the following issue in Toolpad a while ago:

Investigated a bit and I found that the issue disappears when I turn the @mui/icons-material package full ESM. i.e. adding the following to ./node_modules/@mui/icons-material/package.json:

  "type": "module",
  "exports": {
    ".": {
      "types": "./index.d.ts",
      "import": "./esm/index.js",
      "require": "./index.js"
    },
    "./*": {
      "types": "./*.d.ts",
      "import": "./esm/*.js",
      "require": "./*.js"
    }
  },

The problem can be reproduced by cloning this project. and repeatedly running
rm -rf node_modules/.vite && yarn dev
until the error logs in the browser. After applying the update to ./node_modules/@mui/icons-material/package.json I can't reproduce it anymore.

oliviertassinari commented 1 year ago

Note that we tried to move the icons package to ESM only in #26310. We reverted in #26656. I think we should test that we are not breaking other bundlers with this change 😁

hmd-ai commented 1 year ago

I'm facing a similar issue, I summarised it here: https://github.com/mui/material-ui/issues/31835#issuecomment-1715094599

aaronadamsCA commented 1 year ago

This is a really tricky issue because the TypeScript types currently indicate this is safe:

import DeleteIcon from "@mui/icons-material/Delete";

<DeleteIcon />

But this causes esbuild to produce code that crashes the React renderer:

Warning: React.jsx: type is invalid -- expected a string (for built-in components) or a class/function (for composite components) but got: object.

DiegoAndai commented 5 months ago

Moving to v7's milestone as per: https://github.com/mui/material-ui/issues/30671#issuecomment-2130387754

sjdemartini commented 2 months ago

The above esbuild workaround didn't work for me, but I found this esbuild plugin (in the popular https://github.com/refinedev/refine project, where they use it with tsup) https://github.com/refinedev/refine/blob/4c34444633a252fd830fc3ddbc5963e112269e2f/packages/shared/mui-icons-material-esm-replace-plugin.ts which does the trick.

github-actions[bot] commented 2 months ago

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

[!NOTE] We value your feedback @macrozone! How was your experience with our support team? If you could spare a moment, we'd love to hear your thoughts in this brief Support Satisfaction survey. Your insights help us improve!

mui / material-ui