material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[code-infra] Update package layout for better ESM support

Open Janpot opened this issue 1 year ago • 1 comments

Add a new mode to the build:

  • Removes nested package.json files
  • Stores ESM files with .mjs extension
  • Add exports field to package.json
  • Leave icon package layout as is, but add exports field. Next time we generate it we'll have to update the exports field
  • Use rollup instead of babel to compile the modules. this makes it fully specify imports

Notes:

  • added ./esm/* export to icons for people who used the vite alias workaround
  • mjs extension and package.json exports are supported from node 12.17 onward
  • Our ESM bundles don't work in Next.js due to
    ../../packages/mui-system/build/Box/index.mjs
    Error: It's currently unsupported to use "export *" in a client boundary. Please use named exports instead.
        at Object.transformSource (/Users/janpotoms/Projects/material-ui/node_modules/.pnpm/[email protected]_@[email protected]_@[email protected]_@[email protected]_babel-plugin-_fn2ktr3o4nwwaygw7yjiv6t4d4/node_modules/next/dist/build/webpack/loaders/next-flight-loader/index.js:87:31)
    
    Import trace for requested module:
    ../../packages/mui-system/build/Box/index.mjs
    ../../packages/mui-system/build/index.mjs
    ../../packages/mui-material/build/styles/index.mjs
    ./src/app/layout.tsx
    
    https://github.com/mui/material-ui/actions/runs/10578363192/job/29308312240?pr=43264#step:7:1960
  • Another issue that is likely caused by our abismal ESM support https://github.com/mui/material-ui/issues/37934

To Do:

  • [x] bundle size increase - explanation below
  • [x] make sure babel helpers are a dependency everywhere https://github.com/mui/material-ui/pull/43473#event-14030760284
  • [ ] ~figure out the export * usage in files that have a 'use client' pragma.~ Blocked on https://github.com/mui/material-ui/pull/41956
  • [ ] investigate why it's resolving to cjs @mui/material-pigment-css
  • [ ] export modern target
  • [ ] merge exports fields instead of overwrite
    • [ ] update @mui/aterial-pigment-css and @mui/icons-material

Bundle size increase

The bundle size increase can be fully attributed to the inclusion of the following ESM/CJS interop helper in the webpack output:

Code
/******/ 	/* webpack/runtime/create fake namespace object */
/******/ 	(() => {
/******/ 		var getProto = Object.getPrototypeOf ? (obj) => (Object.getPrototypeOf(obj)) : (obj) => (obj.__proto__);
/******/ 		var leafPrototypes;
/******/ 		// create a fake namespace object
/******/ 		// mode & 1: value is a module id, require it
/******/ 		// mode & 2: merge all properties of value into the ns
/******/ 		// mode & 4: return value when already ns object
/******/ 		// mode & 16: return value when it's Promise-like
/******/ 		// mode & 8|1: behave like require
/******/ 		__webpack_require__.t = function(value, mode) {
/******/ 			if(mode & 1) value = this(value);
/******/ 			if(mode & 8) return value;
/******/ 			if(typeof value === 'object' && value) {
/******/ 				if((mode & 4) && value.__esModule) return value;
/******/ 				if((mode & 16) && typeof value.then === 'function') return value;
/******/ 			}
/******/ 			var ns = Object.create(null);
/******/ 			__webpack_require__.r(ns);
/******/ 			var def = {};
/******/ 			leafPrototypes = leafPrototypes || [null, getProto({}), getProto([]), getProto(getProto)];
/******/ 			for(var current = mode & 2 && value; typeof current == 'object' && !~leafPrototypes.indexOf(current); current = getProto(current)) {
/******/ 				Object.getOwnPropertyNames(current).forEach((key) => (def[key] = () => (value[key])));
/******/ 			}
/******/ 			def['default'] = () => (value);
/******/ 			__webpack_require__.d(ns, def);
/******/ 			return ns;
/******/ 		};
/******/ 	})();

This is caused by the following import https://github.com/mui/material-ui/blob/eab1b9e8cd01ba4693d5c45a3134159f12f81012/packages/mui-system/src/useMediaQuery/useMediaQuery.ts#L75

A potential switch to using use-sync-external-store has been explored in https://github.com/mui/material-ui/pull/43476 but ultimately it makes sense to keep it, it manifests in the webpack runtime.

Janpot avatar Aug 11 '24 12:08 Janpot

Netlify deploy preview

https://deploy-preview-43264--material-ui.netlify.app/

Menu: parsed: +3.73% , gzip: +3.83% @material-ui/unstyled: parsed: +0.92% , gzip: +1.01% Select: parsed: +3.16% , gzip: +3.17% Unstable_Popup: parsed: +3.05% , gzip: +2.63% Unstable_NumberInput: parsed: +5.11% , gzip: +5.90% @material-ui/core/useMediaQuery: parsed: +5.09% , gzip: +4.53% MenuItem: parsed: +6.67% , gzip: +6.83% Option: parsed: +6.97% , gzip: +7.37% Tab: parsed: +7.47% , gzip: +7.53% TabPanel: parsed: +12.28% , gzip: +11.45% useAutocomplete: parsed: +5.06% , gzip: +5.47% TablePagination: parsed: +7.54% , gzip: +8.23% Tooltip: parsed: +0.44% , gzip: +0.48% SpeedDialAction: parsed: +0.37% , gzip: +0.40% @mui/joy/Menu: parsed: +0.41% , gzip: +0.50% TextField: parsed: +0.34% , gzip: +0.31% @mui/joy/Tooltip: parsed: +0.52% , gzip: +0.51% RadioGroup: parsed: +0.68% , gzip: +0.73% @mui/joy/ListSubheader: parsed: +0.77% , gzip: +0.72% LoadingButton: parsed: +0.51% , gzip: +0.49% and 27 more changes

