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

[test] Fix size snapshot including peer dependencies #14636

Merged
merged 3 commits into from Feb 23, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Feb 23, 2019

The programmatic API from size-limit does not use the same webpack config as the cli. Therefore peer dependencies were showing up in the snapshot.

By using a custom webpack config we can exclude those and also properly set up aliases instead of using yarn link. This will also save the stats from webpack in a build artifact that can be inspected with e.g. https://chrisbateman.github.io/webpack-visualizer/ or http://webpack.github.io/analyse/

@eps1lon eps1lon added the test label Feb 23, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Feb 23, 2019

@material-ui/core: parsed: -23.82% 😍, gzip: -29.70% 😍
@material-ui/lab: parsed: -38.14% 😍, gzip: -41.73% 😍
@material-ui/styles: parsed: -9.48% 😍, gzip: -10.39% 😍
@material-ui/system: parsed: +7.21% , gzip: +13.92%

Details of bundle changes.

Comparing: ac57a7c...815ab0c

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -23.82% -29.70% 484,877 369,384 130,093 91,457
@material-ui/core/Paper -7.70% -9.87% 83,040 76,647 21,410 19,297
@material-ui/core/Paper.esm -6.69% -8.26% 76,728 71,595 20,462 18,771
@material-ui/core/Popper -79.25% -77.40% 146,815 30,462 46,826 10,584
@material-ui/core/styles/createMuiTheme +6.47% 🔺 +10.87% 🔺 16,235 17,286 5,159 5,720
@material-ui/core/useMediaQuery -73.03% -69.91% 9,217 2,486 3,503 1,054
@material-ui/lab -38.14% -41.73% 295,473 182,793 86,174 50,217
@material-ui/styles -9.48% -10.39% 63,755 57,708 18,119 16,237
@material-ui/system +7.21% 🔺 +13.92% 🔺 15,915 17,062 3,938 4,486
Button -53.46% -57.26% 212,888 99,068 61,970 26,484
Modal -53.27% -57.61% 211,094 98,649 61,687 26,152
colorManipulator +41.14% 🔺 +54.77% 🔺 2,290 3,232 838 1,297
docs.landing +1.91% 🔺 +4.41% 🔺 48,963 49,899 10,275 10,728
docs.main +0.14% 🔺 +0.22% 🔺 676,139 677,075 205,127 205,583
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 320,511 320,511 84,731 84,731

Generated by 🚫 dangerJS against 815ab0c

@eps1lon eps1lon added on hold There is a blocker, we need to wait and removed on hold There is a blocker, we need to wait labels Feb 23, 2019
@eps1lon
Copy link
Member Author

eps1lon commented Feb 23, 2019

@oliviertassinari This looks about right.

There are some minor increases in e.g. colorManipulator compared to the previous size-limit because size-limit did not include the size of an empty webpack project. However this number was always hard coded and might've been wrong. I think it's fine including the size of an empty project since very small and constant.

/* eslint-disable no-console */
const childProcess = require('child_process');
const fse = require('fs-extra');
const { flatten, fromPairs } = require('lodash');
Copy link
Member

Choose a reason for hiding this comment

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

The downside of using named import is the friction it's creating when editing the code. We have to go to the top of the file to add the missing dependencies. What do you think of using a different tradeoff for the server side scripts, where we don't care about the bundle size?

Suggested change
const { flatten, fromPairs } = require('lodash');
const lodash = require('lodash');

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense to me. Will change

@oliviertassinari oliviertassinari changed the title [ci] Fix size snapshot including peer dependencies [test] Fix size snapshot including peer dependencies Feb 23, 2019
Just creates diff noise if additional imports are used. There
are little to no benefits for named imports in node
@eps1lon eps1lon merged commit bfa9167 into mui:next Feb 23, 2019
@eps1lon eps1lon deleted the ci/better-size-calc branch February 23, 2019 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants