Alpha release breaks snowpack build

[vikigenius]

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Material UI 5.0.0-alpha.35 breaks SnowPack and throws error:
SyntaxError: indirect export not found: SliderMark

See discussion in FredKSchott/snowpack#3004 for details. Particularly comments: FredKSchott/snowpack#3004 (comment), FredKSchott/snowpack#3004 (comment), FredKSchott/snowpack#3004 (comment)

latest stable version of material-ui (4.11.4) seems to be working fine. So i was thinking maybe it would be easy to isolate what the problem is if I opened a bug report here.

Expected Behavior 🤔

MaterialUI should be able to work with SnowPack without issues.

Steps to Reproduce 🕹

Steps:

npx create-snowpack-app material-ui-test --template @snowpack/app-template-react-typescript
cd material-ui-test
npm install @material-ui/core@next @emotion/react @emotion/styled
echo "import { AppBar } from '@material-ui/core';" > src/App2.tsx
cat src/App.tsx >> src/App2.tsx
sed -ie 's/Learn React/<AppBar\/>/' src/App2.tsx
mv src/App2.tsx src/App.tsx
npm run start

Context 🔦

I am trying to test out the new styled solution provided in mui 5 using emotion so was taking a look at the latest alpha release and ran into this issue.

Your Environment 🌎

`npx @material-ui/envinfo`

npx: installed 2 in 3.123s

  System:
    OS: Linux 5.11 void
  Binaries:
    Node: 14.16.0 - /usr/bin/node
    Yarn: 1.22.10 - /usr/bin/yarn
    npm: 6.14.11 - /usr/bin/npm
  Browsers:
    Chrome: Not Found
    Firefox: 88.0
  npmPackages:
    @emotion/react: ^11.4.0 => 11.4.0
    @emotion/styled: ^11.3.0 => 11.3.0
    @material-ui/core: ^5.0.0-alpha.35 => 5.0.0-alpha.35
    @material-ui/private-theming:  5.0.0-alpha.35
    @material-ui/styled-engine:  5.0.0-alpha.34
    @material-ui/system:  5.0.0-alpha.35
    @material-ui/types:  6.0.1
    @material-ui/unstyled:  5.0.0-alpha.35
    @material-ui/utils:  5.0.0-alpha.35
    @types/react: ^17.0.4 => 17.0.9
    react: ^17.0.2 => 17.0.2
    react-dom: ^17.0.2 => 17.0.2
    typescript: ^4.2.4 => 4.3.2

[eps1lon]

Thanks for the report.

I can no longer reproduce the issue with @material-ui/[email protected] and [email protected].

[vikigenius]

@eps1lon It seems like it works fine in the prod build.

npm run build and then npx serve -s build seems to work fine.

But npm run start which starts the dev server is still throwing the same error.

[eps1lon]

@eps1lon It seems like it works fine in the prod build.

Thanks for the confirmation. Going to close this until there's something actionable for us to do. Until then I would defer to snowpack to fixing this issue. I don't think any other bundler has this particular issue.

[vikigenius]

@eps1lon This is a response from @drwpow in the snowpack thread FredKSchott/snowpack#3004 (comment)

👋🏻 Thanks so much everyone for reporting on this issue. I think with the most recent 4.x and 5.x versions of @material-ui/*, and the newest version of Snowpack, most of the issues are resolved? I’m still going through all the issues listed but for the most part I think support for this package in Snowpack has improved pretty drastically overall.

In general @material-ui/core is in a weird spot, as it ships ESM but doesn’t yet take the extra step of declaring subpath exports. This is critical not only for Snowpack, but the larger Node ecosystem as a whole. And because Snowpack depends so much on ESM support, is why you’re seeing differences between Snowpack vs, say, webpack, which is still largely stuck in the old CommonJS ecosystem.

If someone here could open an issue on the Material UI repo to implement Node’s new ESM guidelines, I think most if not all of the issues would be resolved. Not only for Snowpack users, but for Node ESM as a whole.

Is this something actionable? Adding the exports/guidelines as suggested?

[eps1lon]

Is this something actionable? Adding the exports/guidelines as suggested?

Since this will take a lot of time to get right and implements no new features other than new bundlers saying "we don't want to implement the old behavior", we're not prioritizing this.

We ship a lot of components, with a lot of subpaths and it's yet another thing we have to maintain since exports does not offer the proper flexibility that would help us. The syntax used in exports just makes things harder. And if we make it wrong node will just crash.

[drwpow]

We ship a lot of components, with a lot of subpaths and it's yet another thing we have to maintain since exports does not offer the proper flexibility that would help us. The syntax used in exports just makes things harder. And if we make it wrong node will just crash.

I can respect not wanting to ship a change that adds maintenance, and potential bugs. But if it makes it any easier, wildcard exports ("./*": "./esm/*") could potentially ease the maintenance burden.

implements no new features other than new bundlers saying "we don't want to implement the old behavior"

That’s not really Snowpack’s stance, at least. The reality is that there is no official “old behavior” for ESM to even implement, as "module" was never standardized and it doesn’t work with the way the documentation is currently written. Essentially, this is how users should be loading the ESM modules if they’d like to:

- import Button from '@material-ui/core/Button'; // CJS
+ import Button from '@material-ui/core/esm/Button/Button.js'; // ESM option 1: direct path
+ import { Button } from '@material-ui/core'; // ESM option 2: leveraging "module"

Really this comes down to either educating users how to import ESM vs CJS (docs), or doing it automatically for them (package exports). Either will work! But something has to change if a user is following the docs but wants ESM.

[eps1lon]

But if it makes it any easier, wildcard exports ("./*": "./esm/*") could potentially ease the maintenance burden.

Which are not the semantics we document. That is the problem here. We would just further confuse people because external tools parse the exports field and document internal modules. See the conversation in #26264 (comment)

The reality is that there is no official “old behavior” for ESM to even implement, as "module" was never standardized and it doesn’t work with the way the documentation is currently written.

https://nodejs.org/api/modules.html#modules_all_together looks official enough for me.

But something has to change if a user is following the docs but wants ESM.

I'm very pragmatic here. webpack 4, 5, nextjs, create-react-app, rollup, gatsby all just work right now. If snowpack wants to take the technical purity route, then more power to them. But the work required to be technically pure, while also maintaining backwards compat and not creating more issues, is not something that we have the bandwidth at the moment. We do want to get this right eventually. But right now the ecosystem is just not ready. And if I have to choose between snowpack and nextjs, I'll have to choose nextjs.