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] Publish packages in ESM module format #30671

Open
1 of 7 tasks
Tracked by #43938
Janpot opened this issue Jan 17, 2022 · 12 comments
Open
1 of 7 tasks
Tracked by #43938

[ESM] Publish packages in ESM module format #30671

Janpot opened this issue Jan 17, 2022 · 12 comments
Labels
breaking change enhancement This is not a bug, nor a new feature scope: code-infra Specific to the core-infra product

Comments

@Janpot
Copy link
Member

Janpot commented Jan 17, 2022

Why

How

@Janpot Janpot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer new feature New feature or request v6.x and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 17, 2022
@Janpot Janpot changed the title Publish packages as in ESM module format Publish packages in ESM module format Jan 17, 2022
@Janpot Janpot mentioned this issue Jan 17, 2022
2 tasks
@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Jan 20, 2022
@MicahZoltu
Copy link

Internal imports will need file extensions https://github.com/jaydenseric/babel-plugin-transform-require-extensions

I think this should probably be extracted out into a separate issue, as it would be a boon to using Material UI natively in a browser (without a bundler/transpiler), I think even without addressing the other things mentioned here (I could be wrong though).

@cseas
Copy link

cseas commented Feb 23, 2023

Publint is reporting that the components are written in ESM but getting interpreted as CJS.

Screenshot 2023-02-23 at 12 51 47 PM

