-
Notifications
You must be signed in to change notification settings - Fork 16
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
Remove UMD list from bundle size comparison code #162
base: master
Are you sure you want to change the base?
Remove UMD list from bundle size comparison code #162
Conversation
@Janpot or @oliviertassinari Can you take a look at this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it looks good, how the deployment is done?
It's hosted on netlify and should just deploy once we merge. I'm no quite sure why the deploy preview isn't working.
Will it impact PRs cherry-picked for v5?
Yes, I guess we could postpone merging this until v5 is fully unsupported?
I have updated CircleCI and Netlify config to be friendly with forks. I think we didn't consider this use case enough. Let's see if it's enough. https://app.circleci.com/settings/project/github/mui/mui-public/advanced |
@@ -23,11 +20,6 @@ function getMainBundleLabel(bundleId: string): string { | |||
.replace(/^@material-ui\/unstyled$/, '@mui/core') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.replace(/^@material-ui\/unstyled$/, '@mui/core') | |
.replace(/^@material-ui\/unstyled$/, '@mui/base') |
Actually, it would make more sense to me to host the label logic in https://s3.eu-central-1.amazonaws.com/mui-org-ci/artifacts/master/latest/size-snapshot.json:
.replace(/^@material-ui\/unstyled$/, '@mui/core') |
@@ -262,11 +262,6 @@ const CompareTable = memo(function CompareTable({ | |||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to keep the contributor-dashboard-legacy tool?
}); |
@@ -2,9 +2,6 @@ | |||
import axios from 'axios'; | |||
|
|||
function getMainBundleLabel(bundleId: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like by default we should use the bundleId
. @material-ui/core/Textarea
looks clearer than TextareaAutosize
to me. I recall Sebastian wanted the bundleId
to never change so we could always compare sizes with any previous build. While I admire Sebastian for the depth at which he's considering problems, I never felt that this was matching the use case for the tool, removing this constraint feels more pragmatic.
In any case, if we want to keep bundleId
immutable, it still feels like we would do this: https://github.com/mui/mui-public/pull/162/files#r1597223021, moving the labels logic to each repository, not have it in https://github.com/mui/mui-public.
UMD bundle is being removed in mui/material-ui#42172.
I don't have access to add labels and ask for reviewers.
If it looks good, how the deployment is done? What's the process? Will it impact PRs cherry-picked for v5?