-
Notifications
You must be signed in to change notification settings - Fork 216
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
Add tool to compare bundle sizes #328
Conversation
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.
Few comments otherwise great job ;)
@@ -25,7 +25,9 @@ | |||
"main": "dist/vast-client-node.min.js", | |||
"browser": "dist/vast-client.min.js", | |||
"scripts": { | |||
"prebuild": "rm -rf dist_old && mkdir dist_old && cp -a dist/. dist_old/", |
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.
Not windows (cmd) proof I think but I'm not sure if it's actually a problem
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'm not sure how I would make it work on both windows and mac 🤔 is it ok if I leave it as is?
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 guess it's ok like that, there is still ways for windows users to use it, so install the bash from "windows features" (from what I quickly checked on the internet) or use Git bash (what I do)
bundleSize.js
Outdated
const newSize = newValues[name]; | ||
const delta = newSize - size; | ||
const sizeColorFg = delta <= 0 ? '\x1b[32m' : '\x1b[31m'; | ||
const reset = '\x1b[0m'; |
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 spent some minutes try to figure out what this reset variable stood for, It's maybe only because I'm not used to manipulate colors in console outputs but maybe a naming like "resetColorFg" or something like this would helped me a bit.
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.
sure I can rename 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.
LGTM !
Description
It is possible now to compare local bundle size vs last build.
Type