Skip to content

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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/test-all.yml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ jobs:
github-token: ${{ secrets.GITHUB_TOKEN }}
path-to-lcov: ./lcov-all.info
continue-on-error: true
- name: Generate Size Report
run: yarn size-report
deploy:
name: Canary Deploy
runs-on: ubuntu-latest
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/test-changed.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,5 @@ jobs:
run: yarn build
- name: Run tests on changed packages
run: xvfb-run yarn test:changed
- name: Generate Size Report
run: yarn size-report
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@
"docgen:node": "node scripts/docgen/generate-docs.js --api node",
"docgen": "yarn docgen:js; yarn docgen:node",
"prettier": "prettier --config .prettierrc --write '**/*.{ts,js}'",
"lint": "lerna run --scope @firebase/* --scope rxfire lint"
"lint": "lerna run --scope @firebase/* --scope rxfire lint",
"size-report": "node scripts/report_binary_size.js"
},
"repository": {
"type": "git",
Expand Down
93 changes: 93 additions & 0 deletions scripts/report_binary_size.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
const { resolve } = require('path');
const fs = require('fs');
const { execSync } = require('child_process');

const repoRoot = resolve(__dirname, '..');

const commitHash = process.env.GITHUB_SHA || execSync('git rev-parse HEAD').toString();
Copy link
Contributor

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.

const runId = process.env.GITHUB_RUN_ID || 'local-run-id';

// CDN scripts
function generateReportForCDNScripts() {
const reports = [];
const firebaseRoot = resolve(__dirname, '../packages/firebase');
const pkgJson = require(`${firebaseRoot}/package.json`);

const special_files = [
'firebase-performance-standalone.es2017.js',
'firebase-performance-standalone.js',
'firebase.js'
];

const files = [
...special_files.map(file => `${firebaseRoot}/${file}`),
...pkgJson.components.map(component => `${firebaseRoot}/firebase-${component}.js`)
];

for (const file of files) {
const { size } = fs.statSync(file);
const fileName = file.split('/').slice(-1)[0]
reports.push(makeReportObject('firebase', fileName, size))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

return reports;
}

// @firebase/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// @firebase/*
// NPM packages

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

function generateReportForNPMPacakges() {
const reports = [];
const fields = [
'main',
'module',
'esm2017',
'browser',
'react-native',
'lite',
'lite-esm2017'
];

const packageInfo = JSON.parse(
execSync('npx lerna ls --json --scope @firebase/*', { cwd: repoRoot }).toString()
);

for (const package of packageInfo) {
const packageJson = require(`${package.location}/package.json`);

for (const field of fields) {
if (packageJson[field]) {
const filePath = `${package.location}/${packageJson[field]}`;
const { size } = fs.statSync(filePath);
reports.push(makeReportObject(packageJson.name, field, size));
}
}
}

return reports;
}

function makeReportObject(sdk, type, value) {
return {
sdk,
type,
value
};
}

function generateSizeReport() {
const reports = [
...generateReportForCDNScripts(),
...generateReportForNPMPacakges()
];

for (const r of reports) {
console.log(r.sdk, r.type, r.value);
}

return {
log: "https://www.goog.com",
Copy link
Contributor

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.)

Copy link
Member Author

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.

metric: "BinarySize",
results: reports
};
}

generateSizeReport();