-
Notifications
You must be signed in to change notification settings - Fork 6.8k
build: round payload size for comparison #7174
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
Conversation
50a8c18
to
7d579fe
Compare
@@ -93,10 +93,10 @@ async function calculatePayloadDiff(database: firebaseAdmin.database.Database, c | |||
|
|||
// Calculate the payload diffs by subtracting the previous size of the FESM ES2015 bundles. | |||
const cdkFullSize = currentPayload.cdk_fesm_2015; | |||
const cdkDiff = cdkFullSize - previousPayload.cdk_fesm_2015; | |||
const cdkDiff = roundFileSize(cdkFullSize - previousPayload.cdk_fesm_2015); |
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.
Why not just use toFixed
?
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 returns a string instead of a number. I don't think re-parsing is better.
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.
Generally you want to keep your values in number format as long as possible and them format them just before or past of displaying them.
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.
Yeah that's what this does now. toFixed()
returns a string instead of a number. The formatting will happen in the Firebase function.
In some cases the payload is still showing up in the CI even if there was no change to the library. This seems to be because the file sizes are not rounded and can sometimes be not very exact. Resulting in numbers like: `material_fesm_2014": 1004.1319999999998`. To have a better output in the GitHub statuses, the file size should be rounded
7d579fe
to
4f69d19
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.
LGTM
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
In some cases the payload is still showing up in the CI even if there was no change to the library. This seems to be because the file sizes are not rounded and can sometimes be not very exact. Resulting in numbers like:
material_fesm_2014": 1004.1319999999998
.To have a more readable output in the gulp task, the file sizes should be rounded. This also makes the diff comparsion in the Firebase function better.
See PRs like: #7169