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

🚧 WIP: bundle analysis #61

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🚧 WIP: bundle analysis #61

wants to merge 1 commit into from

Conversation

FND
Copy link
Contributor

@FND FND commented Jul 7, 2018

this supersedes #39

test_browserslist
transpiling JavaScript for Chrome 63
transpiling JavaScript for IE 11
`dist/bundle.js`: 0 kB β†’ 0 kB (Ξ” 38 %)
`dist/bundle_alt.js`: 23 kB β†’ 0 kB (Ξ” 98 %)
`dist/bundle_legacy.js`: 23 kB β†’ 0 kB (Ξ” 98 %)
⚠️ this bundle looks to be fairly big - you might want to double-check whether that's intended and consider performance implications for your users: `dist/bundle.js`
βœ“ dist/bundle.js
βœ“ dist/bundle_alt.js
βœ“ dist/bundle_legacy.js

format: "iife"
},
reporting: {
threshold: 100000 // ~100 kB
Copy link
Contributor Author

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 πŸ€·β€β™‚οΈ

Copy link

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.

Copy link
Contributor Author

@FND FND Jul 11, 2018

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 });
Copy link
Contributor Author

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)} %)`);
Copy link
Contributor Author

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.

@moonglum
Copy link
Member

moonglum commented Jul 8, 2018

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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants