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

[material-ui] Remove UMD bundle #42172

Merged
merged 24 commits into from
May 17, 2024
Merged

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented May 9, 2024

Closes #40960

PR to remove listing of UMD bundle size from contributor dashboard and toolpad dashboard - mui/mui-public#162.

@ZeeshanTamboli ZeeshanTamboli added package: material-ui Specific to @mui/material core Infrastructure work going on behind the scenes v6.x labels May 9, 2024
@mui-bot
Copy link

mui-bot commented May 9, 2024

Netlify deploy preview

packages/material-ui/material-ui.production.min.js: parsed: -100.00% 😍, gzip: -100.00% 😍

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against a477294

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 9, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 9, 2024
@ZeeshanTamboli ZeeshanTamboli marked this pull request as ready for review May 9, 2024 07:45
@@ -159,20 +159,9 @@ For instance, via Google Web Fonts:
## CDN

You can start using Material UI right away with minimal front-end infrastructure by installing it via CDN, which is a great option for rapid prototyping.
Follow [this CDN example](https://github.com/mui/material-ui/tree/master/examples/material-ui-via-cdn) to get started.
Follow [this CDN example](https://github.com/mui/material-ui/tree/next/examples/material-ui-via-cdn) to get started.
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a marker so we don't forget to switch back when master becomes default branch again

Suggested change
Follow [this CDN example](https://github.com/mui/material-ui/tree/next/examples/material-ui-via-cdn) to get started.
<!-- #default-branch-switch -->
Follow [this CDN example](https://github.com/mui/material-ui/tree/next/examples/material-ui-via-cdn) to get started.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, and I would add this in

link: 'https://github.com/mui/material-ui/tree/next/examples/material-ui-nextjs',
too.

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.

A few tweaks to docs, but otherwise looks good to me

packages/material-ui/material-ui.production.min.js: parsed: -100.00% 😍, gzip: -100.00% 😍

Seems to check out 😄

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

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @ZeeshanTamboli!

examples/material-ui-via-cdn/README.md Show resolved Hide resolved
docs/data/material/migration/migration-v5/migration-v5.md Outdated Show resolved Hide resolved
examples/material-ui-via-cdn/README.md Outdated Show resolved Hide resolved
@DiegoAndai DiegoAndai requested a review from samuelsycamore May 9, 2024 16:01
@DiegoAndai

This comment was marked as outdated.

@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 ZeeshanTamboli requested a review from DiegoAndai May 10, 2024 06:58
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 12, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 14, 2024
@DiegoAndai DiegoAndai removed the request for review from samuelsycamore May 16, 2024 17:15
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

I would like to unblock this so we can start closing out work for v6. We can review the migration guide copy before release. @ZeeshanTamboli if this is ready, please merge 🙌🏼

@ZeeshanTamboli
Copy link
Member Author

@Janpot @DiegoAndai I've pushed two additional commits to remove some remaining elements. Could you review them and confirm if it's correct to remove them?

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.

Great job 👍

@ZeeshanTamboli ZeeshanTamboli merged commit 393f744 into mui:next May 17, 2024
22 checks passed
@ZeeshanTamboli ZeeshanTamboli deleted the remove-umd-bundle branch May 17, 2024 10:31
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

A few more comments for a follow up 😁


<!-- #default-branch-switch -->

The UMD bundle is no longer provided. This was replaced in favor of [ESM CDNs](https://esm.sh/). Please refer to the [CDN docs](https://next.mui.com/material-ui/getting-started/installation/#cdn) for alternatives.
Copy link
Member

Choose a reason for hiding this comment

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

This should be a relative URL not an absolute one.

Suggested change
The UMD bundle is no longer provided. This was replaced in favor of [ESM CDNs](https://esm.sh/). Please refer to the [CDN docs](https://next.mui.com/material-ui/getting-started/installation/#cdn) for alternatives.
The UMD bundle is no longer provided. This was replaced in favor of [ESM CDNs](https://esm.sh/).
Please refer to the [CDN docs](/material-ui/getting-started/installation/#cdn) for alternatives.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I think we should mention that we are doing this to have parity with React 19 https://react.dev/blog/2024/04/25/react-19-upgrade-guide#umd-builds-removed. Also, helps transfer the blame upstream.

@@ -159,20 +159,9 @@ For instance, via Google Web Fonts:
## CDN

You can start using Material UI right away with minimal front-end infrastructure by installing it via CDN, which is a great option for rapid prototyping.
Follow [this CDN example](https://github.com/mui/material-ui/tree/master/examples/material-ui-via-cdn) to get started.
Follow [this CDN example](https://github.com/mui/material-ui/tree/next/examples/material-ui-via-cdn) to get started.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, and I would add this in

link: 'https://github.com/mui/material-ui/tree/next/examples/material-ui-nextjs',
too.

{
"imports": {
"react": "https://esm.sh/[email protected]",
"react-dom": "https://esm.sh/[email protected]",
Copy link
Member

@oliviertassinari oliviertassinari May 17, 2024

Choose a reason for hiding this comment

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

It feels like it would be best to use latest here, so we know a lot faster when it breaks with a latest version. Especially since we don't have a test for this anymore.

Suggested change
"react-dom": "https://esm.sh/react-dom@18.3.0",
"react-dom": "https://esm.sh/react-dom@latest",

Copy link
Member

Choose a reason for hiding this comment

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

Another approach could be to version all dependencies with just their major version. To ensure compatibility across all dependencies while still enjoying the latest version.

"react-dom": "https://esm.sh/react-dom@18",
"@mui/material": "https://esm.sh/@mui/material@5?external=react"

Copy link
Member

@oliviertassinari oliviertassinari May 21, 2024

Choose a reason for hiding this comment

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

@Janpot Yeah, we could maybe do this. However, I think this should be for all the examples, not just this one, so let's go with the latest to scope down the change. First, let's get consistency. Second, we will see if we should change the tradeoff for all examples.

Setting the major version delays the report of bugs with the latest version and it's extra work. However, it also means that our examples are more likely to work at all times. I think that until we have more bandwidth than at the time of Me + Sebastian + Matt + Marija we shouldn't change the tradeoff. From what I have observed these last few years, we have been clearly slower, so I would delay this change. If you also consider that React, between two majors, almost never introduces breaking changes, they do deprecations first and that we are trending to do the same thing between MUI projects, then it might be fine. After a major, examples would continue to work, people would get new warnings, and we will fix them. A breakage would signal something isn't right.

A quick benchmark, Next.js is cherry-picking its examples version: https://github.com/vercel/next.js/blob/canary/examples/with-redis/package.json.

'./umd/material-ui.development.js',
'./umd/material-ui.production.min.js',
].map(async (file) => {
['./index.js', './modern/index.js', './node/index.js'].map(async (file) => {
Copy link
Member

@oliviertassinari oliviertassinari May 17, 2024

Choose a reason for hiding this comment

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

This is a win for https://packagephobia.com. Maybe enough to mention in the blog post.

root.render(
<ThemeProvider theme={theme}>
{/* CssBaseline kickstart an elegant, consistent, and simple baseline to build upon. */}
<CssBaseline />
Copy link
Member

Choose a reason for hiding this comment

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

We need CssBaseline for all examples, it's part of the recommanded approach to get started that we have in the docs. So I think it's a must add back.

Same for the rest, I think that we should match the other examples.

"react": "https://esm.sh/[email protected]",
"react-dom": "https://esm.sh/[email protected]",
"react/jsx-runtime": "https://esm.sh/[email protected]/jsx-runtime",
"@mui/material": "https://esm.sh/@mui/material?external=react"
Copy link
Member

Choose a reason for hiding this comment

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

Oh, interesting approach, makes sense to be able to use MUI X without duplication.

@ZeeshanTamboli
Copy link
Member Author

A few more comments for a follow up

@oliviertassinari PR - #42284

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 package: material-ui Specific to @mui/material v6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui] Remove UMD bundle
5 participants