-
Notifications
You must be signed in to change notification settings - Fork 6
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
π§ WIP: bundle analysis #61
base: main
Are you sure you want to change the base?
Conversation
lib/bundle/index.js
Outdated
format: "iife" | ||
}, | ||
reporting: { | ||
threshold: 100000 // ~100 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.
Not sure this is actually correct: 1000 vs. 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.
Speaking of which, I've used 1000 in rollup-plugin-analyzer
for formatting sizes. Do you think that might ought to be a configurable option there? Sorry to drop in here to ask, was just looking over this and noticed that might be something I should make configurable as well.
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.
We appreciate you dropping by.
I've just tried this:
let fs = require("fs");
let txt = fs.readFileSync("/tmp/lipsum.txt");
Buffer.byteLength(txt, "utf8") / 1000; // 282.292
Buffer.byteLength(txt, "utf8") / 1024; // 275.676
The file system (both on Debian and macOS) reports 276 kB, so I suppose 1024 is correct - which would also mean it doesn't need to be configurable?
|
||
module.exports = (config, assetManager, { watcher, browsers, compact } = {}) => { | ||
let bundles = config.map(bundleConfig => { | ||
// NB: bundle-specific configuration can override global options | ||
bundleConfig = Object.assign({ compact }, bundleConfig); | ||
return makeBundle(bundleConfig, assetManager.resolvePath, { browsers }); | ||
return makeBundle(bundleConfig, assetManager, { browsers }); |
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 happy with the increasing amount of context we have to pass through in situations like this - not sure that can be avoided though. (In this case, we need assertManager.referenceDir
so we can pass it to rollup-plugin-analyzer, eventually, via Bundle
β generateBundle
; it all seems a little too convoluted.)
report: ({ size, originalSize, reduction }) => { | ||
let b2kb = i => `${Math.round(i / 1024)} kB`; | ||
console.error(`${bundle}: ${b2kb(originalSize)} β ` + | ||
`${b2kb(size)} (Ξ ${Math.round(reduction)} %)`); |
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'd consider this a placeholder for now, until we've figured out detailed reporting (see above). It sort of duplicates AssetManager#writeFile
's reporting.
Interesting work, need to look at it in detail π A unified reporting concept for CSS, JS and images would be a great feature for faucet 1.1 |
caveats: * there might be performance implications: doesdev/rollup-plugin-analyzer#3 * no detailed report yet: doesdev/rollup-plugin-analyzer#2 * missing configuration options (cf. inline comment)
3e9a3cd
to
a4b1278
Compare
this supersedes #39
β omit plugin when invoking
rollup.rollup
limit
,filter
,showExports
,hideDeps
(not sure which abstractions make sense there)