-
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
misc(build): minify report javascript for lightrider report generator #9823
Conversation
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@@ -186,11 +186,15 @@ | |||
}, | |||
{ | |||
"path": "./dist/viewer/src/viewer.js", | |||
"maxSize": "76 kB" | |||
"maxSize": "65 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.
is 58 now.
}, | ||
{ | ||
"path": "./dist/lightrider/report-generator-bundle.js", | ||
"maxSize": "50 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.
is 40 now
package.json
Outdated
@@ -94,12 +94,12 @@ | |||
"@types/update-notifier": "^1.0.2", | |||
"@types/ws": "^4.0.1", | |||
"@types/yargs": "^8.0.2", | |||
"@wardpeet/brfs": "^2.1.0-0", |
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.
@wardpeet did you rename/republish to wardpack?
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.
haha 😂, should I? 😛
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.
no need, but if you want to ;)
I was mostly making sure you didn't publish to a new package w/ possible updates
hehe, you should install https://hub.github.com/, it makes it super easy to checkout prs from other origins. Cool that it's getting merged in! |
6d94f36
to
810f5e4
Compare
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) ℹ️ Googlers: Go here for more info. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@wardpeet, if you consent, can you say the magic phrase: |
@googlebot I consent |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@googlebot i consent, too. |
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.
Looks good to me :)
cont. #9596 (I meant to just update that PR, but apparently I don't know how to push to a PR branch ...)
I updated @wardpeet's work with master, and also minified the assets for the LR report generator. I verified the report still works with a terser pass (if we rely on function or class names, we'd have to pass special flags, but it seems we don't for the report).
Can confirm this saves a lot of bytes.
dist/lightrider/report-generator-bundle.js
247 KB -> 152 KB (38%)dist/viewer/src/viewer.js
309 KB -> 213 KB (31%) (done in #9596)