I tried the MUI imports in a Next.js project with type: module and moduleResolution: node16 though, seems to work fine. (https://codesandbox.io/p/sandbox/angry-benji-oi33ry)

Is this because Next.js is reading the CJS exports under node/ directory instead of ESM ones?

@zbynekwinkler
Copy link

In order to be able to use @mui/icons-material in ES module (with remix), I had to use named import:

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

I believe it relies on the presence of "module": "./esm/index.js" field in the installed package.json which reexports everything. Reaching out to @mui/icons-material/LockOutlinedIcon like the examples do, fails.

@oliviertassinari oliviertassinari changed the title Publish packages in ESM module format [ESM] Publish packages in ESM module format Aug 12, 2023
@samuelsycamore
Copy link
Member

This seems to be a blocker for getting MUI components to play nicely with Remix. Named imports appear to work fine.

 import Box from "@mui/joy/Box";
 import { Box } from "@mui/material";

@cjoecker
Copy link
Contributor

As a workaround, you can do for now the following search-and-replace with regex to fix it:
Search import (.*)Icon from "@mui/icons-material/(.*)"; and replace with import { $1 as $1Icon } from "@mui/icons-material";

@DiegoAndai DiegoAndai added this to the Material UI: v6 milestone Dec 7, 2023
magjac added a commit to magjac/graphviz-visual-editor that referenced this issue Dec 9, 2023
magjac added a commit to magjac/graphviz-visual-editor that referenced this issue Dec 9, 2023
@DiegoAndai DiegoAndai removed this from the Material UI: v6 milestone Dec 12, 2023
@DiegoAndai DiegoAndai removed the v6.x label Dec 12, 2023
magjac added a commit to magjac/graphviz-visual-editor that referenced this issue Dec 16, 2023
Add .js

Add more .js

DEBUG: comment out text editor and all icon. Rendering ok now

add workaround for @mui/icons-material not published in ESM module format

See
mui/material-ui#30671 (comment).

add workaround for react-ace not published in ESM module format

See
securingsincity/react-ace#1540 (comment).
magjac added a commit to magjac/graphviz-visual-editor that referenced this issue Dec 16, 2023
Add .js

Add more .js

DEBUG: comment out text editor and all icon. Rendering ok now

add workaround for @mui/icons-material not published in ESM module format

See
mui/material-ui#30671 (comment).

add workaround for react-ace not published in ESM module format

See
securingsincity/react-ace#1540 (comment).
magjac added a commit to magjac/graphviz-visual-editor that referenced this issue Dec 16, 2023
Add .js

Add more .js

DEBUG: comment out text editor and all icon. Rendering ok now

add workaround for @mui/icons-material not published in ESM module format

See
mui/material-ui#30671 (comment).

add workaround for react-ace not published in ESM module format

See
securingsincity/react-ace#1540 (comment).
magjac added a commit to magjac/graphviz-visual-editor that referenced this issue Dec 16, 2023
Add .js

Add more .js

DEBUG: comment out text editor and all icon. Rendering ok now

add workaround for @mui/icons-material not published in ESM module format

See
mui/material-ui#30671 (comment).

add workaround for react-ace not published in ESM module format

See
securingsincity/react-ace#1540 (comment).
magjac added a commit to magjac/graphviz-visual-editor that referenced this issue Dec 17, 2023
Add .js

Add more .js

DEBUG: comment out text editor and all icon. Rendering ok now

add workaround for @mui/icons-material not published in ESM module format

See
mui/material-ui#30671 (comment).

add workaround for react-ace not published in ESM module format

See
securingsincity/react-ace#1540 (comment).
magjac added a commit to magjac/graphviz-visual-editor that referenced this issue Dec 17, 2023
Add .js

Add more .js

DEBUG: comment out text editor and all icon. Rendering ok now

add workaround for @mui/icons-material not published in ESM module format

See
mui/material-ui#30671 (comment).

add workaround for react-ace not published in ESM module format

See
securingsincity/react-ace#1540 (comment).
magjac added a commit to magjac/graphviz-visual-editor that referenced this issue Dec 18, 2023
Add .js

Add more .js

DEBUG: comment out text editor and all icon. Rendering ok now

add workaround for @mui/icons-material not published in ESM module format

See
mui/material-ui#30671 (comment).

add workaround for react-ace not published in ESM module format

See
securingsincity/react-ace#1540 (comment).
@akomm
Copy link
Contributor

akomm commented Feb 13, 2024

Output .cjs file, set it under package.json should make it possible use it with tsconfig esModuleInterop: true. Currently you can't do anything in esm configured project, because there is just a .js file in the build. And this .js file uses esm syntax.

Having this issue with the AdapterXYZ exports - date-pickers.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 26, 2024

Requirement for both CommonJS and ESM support from Docker: https://app.supernormal.com/posts/mui-docker-x-charts-6b722290c9c8013c6d350e8110019023

SCR-20240404-pida

@Janpot
Copy link
Member Author

Janpot commented Apr 4, 2024

Would be interested in what those infrastructure requirements are exactly. What mandates the need for a commonjs bundle? Does their bundler not understand ESM? Or does their SSR setup not compile modules on the server? It's an odd choice to try and fix this by adding a commonjs version to their chart library, rather than to force their bundler to transpile the dependency.

@DiegoAndai
Copy link
Member

DiegoAndai commented May 24, 2024

Sadly, I'm removing this issue from the v6 milestone, tentatively moving it to v7. I'll write an in-depth document on what we tried, why we're blocked, and the steps forward. As a summary, we found a blocker with Next.js Pages Router: vercel/next.js#65056. The probable path forward is removing the esmExternals: false from our documentation infra, but that's currently blocked by vercel/next.js#49898 (comment).

esmExternal was added in #37264 (comment).

@Janpot
Copy link
Member Author

Janpot commented Aug 8, 2024

Just recording some tips for anyone that wants to debug this:

  • Remove all pages and add just a single page with

    import * as React from 'react';
    import { DataGridPro } from '@mui/x-data-grid-pro';
    
    export default function MyPage() {
      return <DataGridPro rows={[]} columns={[]} />;
    }

    This will drastically speed up the build and reproduce the error (pnpm docs:build)

  • Disable the minifier to be able to inspect the bundles more easily. Look into the server bundles (.next/server/pages/...) If you've only got 1 page, there should be no code splitting happening.

    // next.config.mjs
      // ...
      webpack: (config, options) => {
        // ...
        config.optimization.minimize = false;
        config.optimization.minimizer = [];
        // ...

It looks like webpack isn't using the ESM bundle of @mui/x-data-grid. We may be able to fix this by:

  • Being strict about file extensions, our packages are type: 'commonjs', so our ESM files should have the .mjs extension.
  • For Node.js to be actually able to load these ESM files, we'll need to make sure the files contain only fully resolved module specifiers. This can't be done easily with our current build setup. Short term I see three potential solutions:
    • We rewrite every import to the full path without extension (so resolve index files). The add a babel plugin that adds a specific extension to each relative import. Open question: how to avoid regressions in the future? This needs to be linted somehow.
    • Build a babel plugin that resolves all relative imports and rewrites them with their full file name and extension (we can't just straight add the extension because of index.ts files)
    • Move to rollup for building (second snippet in this guide shows how to transpile a full set of files 1-to-1, just like we currently do with babel). Unlike babel, rollup is a bundler, so it should fully resolve every import. If we just compile each file 1 on 1, we can keep the type generation.

@akomm
Copy link
Contributor

akomm commented Aug 9, 2024

Currently the issue is maximized in the sense of blocking every possible simple workaround at once, by exporting .js, not .cjs (for interop workaround TS) or .mjs and at the same time using esm syntax in the .js build:

#30671 (comment)

main points to .js:
grafik

ESM syntax in .js output:
grafik

@oliviertassinari oliviertassinari added the scope: code-infra Specific to the core-infra product label Aug 30, 2024
@oliviertassinari oliviertassinari added breaking change enhancement This is not a bug, nor a new feature and removed v7.x core Infrastructure work going on behind the scenes new feature New feature or request labels Sep 29, 2024
@justin-hackin
Copy link

I've gotten used to copy-pasting icons from the website into my project. I forgot about this issue and accidentally broke my app in dev. That's why I went out of my way to prevent this from happening in the future by adding an eslint rule to prevent imports of the icons as in the copypasta:

       "no-restricted-imports": ["error", {
          "patterns": [{
            "group": ["@mui/icons-material/*"],
          }]
        }]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change enhancement This is not a bug, nor a new feature scope: code-infra Specific to the core-infra product
Projects
Status: Backlog
Development

No branches or pull requests

10 participants