Bundle size report

Details of bundle changes (Toolpad) Details of bundle changes

Generated by :no_entry_sign: dangerJS against ab5dcce4f544e7ddc8c9dbf5713175fcd1827b97

mui-bot avatar Aug 11 '24 12:08 mui-bot

  • Why create a subfolder for ESM, instead of maintaining the current convention where the subfolder is for CJS and ESM is at the root?

To be able to mark all .js files in esm/ as ESM with a package.json containing type: 'module'. It's interesting to be able to keep the .js extension because that means we can just copy the typings from one folder to the other. Also to keep some parity with base UI.

  • Can (should) we create tests for the package layout/build process? To ensure, for example, that the ATTW check is not broken, or that the package layout is not changed unknowingly.

I also fixed up the existing bundler tests and added an extra case for the next.js issue we saw.

Janpot avatar Jan 24 '25 15:01 Janpot

Thanks for replying to my questions @Janpot. Everything looks good from my side. Feel free to mark this as ready for review whenever you're ready.

DiegoAndai avatar Jan 27 '25 19:01 DiegoAndai

@DiegoAndai Only problem left seems to be that the github action that builds @apps/next-app has become a lot heavier on macos. It takes about twice the time to run in CI, locally it runs out of memory for me (have to use NODE_OPTIONS="--max-old-space-size=8192"). windows/linux seem to be fine. cc @brijeshb42 in case you have an idea?

Janpot avatar Jan 28 '25 10:01 Janpot

What we can do is disable this app temporarily in this repo and move it to Pigment where I can take a look later to unblock.

brijeshb42 avatar Jan 28 '25 12:01 brijeshb42

What we can do is disable this app temporarily in this repo and move it to Pigment where I can take a look later to unblock.

I think it would be better first to investigate if this is due to the next-app's specific config or if this would impact everyone using next with this updated package layout.

So, there is no rush to take it out now, but let's work together to find the root cause.

DiegoAndai avatar Jan 28 '25 12:01 DiegoAndai

What's the average build time on master/other PRs ?

Let me check this locally.

brijeshb42 avatar Jan 28 '25 14:01 brijeshb42

What's the average build time on master/other PRs ?

I don't have the specifics for the next-app, but:

The last 5 master test-dev (macos) builds (pnpm build:ci) took between 4 and 7 minutes

The last one on this PR was over 2 hours, the one before that was 14 minutes.

There's definitely something going on.

DiegoAndai avatar Jan 28 '25 14:01 DiegoAndai

Just checked locally on both master and ^ this branch. master built in 2 mins but this branch's build failed with some heap memory error. I'll have to bisect commit by commit to identify the issue. I'll update once I have some result.

brijeshb42 avatar Jan 28 '25 16:01 brijeshb42

@Janpot what was the issue?

brijeshb42 avatar Jan 29 '25 13:01 brijeshb42

It's not solved yet, this action just took 1 hour to run.

Janpot avatar Jan 29 '25 21:01 Janpot

I think it would be better first to investigate if this is due to the next-app's specific config or if this would impact everyone using next with this updated package layout.

I tried removing pigment from the app, but kept as much as possible of the original pages and it reduces runtime drastically, see https://github.com/mui/material-ui/actions/runs/13055965609/job/36426957328?pr=45152 This suggests it's caused by some combination of pigment css with the new package layout. It's also almost 3 to 4 times faster than master, goes to show the overhead it could create.

Janpot avatar Jan 30 '25 15:01 Janpot

@Janpot which option should we follow:

  1. Investigate the issue before merging
  2. Remove the Pigment build and investigate separately

?

I worry that the increase in build time might uncover some edge cases with the new layout we haven't seen. But if you think it's more likely to be caused by Pigment's configuration, then we could unblock this and investigate separately. This would get the layout change to users faster, so we got feedback earlier.

This suggests it's caused by some combination of pigment css with the new package layout. It's also almost 3 to 4 times faster than master, goes to show the overhead it could create.

I didn't understand this.

DiegoAndai avatar Jan 30 '25 17:01 DiegoAndai

While trying to reproduce this locally, on 1-2 runs, it resulted in heap error. But on building again, it built successfully within 3 mins. I thought it'll be easy to find the bug with git bisect but based on the unpredictability of the build, I'd suggest to remove this from the build pipeline. I can add it back when updating the code for Pigment v1.

brijeshb42 avatar Feb 04 '25 09:02 brijeshb42

The pigment app has been very useful as an integration test, so I don't want to just rip it out. I'm adding a version of it with pigment removed which can serve as the integration test for now. We can remove once the issues are resolved. wdyt?

Janpot avatar Feb 04 '25 09:02 Janpot

Yeah. That works as well.

brijeshb42 avatar Feb 04 '25 09:02 brijeshb42

@Janpot may I ask you to create an issue so we can track the debugging of the Pigment Next App issue? That way, we could add it to the backlog to avoid forgetting about it.

Added in the pigment css repo: https://github.com/mui/pigment-css/issues/376

Janpot avatar Feb 05 '25 09:02 Janpot