-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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] Run @mui/icon-material src:icons #44097
[core] Run @mui/icon-material src:icons #44097
Conversation
Netlify deploy previewhttps://deploy-preview-44097--material-ui.netlify.app/ @material-ui/core: parsed: +Infinity% , gzip: +Infinity% Bundle size reportDetails of bundle changes (Toolpad) |
@zannager How did you come up with this assignment? It doesn't match with https://www.notion.so/mui-org/GitHub-community-issues-PRs-Tier-1-12a84fdf50e44595afc55343dac00fca?pvs=4#9734f2f5d754435c8838e74523851d54 or https://www.notion.so/mui-org/Icons-7ff27203df124147850ec3e26d04d517 so I'm curious. I wouldn't want to overwhelm Marija, feels like a bit of a distraction (same for me actually to open this PR but 🤷♂️ it was quick 😄). @mnajdova One of the possible playbook:
This would mean that some scopes will get neglected for a while, as we learn. |
I just can't recall if she showed up under suggestions or I leaned towards https://www.notion.so/mui-org/MUI-Core-92686ffc7cc8437280c13e4a3a0cbc56. I refernce to https://www.notion.so/mui-org/MUI-Core-92686ffc7cc8437280c13e4a3a0cbc56 when I see it labeled as "core" but with "Icons" or with child scopes, I usually assigned someone else.
I agree, which is why I only assign or request reviews from Marija when she is the sole maintainer. Please let me know if you’d prefer I skip assigning her, even for the products/components she solely owns. |
Thanks for the explanation for each change Olivier, it helps with the review. I agree with the proposed changes in #44097 (comment), I will try to clean up the owners and bring this up with the core team. I will ask for a second review from @siriwatknp / @michaldudak as they are the co-owner of the icons package from what I remember. Considering Michal is involved with Base UI now, Jun looks like the right person who we would by default ask a review from. |
@zannager Ok thanks, from there, I think:
I don't think that we should have exceptions, it adds complexity to the process, let's keep it as simple as we can get away with. With the changes ⬆️, it might be enough. |
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.
It looks like it works correctly. I couldn't spot any errors
I noticed this quickly while working on #33746 (comment). This should help tackle: #32846 (we would likely need to do this change anyway).
I have rerun
pnpm src:icons
to keep it in sync with the codebase. This command used to be run during the icon publish step, until Sebastian moved it to be stored in git to make it easier to spot regressions.There is so many changes that it's hard to follow, but it's typically:
packages/mui-icons-material/lib/Abc.js
and
packages/mui-icons-material/lib/esm/ScreenRotationRounded.js
Now, I usually never trust something that I don't understand, so I dove a bit to understand what's going on:
(
and comments babel/babel#16780..default
seems to be about https://github.com/babel/babel/pull/14120/files#diff-6b555d8887bc858212b04534b4146442f8880fcf26df080baab49b90b1c10fb4R89. Ok, makes sense: https://unpkg.com/@babel/[email protected]/helpers/interopRequireDefault.js..js
extension. I can't find where the behavior changed, but Add explicit.ts
/.js
extension to all imports insrc
babel/babel#15892 explains why it makes sense.