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

[zero] Move extendTheme to its own subpath #41204

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

brijeshb42
Copy link
Contributor

@brijeshb42 brijeshb42 commented Feb 20, 2024

fixing the size of runtime bundle

Context -

zero-runtime package has 2 different types of exports -

  1. Package level, ie, import {styled} from '@mui/zero-runtime';
  2. Subpaths, ie, import {generateCss} from '@mui/zero-runtime/utils'

package level exports should be what we want to end-up in the final bundle and subpaths is for sharing common logic across different bundle plugin implementation (ie, implement the common functionality in one place and import and use it it next/vite plugin).

When @siriwatknp implemented the extendTheme functionality, he added it directly to the package level export, ie, import {extendTheme} from '@mui/zero-runtime';. I think this was an oversight as without proper tree-shaking, extendTheme will also end-up being part of the bundle (which also includes @mui/system).
So I was mainly fixing that. So moved it to its own subpath (@mui/zero-runtime/extendTheme). While doing this, I also made the change to re-export these items from the bundle specific packages as well.
My thinking is that if users are configuring their bundler, they should only import stuff from the bundler package and think of @mui/zero-runtime as the runtime package. Rest of the context is here.

fixing the size of runtime bundle
@brijeshb42 brijeshb42 added the package: pigment-css Specific to @pigment-css/* label Feb 20, 2024
@mui-bot
Copy link

mui-bot commented Feb 20, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against d9b8315

@@ -81,3 +82,5 @@ export function withZeroPlugin(nextConfig: NextConfig, zeroConfig: ZeroPluginCon
webpack,
};
}

export { extendTheme };
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to reexport it from the plugin? I am afraid that it might be confusing for users especially with auto import/autocomplete in IDE.

Copy link
Contributor Author

@brijeshb42 brijeshb42 Feb 21, 2024

Choose a reason for hiding this comment

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

It'll reduce the mental overhead for users trying to import this.
Ideally, they should only think of @mui/zero-runtime imports to be imported only in their source code, not in the config files outside the source.
They should only use bundler specific package in their config and zero runtime in source code.
That's why in the main export of zero runtime, I've only added what you could similarly import from emotion.
Other exports are in subpaths mainly to share same logic across different bundler plugins that we'll support.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. What's your thought @mnajdova ?

Copy link
Member

Choose a reason for hiding this comment

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

They should only use bundler specific package in their config and zero runtime in source code.

In general this makes sense to me 👍

@brijeshb42 brijeshb42 merged commit b98c0b5 into mui:master Feb 21, 2024
19 of 20 checks passed
@brijeshb42 brijeshb42 deleted the zero-extendtheme branch February 21, 2024 16:58
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: pigment-css Specific to @pigment-css/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants