chore(firestore): add automated presubmit serverless bundle size regression guard (#9075) - #9075
Conversation
…bundle-size-guard
…bundle-size-guard
…bundle-size-guard
…bundle-size-guard
…bundle-size-guard
…bundle-size-guard
There was a problem hiding this comment.
Code Review
This pull request introduces an automated bundle size verification check for Firestore. It adds a budget configuration file (bundle-budget.json), a script (check-bundle-size.js) that bundles and minifies the code using esbuild to verify its size, and a corresponding npm script. The review feedback suggests several improvements: using a unique, OS-level temporary directory via fs.mkdtempSync and os.tmpdir() to avoid concurrency conflicts; quoting paths in the execSync command to handle spaces and using npx --yes for non-interactive CI environments; and adding a precheck-bundle-size script to ensure the project is compiled before running the size check.
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const { execSync } = require('child_process'); | ||
| const budget = require('../bundle-budget.json'); |
There was a problem hiding this comment.
To support creating a secure and collision-free temporary directory using fs.mkdtempSync in the OS temp directory, we should import the native os module.
| const fs = require('fs'); | |
| const path = require('path'); | |
| const { execSync } = require('child_process'); | |
| const budget = require('../bundle-budget.json'); | |
| const fs = require('fs'); | |
| const path = require('path'); | |
| const os = require('os'); | |
| const { execSync } = require('child_process'); | |
| const budget = require('../bundle-budget.json'); |
| const tmpDir = path.join(__dirname, '../.tmp-bundle-check'); | ||
| if (!fs.existsSync(tmpDir)) { | ||
| fs.mkdirSync(tmpDir, { recursive: true }); | ||
| } | ||
|
|
||
| const entrypoint = path.join(tmpDir, 'test_bundle_input.js'); | ||
| const outfile = path.join(tmpDir, 'test_bundle_out.min.js'); | ||
|
|
||
| fs.writeFileSync(entrypoint, ` | ||
| const { Firestore } = require('../build/src/index'); | ||
| const f = new Firestore(); | ||
| console.log(f); | ||
| `, 'utf8'); |
There was a problem hiding this comment.
Creating a static temporary directory .tmp-bundle-check inside the repository can cause conflicts during concurrent runs, pollute the git status if cleanup fails, and trigger watcher rebuilds.
Instead, use fs.mkdtempSync with os.tmpdir() to create a unique temporary directory outside the repository. To make this work seamlessly, resolve the absolute path to the build entrypoint and inject it into the generated file.
| const tmpDir = path.join(__dirname, '../.tmp-bundle-check'); | |
| if (!fs.existsSync(tmpDir)) { | |
| fs.mkdirSync(tmpDir, { recursive: true }); | |
| } | |
| const entrypoint = path.join(tmpDir, 'test_bundle_input.js'); | |
| const outfile = path.join(tmpDir, 'test_bundle_out.min.js'); | |
| fs.writeFileSync(entrypoint, ` | |
| const { Firestore } = require('../build/src/index'); | |
| const f = new Firestore(); | |
| console.log(f); | |
| `, 'utf8'); | |
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'firestore-bundle-')); | |
| const entrypoint = path.join(tmpDir, 'test_bundle_input.js'); | |
| const outfile = path.join(tmpDir, 'test_bundle_out.min.js'); | |
| const buildIndexPath = path.resolve(__dirname, '../build/src/index'); | |
| fs.writeFileSync(entrypoint, '\nconst { Firestore } = require(' + JSON.stringify(buildIndexPath) + ');\nconst f = new Firestore();\nconsole.log(f);\n', 'utf8'); |
|
|
||
| try { | ||
| // Execute esbuild bundling and minification | ||
| execSync(`npx esbuild --bundle --platform=node --target=node18 --minify ${entrypoint} --outfile=${outfile}`, { |
There was a problem hiding this comment.
If the path to the temporary directory or repository contains spaces (common on Windows or certain user directories), the execSync command will fail because the paths are not quoted. Additionally, using npx --yes ensures that esbuild can be run non-interactively in CI environments without prompting for installation.
| execSync(`npx esbuild --bundle --platform=node --target=node18 --minify ${entrypoint} --outfile=${outfile}`, { | |
| execSync('npx --yes esbuild --bundle --platform=node --target=node18 --minify "' + entrypoint + '" --outfile="' + outfile + '"', { |
| "system-test:emulator": "concurrently -p \"[{name}]\" -n \"grpc,rest,enterprise-grpc,enterprise-rest\" -c \"cyan,magenta,blue,yellow\" \"npm:system-test:emulator:grpc\" \"npm:system-test:emulator:rest\" \"npm:system-test:enterprise:emulator:grpc\" \"npm:system-test:enterprise:emulator:rest\"", | ||
| "presystem-test": "npm run compile", | ||
| "conformance": "mocha build/conformance", | ||
| "check-bundle-size": "node scripts/check-bundle-size.js", |
There was a problem hiding this comment.
Running check-bundle-size requires the compiled JS files in the build/ directory to exist. Adding a precheck-bundle-size script ensures that npm run compile is automatically executed beforehand, preventing failures when running the check on a clean clone.
| "check-bundle-size": "node scripts/check-bundle-size.js", | |
| "precheck-bundle-size": "npm run compile", | |
| "check-bundle-size": "node scripts/check-bundle-size.js", |
Part 3 of stacked disentanglement (#8928). Add automated presubmit bundle size regression testing for
@google-cloud/firestorein serverless deployment environments.Summary of Changes
test-bundle-size) usingesbuildto compile and bundle the SDK under simulated serverless production conditions.Internal: b/531788771
📚 Stack Navigation Index