-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
report(viewer): minify inlined report-generator bundle #9596
report(viewer): minify inlined report-generator bundle #9596
Conversation
8b389c0
to
ced584d
Compare
ced584d
to
7aefe88
Compare
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.
would fix the first checkbox in #2591!
I have my doubts that it will get accepted but i've already pushed it to my own npm account to get feedback on this change.
given the rate of change on brfs I also don't hold out a lot of hope, but it's worth a try :) Implementation looks 👍
build/build-viewer.js
Outdated
/** | ||
* @param {string} file | ||
*/ | ||
readFileSyncTransform: file => { |
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.
probably pull this out into a top-level named function
i love how you made webpack in browserify :) |
@connorjclark it's now called wardpack 😂 |
* | ||
* @param {string} file | ||
*/ | ||
function minifyReadFileContent(file) { |
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 couldn't really find a proper name for this function, suggestions are welcome
I updated this PR but don't know how to push my changes to this PR, so I just opened a new one ... #9823 let's do thissss |
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're going to live with @wardpeet/brfs
for now until/if browserify/brfs#94 gets looked at :)
My legacy will live on! 🎉 |
Summary
Brfs inlines readFileSync functions from report/html/html-report-assets.js inside viewer.js to generate an html report when clicking on saveHtml.
I've added a PR 94 to enable options to transform the file content. I have my doubts that it will get accepted but i've already pushed it to my own npm account to get feedback on this change. I'm happy to publish this under a different namespace or packagename.
The new options allow us to run terser on our js files and compress them. We'll save 18.96KB. The smaller the better is what I was thinking especially for a tool like Lighthouse 💪.
Before:
After:
This commit e4bc45a is the one you should look at.
This PR builds further on #9594