-
Notifications
You must be signed in to change notification settings - Fork 940
size presubmit check #2720
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
size presubmit check #2720
Conversation
|
||
const repoRoot = resolve(__dirname, '..'); | ||
|
||
const commitHash = process.env.GITHUB_SHA || execSync('git rev-parse HEAD').toString(); |
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 seem to use simple-git
for git operations in other places in this repo.
I'm okay with both. Will leave it up to you whether we want to pursue any consistency.
scripts/report_binary_size.js
Outdated
const fileName = file.split('/').slice(-1)[0] | ||
reports.push(makeReportObject('firebase', fileName, size)) |
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.
const fileName = file.split('/').slice(-1)[0] | |
reports.push(makeReportObject('firebase', fileName, size)) | |
const fileName = file.split('/').slice(-1)[0]; | |
reports.push(makeReportObject('firebase', fileName, size)); |
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.
Done.
scripts/report_binary_size.js
Outdated
return reports; | ||
} | ||
|
||
// @firebase/* |
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.
// @firebase/* | |
// NPM packages |
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.
Done.
scripts/report_binary_size.js
Outdated
} | ||
|
||
return { | ||
log: "https://www.goog.com", |
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 guess this is only applicable to CI runs. So maybe we want to configure it to be some value when on CI and empty when running locally.
Or we can leave this field empty. The only use of this field is to provide a link on the PR comment for developers to examine the original log (when they are surprised by the number.)
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.
updated to report github action url.
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.
A couple of small questions, but overall looks good to me! I'll try to configure our CI to upload this report to the metrics service.
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.
Can you add a brief explanation in the PR description of where the output of the reports goes to and where it fits in for the CI dev workflow? For future reference.
replaced by #2744 |
No description provided.