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

[core] Remove outdated Babel plugins

Open ZeeshanTamboli opened this issue 1 year ago • 11 comments

Part of #40958.

With the updated browser support, particularly for the new Safari browser, it seems we no longer require these plugins. Previous attempted PRs mentioned in #40958.

Additionally, it appears that the assumptions feature of Babel is also not needed. This closes #37461.

The plugins are now included by default in the latest version of @babel/preset-env. They are applied based on the browsers listed in browserslist, with Babel maintaining a mapping of versions for each plugin here.

browserstack-force run for Safari: https://app.circleci.com/pipelines/github/mui/material-ui/128548/workflows/5eb395f3-c3f8-4e63-bdd3-f51a5e790419/jobs/693108

Bundle size reduction.

ZeeshanTamboli avatar May 06 '24 07:05 ZeeshanTamboli

Netlify deploy preview

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

@material-ui/core: parsed: -3.86% :heart_eyes:, gzip: -3.80% :heart_eyes: @mui/joy: parsed: -3.00% :heart_eyes:, gzip: -3.12% :heart_eyes: @material-ui/lab: parsed: -2.90% :heart_eyes:, gzip: -3.08% :heart_eyes: TextField: parsed: -3.63% :heart_eyes:, gzip: -2.95% :heart_eyes: @material-ui/unstyled: parsed: -2.86% :heart_eyes:, gzip: -2.70% :heart_eyes: @mui/joy/Autocomplete: parsed: -2.18% :heart_eyes:, gzip: -2.19% :heart_eyes: Autocomplete: parsed: -2.32% :heart_eyes:, gzip: -2.10% :heart_eyes: SpeedDialAction: parsed: -2.18% :heart_eyes:, gzip: -1.95% :heart_eyes: SwipeableDrawer: parsed: -2.74% :heart_eyes:, gzip: -2.23% :heart_eyes: @mui/joy/Tooltip: parsed: -2.40% :heart_eyes:, gzip: -2.42% :heart_eyes: @mui/joy/Select: parsed: -2.01% :heart_eyes:, gzip: -1.45% :heart_eyes: @mui/joy/Checkbox: parsed: -2.66% :heart_eyes:, gzip: -2.76% :heart_eyes: Popover: parsed: -2.71% :heart_eyes:, gzip: -2.02% :heart_eyes: @mui/joy/Textarea: parsed: -2.77% :heart_eyes:, gzip: -2.72% :heart_eyes: @mui/joy/ChipDelete: parsed: -2.38% :heart_eyes:, gzip: -2.51% :heart_eyes: @mui/joy/ModalClose: parsed: -2.31% :heart_eyes:, gzip: -2.48% :heart_eyes: @mui/joy/Input: parsed: -2.69% :heart_eyes:, gzip: -2.71% :heart_eyes: @mui/joy/MenuButton: parsed: -2.32% :heart_eyes:, gzip: -2.24% :heart_eyes: Drawer: parsed: -2.53% :heart_eyes:, gzip: -1.88% :heart_eyes: @mui/joy/Radio: parsed: -2.51% :heart_eyes:, gzip: -2.69% :heart_eyes: and 217 more changes

Bundle size report

Details of bundle changes (Toolpad) Details of bundle changes

Generated by :no_entry_sign: dangerJS against f84b8c0456c645cff33f4ef3ee2af5ee6f3bfe58

mui-bot avatar May 06 '24 07:05 mui-bot

packages/material-ui/material-ui.production.min.js: parsed: +18.78% , gzip: +5.84%

Any idea where this 18% increase comes from?

Janpot avatar May 06 '24 10:05 Janpot

packages/material-ui/material-ui.production.min.js: parsed: +18.78% , gzip: +5.84%

Any idea where this 18% increase comes from?

Not sure. It seems to be related to the umd build. Do you have any ideas on how we can compare the actual build size of master versus the build on this branch for umd? It's possible that our code in dangerFile.ts or contributor-dashboard-legacy code in mui-public is miscalculating it.

ZeeshanTamboli avatar May 06 '24 15:05 ZeeshanTamboli

You could try running

 pnpm --filter @mui/material build:umd

on master and copy the packages/mui-material/build/umd/material-ui.production.min.js somewhere else, then run the same command on this branch and then git diff both files . Maybe that will give a clue on what was added extra by this branch?

Janpot avatar May 07 '24 07:05 Janpot

@Janpot Yesterday, I spent quite some time checking and debugging.

I compared the development files instead of the production ones because the production file is unreadable, likely due to terser. Here's what I did:

  • Checked out the next branch.
  • Ran pnpm install to get the Babel plugins.
  • Built the UMD bundle using pnpm --filter @mui/material build:umd.
  • Copied the material-ui.development.js file and renamed it to next-material-ui.development.js.
  • Checked out this branch.
  • Ran pnpm install and built the UMD bundle again.
  • Copied the material-ui.development.js file and renamed it to babel-branch-material-ui.development.js.
  • Compared both files using git diff --no-index --word-diff next-material-ui.development.js babel-branch-material-ui.development.js.

Here's a sample diff which is the same till the end:

https://github.com/mui/material-ui/assets/20900032/01789cb9-cf46-4df3-82af-5f36e191f0bd

What I noticed is that it's using objectSpread (which is defined multiple times) instead of _extends, resulting in a larger bundle size. This change is due to the @babel/plugin-transform-object-rest-spread plugin removal. When loose: true, it uses the _extends helper (https://babeljs.io/docs/babel-plugin-transform-object-rest-spread#loose), which was the case in the next branch.

What I don't understand is why rollup's babel is using the object-spread plugin in the first place? Is it that it is not considering the .browserslistrc file and checking the browser versions?

ZeeshanTamboli avatar May 08 '24 06:05 ZeeshanTamboli

@ZeeshanTamboli I plan to work on the UMD removal hopefully this week or next, but if you want to unblock this PR by removing it yourself, feel free to go ahead.

DiegoAndai avatar May 08 '24 19:05 DiegoAndai

How about we open a new issue for this? https://pagespeed.web.dev/analysis?url=https%3A%2F%2Fdeploy-preview-42140--material-ui.netlify.app%2F

SCR-20240508-qkhd

This can be reproduced too in HEAD. I imagine that Lighthouse uses Cannot call a class as a function to detect https://babeljs.io/docs/babel-plugin-transform-classes. Yes, bingo: https://github.com/GoogleChrome/lighthouse/blob/369979f498bd6560127e10476edffb264d4fa3b9/core/audits/byte-efficiency/legacy-javascript.js#L301C22-L301C55.

Seeing this on https://deploy-preview-42140--material-ui.netlify.app/ is wrong:

SCR-20240508-tytg

We should at minimum go into loose mode: https://github.com/vercel/next.js/blob/5ff2731c589692ed86379f876a38e1ca46f5761e/packages/next/src/build/babel/preset.ts#L161.

I also left the feedback in https://github.com/vercel/next.js/issues/65540.

oliviertassinari avatar May 08 '24 21:05 oliviertassinari

@ZeeshanTamboli I plan to work on the UMD removal hopefully this week or next, but if you want to unblock this PR by removing it yourself, feel free to go ahead.

@DiegoAndai Thanks. I've already begun working on this yesterday. Here's the PR: https://github.com/mui/material-ui/pull/42172. Feel free to push any changes if needed.


How about we open a new issue for this?

@oliviertassinari Do we need to make any changes outside of this PR? I suppose it's all related to Next.js with the new issue you created there.

ZeeshanTamboli avatar May 09 '24 06:05 ZeeshanTamboli

Given the Next.js team's track record with the maintenance of the babel setup in Next.js I would assume we'll be using their SWC setup before this gets fixed 😄.

Btw, yesterday, on top of this PR on the X repo I did a few quick tests and I could just remove the babel config without breaking their build. Page compilation in dev mode went from ~8s to ~4.5s on my machine. I couldn't enable turbopack due to our usage of esmExternals.

Janpot avatar May 09 '24 07:05 Janpot

@ZeeshanTamboli would it be possible to come up with a detailed list of Safari 12 features that will stop working on v6? This is so users can adjust accordingly.

DiegoAndai avatar May 09 '24 20:05 DiegoAndai

would it be possible to come up with a detailed list of Safari 12 features that will stop working on v6? This is so users can adjust accordingly.

@DiegoAndai I'm not familiar with the specific features of Safari 12, but whatever was available in Safari 12 will be carried forward to the newly supported Safari 15.6 version. We can document the list of supported browser versions, as done in the Migrating to v5 docs, but that should be done in a separate PR.

ZeeshanTamboli avatar May 10 '24 13:05 ZeeshanTamboli

@Janpot @DiegoAndai Now that #42172 is merged, this PR is ready for review.

ZeeshanTamboli avatar May 17 '24 10:05 ZeeshanTamboli

Do we need to make any changes outside of this PR? I suppose it's all related to Next.js with the new issue you created there.

@ZeeshanTamboli Not for this PR, but there is something for docs-infra here, issue created #42385.

oliviertassinari avatar May 24 '24 23:05 oliviertassinari