-
-
Notifications
You must be signed in to change notification settings - Fork 2.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
Log bundle metrics #733
Log bundle metrics #733
Conversation
Codecov Report
@@ Coverage Diff @@
## master #733 +/- ##
==========================================
+ Coverage 94.1% 94.43% +0.33%
==========================================
Files 64 66 +2
Lines 2036 2103 +67
==========================================
+ Hits 1916 1986 +70
+ Misses 120 117 -3
Continue to review full report at Codecov.
|
src/Bundle.js
Outdated
@@ -18,6 +18,14 @@ class Bundle { | |||
this.siblingBundles = new Set(); | |||
this.siblingBundlesMap = new Map(); | |||
this.offsets = new Map(); | |||
this.startTime = new Date().getTime(); |
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.
Possibly a micro-optimization, but Date.now()
is historically faster.
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 will change it, performance boosts are always a good thing
src/packagers/CSSPackager.js
Outdated
@@ -23,6 +23,7 @@ class CSSPackager extends Packager { | |||
} | |||
|
|||
await this.dest.write(css); | |||
this.bundle.addAssetSize(asset, this.dest); |
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.
Any way for this to be in the base Packager
without reading the file every time? Or maybe stat
is efficient enough? Would be nice to not have to re-implement this for every Packager (and remember to do so in the future).
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 can possibly move it to asset, but stat
is an extra operation and it wouldn't keep in mind things like treeshaking and uglifying. So i guess this is the correct implementation, keeping in mind that some Packagers have a different way of writing files, for example sourcemap writes everything at the end and rawPackager copies the file (without a writestream)
I did my best to abstract it using bundle.addAssetSize
src/utils/bundleReport.js
Outdated
return `${size.toFixed(2)} ${sizeTypes[type]}`; | ||
} | ||
|
||
function prettifyTime(time) { |
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.
Similar logic exists in Bundler.js
and could be consolidated into a util:
Lines 196 to 201 in 391e17f
let buildTime = Date.now() - startTime; | |
let time = | |
buildTime < 1000 | |
? `${buildTime}ms` | |
: `${(buildTime / 1000).toFixed(2)}s`; | |
logger.status(emoji.success, `Built in ${time}.`, 'green'); |
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.
Will change
src/utils/bundleReport.js
Outdated
size = size / 1024; | ||
type++; | ||
} | ||
return `${size.toFixed(2)} ${sizeTypes[type]}`; |
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.
While unlikely, you could overflow your array in which case you'd end up with 1.54 undefined
. Should at least safe-guard against it and stop dividing by 1024.
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.
Ow yeah, thought it wouldn't be an issue as a bundle larger than 1024 GB would be insane for a web application (or any other application)
But i've fixed it anyway
src/utils/bundleReport.js
Outdated
@@ -0,0 +1,106 @@ | |||
const path = require('path'); | |||
const sizeTypes = ['B', 'KB', 'MB', 'GB']; |
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.
Should KB
be kB
?
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.
It should be KB as it's 1024 not 1000
"1 KB" means 1024 bytes (as Windows would report it, traditional usage)
"1 kB" means 1000 bytes (as Mac OS would report it, IEC usage)
"1 KiB" means 1024 bytes (unambiguous, but perhaps unfamiliar terminology)
src/utils/bundleReport.js
Outdated
const path = require('path'); | ||
const sizeTypes = ['B', 'KB', 'MB', 'GB']; | ||
|
||
function spaces(amount) { |
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.
Can use String.prototype.repeat()
here:
function spaces(count) {
return ' '.repeat(count);
}
It also correctly returns an empty string for 0
.
There is also String.prototype.padEnd()
in Node 8+ which may simplify some code.
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'll use the repeat as babel didn't catch padEnd when i tried to build
Curious why the total time is so much greater (as a %) than the bundle time in both of the screenshots. |
@brandon93s the bigger time is because the total time in the metrics is the total processing time, basically the time it would've taken if ran on a single thread/worker. |
src/utils/bundleReport.js
Outdated
@@ -0,0 +1,95 @@ | |||
const path = require('path'); | |||
const prettifyTime = require('./prettifyTime'); | |||
const sizeTypes = ['B', 'KB', 'MB', 'GB']; |
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.
What about using KiB
, MiB
, GiB
or changing the factor to 1000 instead of 1024, for correctness?
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, the current implementation seems like the accurate notation, as it's being used by a lot of OS's (I've changed it anyway so that it is 100% correct)
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 know that Windows uses the wrong KB notation, and that most Linux distros use KiB notation. Don't know about Mac, Android, iOS and others.
(nice!)
Question: should we only do this on |
Ok I ended up reworking this a bit to make it a bit prettier. Here's what it looks like: When you want a detailed report it looks like this: As you can see, it's clearer which items are bundles, and which items are assets within bundles. Bundles are bold, and assets are dimmed and indented. Plus it's colorful! Directory names are dimmed and filenames highlighted. 🎨 It will also warn when you have a large bundle size with a warning triangle and yellow text: I moved the table rendering stuff to the logger class, and used the widely used Let me know what you think! |
Feature requested in closes #415
Minimal report (default behaviour)
Detailed report (cli flag
--detailed-report
)