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

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

Open
2 tasks done
macrozone opened this issue Nov 22, 2022 · 17 comments
Open
2 tasks done

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

macrozone opened this issue Nov 22, 2022 · 17 comments
Assignees
Labels
enhancement This is not a bug, nor a new feature package: icons Specific to @mui/icons

Comments

@macrozone
Copy link

macrozone commented Nov 22, 2022

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

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
@macrozone macrozone added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 22, 2022
@zannager zannager added the package: icons Specific to @mui/icons label Nov 23, 2022
@michaldudak michaldudak self-assigned this Dec 6, 2022
@michaldudak
Copy link
Member

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.

@michaldudak michaldudak added enhancement This is not a bug, nor a new feature and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Dec 9, 2022
@michaldudak michaldudak added this to the v6 milestone Dec 9, 2022
@arobert93
Copy link

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

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

Janpot commented Dec 9, 2022

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 #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

@arobert93
Copy link

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

Janpot commented Dec 9, 2022

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

@arobert93
Copy link

arobert93 commented Dec 9, 2022

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

Janpot commented Dec 9, 2022

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

4-1-8 commented Feb 21, 2023

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

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

Janpot commented Jun 7, 2023

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

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],
    ...
}

@oliviertassinari oliviertassinari changed the title cannot import @mui/icons-material/XXX with ESM @mui/icons-material move to pure ESM Jul 24, 2023
@oliviertassinari oliviertassinari changed the title @mui/icons-material move to pure ESM [ESM] @mui/icons-material move to pure ESM Aug 12, 2023
@Janpot
Copy link
Member

Janpot commented Aug 29, 2023

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

oliviertassinari commented Aug 31, 2023

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

hmd-ai commented Sep 12, 2023

I'm facing a similar issue, I summarised it here: #31835 (comment)

@aaronadamsCA
Copy link
Contributor

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

Moving to v7's milestone as per: #30671 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is not a bug, nor a new feature package: icons Specific to @mui/icons
Projects
Status: Backlog
Development

No branches or pull requests