-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
improve bundle size benchmarks and add initial skill #7450
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| #!/usr/bin/env node | ||
|
|
||
| import fs from 'node:fs' | ||
| import path from 'node:path' | ||
| import { parseArgs } from 'node:util' | ||
|
|
||
| const { values } = parseArgs({ | ||
| allowPositionals: false, | ||
| options: { | ||
| current: { | ||
| type: 'string', | ||
| default: 'benchmarks/bundle-size/results/current.json', | ||
| }, | ||
| id: { type: 'string' }, | ||
| 'top-sources': { type: 'string', default: '30' }, | ||
| json: { type: 'boolean' }, | ||
| }, | ||
| }) | ||
|
|
||
| if (!values.id) { | ||
| throw new Error('Missing required argument: --id') | ||
| } | ||
|
|
||
| const current = JSON.parse( | ||
| fs.readFileSync(path.resolve(values.current), 'utf8'), | ||
| ) | ||
| const metric = (current.metrics || []).find((item) => item.id === values.id) | ||
|
|
||
| if (!metric) { | ||
| throw new Error(`Unknown bundle-size metric: ${values.id}`) | ||
| } | ||
|
|
||
| if (!metric.sources) { | ||
| throw new Error( | ||
| `No source attribution found for ${values.id}. Re-run measure with --analysis.`, | ||
| ) | ||
| } | ||
|
|
||
| const sourceBytes = new Map() | ||
| for (const chunk of metric.sources) { | ||
| for (const source of chunk.sources || []) { | ||
| sourceBytes.set( | ||
| source.source, | ||
| (sourceBytes.get(source.source) || 0) + source.estimatedBytes, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| const rows = [...sourceBytes] | ||
| .map(([source, estimatedBytes]) => ({ source, estimatedBytes })) | ||
| .sort((a, b) => b.estimatedBytes - a.estimatedBytes) | ||
| .slice(0, Number.parseInt(values['top-sources'], 10)) | ||
|
|
||
| if (values.json) { | ||
| process.stdout.write(JSON.stringify(rows, null, 2) + '\n') | ||
| } else { | ||
| for (const row of rows) { | ||
| process.stdout.write(`${row.estimatedBytes} ${row.source}\n`) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,78 @@ | ||||||||||||||||||||||||||||||||||||
| #!/usr/bin/env node | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| import fs from 'node:fs' | ||||||||||||||||||||||||||||||||||||
| import path from 'node:path' | ||||||||||||||||||||||||||||||||||||
| import { parseArgs } from 'node:util' | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const { values } = parseArgs({ | ||||||||||||||||||||||||||||||||||||
| allowPositionals: false, | ||||||||||||||||||||||||||||||||||||
| options: { | ||||||||||||||||||||||||||||||||||||
| baseline: { type: 'string' }, | ||||||||||||||||||||||||||||||||||||
| current: { | ||||||||||||||||||||||||||||||||||||
| type: 'string', | ||||||||||||||||||||||||||||||||||||
| default: 'benchmarks/bundle-size/results/current.json', | ||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||
| id: { type: 'string' }, | ||||||||||||||||||||||||||||||||||||
| json: { type: 'boolean' }, | ||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if (!values.baseline) { | ||||||||||||||||||||||||||||||||||||
| throw new Error('Missing required argument: --baseline') | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| function readCurrent(filePath) { | ||||||||||||||||||||||||||||||||||||
| return JSON.parse(fs.readFileSync(path.resolve(filePath), 'utf8')) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| function byId(current) { | ||||||||||||||||||||||||||||||||||||
| return new Map((current.metrics || []).map((metric) => [metric.id, metric])) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const baselineById = byId(readCurrent(values.baseline)) | ||||||||||||||||||||||||||||||||||||
| const currentById = byId(readCurrent(values.current)) | ||||||||||||||||||||||||||||||||||||
| const ids = values.id | ||||||||||||||||||||||||||||||||||||
| ? [values.id] | ||||||||||||||||||||||||||||||||||||
| : [...new Set([...baselineById.keys(), ...currentById.keys()])].sort() | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
Comment on lines
+34
to
+37
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fail fast on unknown When Suggested guard const baselineById = byId(readCurrent(values.baseline))
const currentById = byId(readCurrent(values.current))
+
+if (
+ values.id &&
+ !baselineById.has(values.id) &&
+ !currentById.has(values.id)
+) {
+ throw new Error(`Unknown bundle-size metric: ${values.id}`)
+}
+
const ids = values.id
? [values.id]
: [...new Set([...baselineById.keys(), ...currentById.keys()])].sort()📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
| const rows = ids.map((id) => { | ||||||||||||||||||||||||||||||||||||
| const baseline = baselineById.get(id) | ||||||||||||||||||||||||||||||||||||
| const current = currentById.get(id) | ||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||
| id, | ||||||||||||||||||||||||||||||||||||
| baseline: baseline?.gzipBytes, | ||||||||||||||||||||||||||||||||||||
| current: current?.gzipBytes, | ||||||||||||||||||||||||||||||||||||
| delta: | ||||||||||||||||||||||||||||||||||||
| Number.isFinite(baseline?.gzipBytes) && | ||||||||||||||||||||||||||||||||||||
| Number.isFinite(current?.gzipBytes) | ||||||||||||||||||||||||||||||||||||
| ? current.gzipBytes - baseline.gzipBytes | ||||||||||||||||||||||||||||||||||||
| : undefined, | ||||||||||||||||||||||||||||||||||||
| initialDelta: | ||||||||||||||||||||||||||||||||||||
| Number.isFinite(baseline?.initialGzipBytes) && | ||||||||||||||||||||||||||||||||||||
| Number.isFinite(current?.initialGzipBytes) | ||||||||||||||||||||||||||||||||||||
| ? current.initialGzipBytes - baseline.initialGzipBytes | ||||||||||||||||||||||||||||||||||||
| : undefined, | ||||||||||||||||||||||||||||||||||||
| rawDelta: | ||||||||||||||||||||||||||||||||||||
| Number.isFinite(baseline?.rawBytes) && Number.isFinite(current?.rawBytes) | ||||||||||||||||||||||||||||||||||||
| ? current.rawBytes - baseline.rawBytes | ||||||||||||||||||||||||||||||||||||
| : undefined, | ||||||||||||||||||||||||||||||||||||
| brotliDelta: | ||||||||||||||||||||||||||||||||||||
| Number.isFinite(baseline?.brotliBytes) && | ||||||||||||||||||||||||||||||||||||
| Number.isFinite(current?.brotliBytes) | ||||||||||||||||||||||||||||||||||||
| ? current.brotliBytes - baseline.brotliBytes | ||||||||||||||||||||||||||||||||||||
| : undefined, | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if (values.json) { | ||||||||||||||||||||||||||||||||||||
| process.stdout.write(JSON.stringify(rows, null, 2) + '\n') | ||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||
| for (const row of rows) { | ||||||||||||||||||||||||||||||||||||
| const delta = Number.isFinite(row.delta) | ||||||||||||||||||||||||||||||||||||
| ? `${row.delta >= 0 ? '+' : ''}${row.delta}` | ||||||||||||||||||||||||||||||||||||
| : 'n/a' | ||||||||||||||||||||||||||||||||||||
| process.stdout.write( | ||||||||||||||||||||||||||||||||||||
| `${row.id} ${row.baseline ?? 'n/a'} -> ${row.current ?? 'n/a'} (${delta}) initial=${row.initialDelta ?? 'n/a'} raw=${row.rawDelta ?? 'n/a'} brotli=${row.brotliDelta ?? 'n/a'}\n`, | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| #!/usr/bin/env node | ||
|
|
||
| import fs from 'node:fs' | ||
| import { execFileSync } from 'node:child_process' | ||
| import vm from 'node:vm' | ||
| import { parseArgs } from 'node:util' | ||
|
|
||
| const HISTORY_PATH = 'benchmarks/bundle-size/data.js' | ||
|
|
||
| const { values } = parseArgs({ | ||
| allowPositionals: false, | ||
| options: { | ||
| history: { type: 'string' }, | ||
| id: { type: 'string' }, | ||
| 'top-deltas': { type: 'string', default: '20' }, | ||
| json: { type: 'boolean' }, | ||
|
Comment on lines
+15
to
+16
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validate Invalid or negative values currently degrade into ambiguous output. Add a finite positive-integer check and throw a clear error. Also applies to: 75-75 🤖 Prompt for AI Agents |
||
| }, | ||
| }) | ||
|
|
||
| function parseHistory(raw) { | ||
| const trimmed = raw.trim() | ||
| if (trimmed.startsWith('window.BENCHMARK_DATA')) { | ||
| const sandbox = { window: {} } | ||
| vm.runInNewContext(trimmed, sandbox, { timeout: 1000 }) | ||
| return sandbox.window.BENCHMARK_DATA | ||
|
Comment on lines
+22
to
+25
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: The Node.js documentation explicitly states that the node:vm module is not a security mechanism and should not be used to run untrusted code [1][2]. The official documentation repeatedly emphasizes this warning, clarifying that the term "sandbox" is used in the context of the module only to refer to a separate execution context, not to provide security guarantees [3][4]. It is not recommended as a security boundary because it does not provide true isolation [5]. The node:vm module allows code to be compiled and run within V8 virtual machine contexts, which is primarily a tool for scoping variables and creating separate global environments [1][2]. Malicious code can easily escape these contexts by walking up the prototype chain to access host-realm objects or the host process, effectively bypassing the intended isolation [5]. Consequently, attempting to use node:vm to safely execute untrusted code is considered inherently insecure and is not a valid approach for building secure sandboxes [6][5]. Citations:
🏁 Script executed: # Find the file and examine the parseHistory function and surrounding context
cat -n scripts/benchmarks/bundle-size/history.mjsRepository: TanStack/router Length of output: 2937 🏁 Script executed: # Search for CLI argument handling related to history
rg -i "history" scripts/benchmarks/bundle-size/ -A 2 -B 2Repository: TanStack/router Length of output: 13412 🏁 Script executed: # Search for how the history file is used in the benchmark script
rg -i "\.mjs|history" --type javascript | grep -i benchmark | head -20Repository: TanStack/router Length of output: 94 Do not execute history file payloads with The Since the benchmark data is JSON (optionally wrapped with if (trimmed.startsWith('window.BENCHMARK_DATA')) {
const jsonStr = trimmed.replace(/^window\.BENCHMARK_DATA\s*=\s*/, '').replace(/;$/, '')
return JSON.parse(jsonStr)
}This eliminates arbitrary code execution while parsing the data safely. 🤖 Prompt for AI Agents |
||
| } | ||
| return JSON.parse(trimmed) | ||
| } | ||
|
|
||
| function readHistoryFromGit() { | ||
| for (const ref of ['origin/gh-pages', 'gh-pages']) { | ||
| try { | ||
| return execFileSync('git', ['show', `${ref}:${HISTORY_PATH}`], { | ||
| encoding: 'utf8', | ||
| }) | ||
| } catch {} | ||
| } | ||
|
|
||
| throw new Error( | ||
| `Could not read ${HISTORY_PATH} from origin/gh-pages or gh-pages. Run: git fetch origin gh-pages`, | ||
| ) | ||
| } | ||
|
|
||
| const raw = values.history | ||
| ? fs.readFileSync(values.history, 'utf8') | ||
| : readHistoryFromGit() | ||
| const history = parseHistory(raw) | ||
| const entries = history.entries?.['Bundle Size (gzip)'] || [] | ||
| const previous = new Map() | ||
| const deltas = [] | ||
|
|
||
| for (const entry of entries) { | ||
| for (const bench of entry.benches || []) { | ||
| if (values.id && bench.name !== values.id) { | ||
| continue | ||
| } | ||
|
|
||
| const prior = previous.get(bench.name) | ||
| if (prior !== undefined && prior !== bench.value) { | ||
| deltas.push({ | ||
| id: bench.name, | ||
| delta: bench.value - prior, | ||
| value: bench.value, | ||
| sha: entry.commit?.id, | ||
| message: String(entry.commit?.message || '').split('\n')[0], | ||
| timestamp: entry.commit?.timestamp, | ||
| }) | ||
| } | ||
|
|
||
| previous.set(bench.name, bench.value) | ||
| } | ||
| } | ||
|
|
||
| deltas.sort((a, b) => Math.abs(b.delta) - Math.abs(a.delta)) | ||
| const rows = deltas.slice(0, Number.parseInt(values['top-deltas'], 10)) | ||
|
|
||
| if (values.json) { | ||
| process.stdout.write(JSON.stringify(rows, null, 2) + '\n') | ||
| } else { | ||
| for (const row of rows) { | ||
| process.stdout.write( | ||
| `${row.id} ${row.delta >= 0 ? '+' : ''}${row.delta} => ${row.value} ${row.sha?.slice(0, 12)} ${row.message}\n`, | ||
| ) | ||
| } | ||
| } | ||
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.
Add validation for
--top-sources.Please validate that
--top-sourcesis a positive integer before applyingslice, so bad inputs fail explicitly instead of producing confusing output.Also applies to: 52-52
🤖 Prompt for AI Agents