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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ESM] Feature a distribution compatible with Node in "type": "module". #37335

Open
2 tasks done
garronej opened this issue May 20, 2023 · 7 comments
Open
2 tasks done
Assignees
Labels
enhancement This is not a bug, nor a new feature package: material-ui Specific to @mui/material scope: code-infra Specific to the core-infra product

Comments

@garronej
Copy link
Contributor

garronej commented May 20, 2023

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 馃暪

Hi MUI team,

Steps to Reproduce:

You can reproduce the problem using the below repository:

git clone https://github.com/garronej/mui-import-esm  
cd mui-import-esm
yarn install
node index.mjs

Current behavior 馃槸

Node is unable to import MUI in the type: "module" environment.

We're getting Error [ERR_UNSUPPORTED_DIR_IMPORT]: Directory import '/Users/joseph/github/mui-import-esm/node_modules/@mui/material/styles' when importing any submodule of @mui/material.

Expected behavior 馃

MUI should be correctly imported when running under Node's ESM environment.

Context 馃敠

As the publisher of modules that depend on MUI, such as tss-react and onyxia-ui, I've encountered some problems with distributing real ECMAScript Modules (ESM) due to MUI's lack of a functional ESM distribution compatible with a "type": "module" node environment. While MUI does provide an ESM distribution that works with bundlers, it unfortunately fails to do the same for node.

Proposed Solutions:

To resolve the above issue, an additional entry to node_modules/@mui/material/package.json could potentially fix the problem:

  "exports": {
    "./styles": {
      "default": "./node/styles/index.js"
    },
    "./Button": {
      "default": "./node/Button/index.js"
    }
  }

Implementing this works for import { useStyles } from "@mui/material/styles" but doesn't function correctly for import Button from "@mui/material/Button". This issue arises as Button is represented as an object, rather than as the default function.

For the above scenario, there are two possible strategies:

  • Adopt @Andarist's Emotion strategy: Expose a .mjs file that reexports everything from the CommonJS ESModuleInterop distribution (See an example here). This strategy allows for correct defaults. And avoiding the dual package hazard. MUI also stands to gain nothing by being a real ESM, as it does not support lazy imports.

  • Publish a full, standards-compliant ESM distribution: Similar to tsafe.

The popularity of this comment based on its upvotes demonstrates that other developers are experiencing similar issues.

Regardless, I appreciate all the hard work done by the MUI team. Thank you!

Your environment 馃寧

No response

@garronej garronej added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label May 20, 2023
@Andarist
Copy link
Contributor

Implementing this works for import { useStyles } from "@mui/material/styles"

This is true but at the same time, this is also possible in node and I'd consider this to be a problem:

import ns from "@mui/material/styles"
ns.useStyles

@garronej
Copy link
Contributor Author

I'd consider this to be a problem

Really? I'm surprised that you think it's a problem.
To achieve total compliance with the standard upon importing the default from @mui/material/styles, we should encounter the following runtime error:

import ns from "@mui/material/styles"
            ^^^
SyntaxError: The requested module '@mui/material/styles' does not provide an export named 'default'

However, bear in mind that allowSyntheticDefaultImports is enabled by default in TypeScript. Thus, TypeScript won't raise any objections when you attempt to import it in this manner.

image

Publish a full, standards-compliant ESM distribution: Similar to tsafe.

Following further examination, I recommend not implementing this option, even if it appears to be the sensible path. @Andarist has made the correct choice. Publishing an actual Node compliant ESM distribution will result in situations where MUI is duplicated. If it helps, I can demonstrate this through a repository.

@garronej
Copy link
Contributor Author

garronej commented May 29, 2023

Upon conducting a thorough investigation, it appears there isn't a straightforward solution to this problem there will be a solution once Anderist's PR on Vite gets merged.

Not providing a distribution compatible with the "type: module" mode of Node.js isn't ideal. However, there's also no apparent way (to my knowledge) to provide this distribution without generating the potential risk of a dual package hazard.

For further illustration, I've created a repository where I demonstrate an issue present in Emotion but not in MUI. Here's the link for your reference: https://github.com/garronej/vite-dual-package-repro-repo.

@Andarist
Copy link
Contributor

This particular issue is a Vite bug, you can track the fix here: vitejs/vite#13370

@michaldudak
Copy link
Member

Hey guys, sorry for the late response - I was on vacation.
I'm looking forward to the day when everything will be ESM, and we won't have such problems.
I wonder what the impact would be for the community if we shipped just pure ESM. I think this is something we need to consider for the future (cc @Janpot, @alexfauquette, @flaviendelangle).

As for this particular thread, if I read it correctly, once Vite 4.4 is released, the problem will be solvable, right?

@michaldudak michaldudak removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jun 14, 2023
@Andarist
Copy link
Contributor

As for this particular thread, if I read it correctly, once Vite 4.4 is released, the problem will be solvable, right?

Yes, it should be solvable. Note that solving this in full on your side requires some serious package.json#exports gymnastics but it's doable - as far as I could verify it. Emotion shipped using this strategy and so far we didn't quite get any issue reports about this, apart from the mentioned Vite issue and the one that you have found here. The first one (Vite issue) is supposed to get fixed soon (in the mentioned 4.4 release) but we are still waiting for any kind of response from the Vercel team. Note that the Next.js issue isn't affecting everybody - you have to mix ESM and CJS files in a particular way to end up with that issue and it's not something that people do often.

I wonder what the impact would be for the community if we shipped just pure ESM. I think this is something we need to consider for the future (cc @Janpot, @alexfauquette, @flaviendelangle).

Likely... it could be quite severe 馃槄 and that's definitely not something that you can consider within the current major version. I'm also looking for ESM days ahead of us but, quite frankly, I'm rather afraid of paving the way with my libraries and I'm waiting for other people to start doing that at scale to join them at some point.

@Janpot
Copy link
Member

Janpot commented Jun 14, 2023

Maybe not an ideal solution, but none of them are, so for completeness: To mitigate dual package problems we could also consider adding a check at runtime and warn/error when a module is loaded that has a different format than the previous loaded ones.

@oliviertassinari oliviertassinari changed the title Feature a distribution compatible with Node in "type": "module". [ESM] Feature a distribution compatible with Node in "type": "module". Aug 12, 2023
@michaldudak michaldudak added the enhancement This is not a bug, nor a new feature label Aug 31, 2023
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: material-ui Specific to @mui/material scope: code-infra Specific to the core-infra product
Projects
None yet
Development

No branches or pull requests

6 participants