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] Move MuiError babel macro to babel plugin #43904

Merged
merged 30 commits into from
Oct 7, 2024

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Sep 27, 2024

This plugin replaces code of the form

throw /* minify-error */ new Error(`MUI: invalid ${foo} on ${bar}`);

with

import _formatMuiErrorMessage from '@mui/utils/formatMuiErrorMessage'

// ...

throw new Error(process.env.NODE_ENV !== "production" ? `MUI: invalid ${foo} on ${bar}` : _formatMuiErrorMessage(1, foo, bar));

It replaces the former

import MuiError from '@mui/internal-babel-macros/MuiError.macro';

// ...

throw new MuiError(`MUI: invalid %s on %s`, foo, bar);

Same functionality in terms of error extraction as the MuiError babel macro, but the code is still runnable without the babel-plugin-macros plugin (just with unminified errors).

Also moved the check that guards against calling Error(...) without new to eslint
Also refactored the formatMuiErrorMessage as it looks like the optimizations don't make sense anymore, babel doesn't transform them anymore. Looks like it shaved off 16B 😄

Initially minified every error instance, which resulted in the following bundle sizes:

@material-ui/unstyled: parsed: -0.19% 😍, gzip: -0.22% 😍
@mui/joy: parsed: -0.06% 😍, gzip: -0.09% 😍
Transitions: parsed: +9.38% , gzip: +13.36%
Dropdown: parsed: +6.78% , gzip: +9.52%
@mui/joy/Dropdown: parsed: +6.79% , gzip: +9.52%
Tabs: parsed: +2.90% , gzip: +4.69%
MenuButton: parsed: +2.51% , gzip: +4.26%


@mui/code-infra What are your thoughts. I don't want to refactor the whole setup, the main goal is to remove babel-macros, but I'm on the fence on what to use as a marker for deciding which errors to minify. Alternatives to consider:

  • Tagged template literal exported from '@mui/internal-babel-plugin-minify-error'

    import minifyError from '@mui/internal-babel-plugin-minify-error';
    
    // ...
    
    throw new Error(minifyError`MUI: invalid ${foo} on ${bar}`)
    • pro:
      • Forces one way of doing things
      • agnostic of the Error constructor, can assign them to a variable and share with multiple Errors, e.g.
        const errorMessage = (componentName: string, className: string, slotName: string): string =>
        `${className} description from component ${componentName} should include ${slotName} since its definition includes "{{${slotName}}}"`;
    • con:
      • can't compose them easily like we can with regular string literals. e.g. to spread them across multiple lines.
  • Just minify every error of the pattern new Error(MUI: invalid ${foo} on ${bar})

    • Pro:
      • Forces usage, programmer doesn't need to be aware
    • cons:
      • Seems to affect bundle size a lot, but this may be a false positive, chances are quite high formatMuiErrorMessage will be included one way or the other. The individual bundle size may give a wring impression.

Main advantage of the /* minify-error */ is the fact that it doesn't require an import. The code is exactly the code that would execute when unminified. This makes the plugin a pure optimization.

@Janpot Janpot added the scope: code-infra Specific to the core-infra product label Sep 27, 2024
@mui-bot
Copy link

mui-bot commented Sep 27, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against f49eba0

@Janpot Janpot changed the title [code-infra] Move MuiError babel macros to babel plugin [code-infra] Move MuiError babel macro to babel plugin Sep 29, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 30, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 30, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 2, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 3, 2024
@Janpot Janpot marked this pull request as ready for review October 5, 2024 14:36
@Janpot
Copy link
Member Author

Janpot commented Oct 5, 2024

@michaldudak I renamed to just minify-error

@Janpot Janpot requested a review from a team October 5, 2024 14:41
docs/public/static/error-codes.json Outdated Show resolved Hide resolved
@Janpot Janpot merged commit 0210994 into mui:master Oct 7, 2024
19 checks passed
@Janpot Janpot deleted the replace-error-infra branch October 7, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants