-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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] Build size snapshots from installed packages #43452
Conversation
Netlify deploy previewhttps://deploy-preview-43452--material-ui.netlify.app/ @material-ui/core/styles/createTheme: parsed: -1.00% 😍, gzip: -0.83% 😍 Bundle size reportDetails of bundle changes (Toolpad) |
}, | ||
{ | ||
id: '@material-ui/core/styles/createTheme', | ||
path: 'packages/mui-material/build/styles/createTheme.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import will be disallowed in ESM so I'm adding the possibility to import a specific export createTheme
from @mui/material/styles
instead.
It resulted in a slight decrease, but I'd consider it to be more accurate
entry: { [entry.id]: path.join(workspaceRoot, entry.path) }, | ||
context: __dirname, | ||
entry: { | ||
[entry.id]: `./index.js!=!data:text/javascript,import ${importNames} from '${entry.import}';console.log(foo);`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the magic sauce:
- context to current dir, this is where modules are resolved form. Now that it's a workspace, it has a node_modules folder.
- data url, virtual file
./index.js
which just imports the measured module. and logs to circumvent tree shaking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this deserves a comment in code. I don't think this syntax is widely known (I just learned about this and it's pretty hard to search for it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I've added some links in a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one comment, but it does look good. The next step could be making it more generic and configurable and publishing to npm.
Mimic closer how the bundling process would happen at the end-user by creating a "virtual" entrypoint that imports from installed modules. This should result in a more predictable resolution behavior that's closer to what the end-user would experience.
The idea is to:
Will take care of #43441 in a separate effort, we would actually want to be able to see the diff for this PR.
This should help with the snapshot issues I'm seeing in #43264 which is the initial reason for this refactor.