Skip to content

Conversation

oliviertassinari
Copy link
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 Changes related to the core-infra product. 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

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

@@ -628,58 +628,59 @@ export default async function generateComponentApi(
const filename = componentInfo.filename;
let reactApi: ComponentReactApi;

if (componentInfo.isSystemComponent || componentInfo.name === 'Grid2') {
Copy link
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
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
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
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
22 checks passed
@oliviertassinari oliviertassinari deleted the remove-noise-fragment branch November 28, 2024 20:00
@oliviertassinari
Copy link
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/[email protected]/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 Changes related to the core-infra product.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants