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

[core] Remove outdated Babel plugins #42140

Merged
merged 7 commits into from May 17, 2024

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented May 6, 2024

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 ZeeshanTamboli added core Infrastructure work going on behind the scenes v6.x labels May 6, 2024
@mui-bot
Copy link

mui-bot commented May 6, 2024

Netlify deploy preview

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

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against f84b8c0

@ZeeshanTamboli ZeeshanTamboli marked this pull request as ready for review May 6, 2024 10:36
@ZeeshanTamboli ZeeshanTamboli requested review from DiegoAndai and a team May 6, 2024 10:39
@Janpot
Copy link
Member

Janpot commented May 6, 2024

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

Any idea where this 18% increase comes from?

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented May 6, 2024

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.

@Janpot
Copy link
Member

Janpot commented May 7, 2024

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?

@ZeeshanTamboli
Copy link
Member Author

ZeeshanTamboli commented May 8, 2024

@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:

umd.builds.diff.mp4

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?

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yesterday, I spent quite some time checking and debugging.

Uff, I'm sorry, that was not my intention. I believe the UMD bundle is up for removal in v6. We have visualized now the added code in the bundle, at least we now know it's in the bundling and not in the size calculation. Let's not waste more time on debugging this. For me this PR is good to merge after UMD is removed.

@DiegoAndai
Copy link
Member

@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.

@oliviertassinari oliviertassinari changed the title [core] Remove outdated babel plugins [core] Remove outdated Babel plugins May 8, 2024
@ZeeshanTamboli
Copy link
Member Author

@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: #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.

@Janpot
Copy link
Member

Janpot commented May 9, 2024

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.

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged labels May 9, 2024
@DiegoAndai
Copy link
Member

@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.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 10, 2024
@ZeeshanTamboli
Copy link
Member Author

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
Copy link
Member Author

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

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go 👍

@ZeeshanTamboli ZeeshanTamboli merged commit 1b88862 into mui:next May 17, 2024
19 checks passed
@ZeeshanTamboli ZeeshanTamboli deleted the remove-outdated-babel-plugins branch May 17, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes performance v6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants