Skip to content

[core-infra] Remove useless fragments#44516

Merged
oliviertassinari merged 2 commits intomui:masterfrom
oliviertassinari:remove-noise-fragment
Nov 28, 2024
Merged

[core-infra] Remove useless fragments#44516
oliviertassinari merged 2 commits intomui:masterfrom
oliviertassinari:remove-noise-fragment

Conversation

@oliviertassinari
Copy link
Copy Markdown
Member

@oliviertassinari oliviertassinari commented Nov 24, 2024

I'm having some fun with the AST, allowing https://github.com/reactjs/react-docgen to extract props descriptions even when the component doesn't return a React element. We shouldn't be shipping dead code to developers, this PR avoids this. This should both improve bundle size and runtime.

DefinitelyTyped/DefinitelyTyped#65135 should help in the future.

@oliviertassinari oliviertassinari added the scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd). label Nov 24, 2024
@oliviertassinari oliviertassinari changed the title [core] Remove useless fragments [core-infra] Remove useless fragments Nov 24, 2024
@mui-bot
Copy link
Copy Markdown

mui-bot commented Nov 24, 2024

Netlify deploy preview

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

PigmentHidden: parsed: +2.29% , gzip: +2.84%
Hidden: parsed: +1.85% , gzip: +2.13%
@material-ui/core: parsed: +0.20% , gzip: +0.37%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 16b8868

const filename = componentInfo.filename;
let reactApi: ComponentReactApi;

if (componentInfo.isSystemComponent || componentInfo.name === 'Grid2') {
Copy link
Copy Markdown
Member Author

@oliviertassinari oliviertassinari Nov 24, 2024

Choose a reason for hiding this comment

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

That hard-coded logic felt wrong, we further added stuff around it in 08dce48#diff-eef575d465fe9894710e7eb399c76ad3e3900a8cbb6d374be7f303c4b6c6ef51R627

Copy link
Copy Markdown
Contributor

@romgrk romgrk left a comment

Choose a reason for hiding this comment

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

LGTM overall but I don't think I'm the right person to review this, I have barely touched the docs & mui-material (unlike mui-system).

@oliviertassinari
Copy link
Copy Markdown
Member Author

oliviertassinari commented Nov 25, 2024

It's related to code-infra, right, I wasn't sure who to ask. This will impact Base UI too, e.g. we should be able to simplify NoSsr implementation. Let's ask Michal for a review, I guess it's the closest.

Copy link
Copy Markdown
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

Nice, this looks good. I ran docs:api with this code in the Base UI repo and didn't get any changes in generated files, so I assume it works as correctly as before.

@oliviertassinari oliviertassinari merged commit 28ac35c into mui:master Nov 28, 2024
@oliviertassinari oliviertassinari deleted the remove-noise-fragment branch November 28, 2024 20:00
@oliviertassinari
Copy link
Copy Markdown
Member Author

oliviertassinari commented Nov 28, 2024

Great.

Next, it's about:

  • releasing this, wait to see that we don't break anything
  • updating the Base UI and MUI X dependency on the mono repository
  • removing those dead Fragment from the codebase.

Right now: https://unpkg.com/browse/@mui/material@6.1.9/NoSsr/NoSsr.js It should be a small win with bundle size:

-  // We need the Fragment here to force react-docgen to recognise NoSsr as a component.
-  return /*#__PURE__*/_jsx(React.Fragment, {
-    children: mountedState ? children : fallback
-  });
+  return mountedState ? children : fallback;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants