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

[code-infra] Extract Babel macro from mui-utils #40262

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

michaldudak
Copy link
Member

I moved the MuiError Babel macro to its own package, so @mui/utils contain only run-time (not build-time logic).

This is necessary for the pnpm migration, as we would like to link build outputs between workspace projects (as opposed to sources, like it's done today). Bundlers and other tools will use Babel/Webpack/TS aliases to resolve the sources, but it was proven to be problematic in the case of the macro (somehow aliases did not apply there).

@michaldudak michaldudak added the scope: code-infra Specific to the core-infra product label Dec 20, 2023
@michaldudak michaldudak requested a review from a team December 20, 2023 21:55
@mui-bot
Copy link

mui-bot commented Dec 20, 2023

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 721d200

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.

This feels like an improvement as well.

I think there is a test somewhere that checks whether these macro imports appear in the build output, at which point it errors. Does that still work? (I may be hallucinating this)

@michaldudak
Copy link
Member Author

I think there is a test somewhere that checks whether these macro imports appear in the build output

I haven't found anything like this (I checked all occurrences of "macro" and "MuiError" in the codebase).

@michaldudak
Copy link
Member Author

@mui/x, I found one reference to the macro: https://github.dev/mui/mui-x/blob/d7149efe21b3b43f76fbf65aa35c499a7861d590/docs/babel.config.js#L41
I don't quite understand why it replaces the macro import with react, but I created a PR that updates the path: mui/mui-x#11479

@michaldudak michaldudak merged commit 08511a9 into mui:master Dec 21, 2023
20 checks passed
@michaldudak michaldudak deleted the new-babel-macros-package branch December 21, 2023 09:53
@flaviendelangle
Copy link
Member

😆 Wow this replacement is weird, no idea why it's here

@flaviendelangle
Copy link
Member

@michaldudak
Copy link
Member Author

Unfortunately, there's no word of explanation.

@flaviendelangle
Copy link
Member

flaviendelangle commented Dec 21, 2023

🤷 Yes, it's a mistery

@Janpot
Copy link
Member

Janpot commented Dec 21, 2023

Yep, we had this in Toolpad for a while after copying the setup from X. I remember asking @oliviertassinari about it once. I couldn't figure out what the sense in it was so I removed it on our side. I'd say remove it, and if that breaks something let's figure out a better solution.

@oliviertassinari
Copy link
Member

As I recall about this, the idea was to have an import that works, I used react as I could have used something random, I assumed errors will normally not throw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants