From 4cfd4d93cb264cc090b7a40581d71b4bff091772 Mon Sep 17 00:00:00 2001 From: kevmoo Date: Wed, 1 Apr 2026 12:08:57 -0700 Subject: [PATCH 1/3] refactor(ci): enhance lint script with programmatic ESLint and line-level filtering ### Description Refactored `scripts/lint-changed-files.ts` to use the programmatic ESLint API instead of spawning `execSync` child processes. This improves performance and allows for robust line-level filtering of lint errors. - Extracted monolithic `main` function into modular helpers: `getChangedFiles`, `getChangedLines`, `runLint`, `reportStandard`, and `reportFiltered`. - Unified the ESLint execution path. - Replaced inline `process.exit` with a custom `LintError` class handled at the top level. - Updated `.github/workflows/node-test.yml` to use `--only-changed-lines` by default. ### Scenarios Tested - Ran standard mode to verify it reports all lints for modified files. - Ran `--only-changed-lines` mode to verify it filters out pre-existing warnings on unchanged lines. - Verified that syntax errors on changed lines are correctly reported. ### Sample Commands ```bash npm run lint:changed-files npm run lint:changed-files -- --only-changed-lines ``` --- .github/workflows/node-test.yml | 2 +- scripts/lint-changed-files.ts | 186 ++++++++++++++++++++++++++------ 2 files changed, 153 insertions(+), 35 deletions(-) diff --git a/.github/workflows/node-test.yml b/.github/workflows/node-test.yml index 9e3e258232b..ac0ffcce097 100644 --- a/.github/workflows/node-test.yml +++ b/.github/workflows/node-test.yml @@ -37,7 +37,7 @@ jobs: - run: npm i -g npm@9.5 - run: npm ci - - run: npm run lint:changed-files + - run: npm run lint:changed-files -- --only-changed-lines - run: npm run lint working-directory: firebase-vscode diff --git a/scripts/lint-changed-files.ts b/scripts/lint-changed-files.ts index 07585272bbb..6fb929bf7d2 100644 --- a/scripts/lint-changed-files.ts +++ b/scripts/lint-changed-files.ts @@ -4,35 +4,52 @@ */ import { execSync } from "child_process"; -import { extname, resolve } from "path"; +import { extname, relative, resolve } from "path"; + +interface EslintInstance { + lintFiles(files: string[]): Promise; + loadFormatter(name?: string): Promise<{ + format(results: EslintResult[]): string; + }>; +} + +// eslint-disable-next-line @typescript-eslint/no-var-requires +const { ESLint } = require("eslint") as { + ESLint: { + new (options?: { fix?: boolean }): EslintInstance; + outputFixes(results: EslintResult[]): Promise; + }; +}; const root = resolve(__dirname, ".."); -const deletedFileRegex = /^D\s.+$/; -const extensionsToCheck = [".js", ".ts"]; +class LintError extends Error { + constructor(public mode: "filtered" | "unfiltered") { + super(`Lint errors found (${mode})`); + this.name = "LintError"; + } +} -/** - * Returns the last element of an array. - * @param arr any array. - * @return the last element of the array. - */ -function last(arr: Array): T { - return arr[arr.length - 1]; +interface EslintMessage { + ruleId: string; + severity: number; + message: string; + line: number; + column: number; } -/** - * Main function of the script. - */ -function main(): void { +interface EslintResult { + filePath: string; + messages: EslintMessage[]; + errorCount: number; + warningCount: number; +} + +function getChangedFiles(cmpBranch: string): { files: string[]; ignored: string[] } { const files: string[] = []; const ignoredFiles: string[] = []; - - const otherArgs = process.argv.slice(2); - - let cmpBranch = "main"; - if (process.env.CI) { - cmpBranch = "origin/main"; - } + const deletedFileRegex = /^D\s.+$/; + const extensionsToCheck = [".js", ".ts"]; const gitOutput = execSync(`git diff --name-status ${cmpBranch}`, { cwd: root }) .toString() @@ -40,21 +57,118 @@ function main(): void { for (const line of gitOutput.split("\n")) { const l = line.trim(); + if (!l) continue; if (deletedFileRegex.test(l)) { continue; } const entries = l.split(/\s/); - const file = last(entries); + const file = entries[entries.length - 1]; if (extensionsToCheck.includes(extname(file))) { files.push(file); } else { ignoredFiles.push(file); } } + return { files, ignored: ignoredFiles }; +} + +function getChangedLines(cmpBranch: string): Record> { + const diffOutput = execSync(`git diff -U0 ${cmpBranch}`, { cwd: root }).toString(); + const changedLinesByFile: Record> = {}; + let currentFile = ""; + + for (const line of diffOutput.split("\n")) { + if (line.startsWith("+++ b/")) { + currentFile = line.substring(6); + changedLinesByFile[currentFile] = new Set(); + } else if (line.startsWith("@@ ")) { + const match = /^@@\s+-\d+(?:,\d+)?\s+\+(\d+)(?:,(\d+))?\s+@@/.exec(line); + if (match && currentFile) { + const start = parseInt(match[1], 10); + const length = match[2] ? parseInt(match[2], 10) : 1; + for (let i = 0; i < length; i++) { + changedLinesByFile[currentFile].add(start + i); + } + } + } + } + return changedLinesByFile; +} + +async function runLint( + files: string[], + otherArgs: string[], +): Promise<{ results: EslintResult[]; eslint: EslintInstance }> { + const fix = otherArgs.includes("--fix"); + const eslint = new ESLint({ fix }); + const results = await eslint.lintFiles(files); + + if (fix) { + await ESLint.outputFixes(results); + } + return { results, eslint }; +} + +async function reportStandard(results: EslintResult[], eslint: EslintInstance): Promise { + const formatter = await eslint.loadFormatter("stylish"); + const resultText = formatter.format(results); + console.log(resultText); + + const errorCount = results.reduce((acc: number, r: EslintResult) => acc + r.errorCount, 0); + if (errorCount > 0) { + throw new LintError("unfiltered"); + } +} + +function reportFiltered( + results: EslintResult[], + changedLinesByFile: Record>, +): void { + let errorCount = 0; + + for (const result of results) { + const relPath = relative(root, result.filePath); + const changedLines = changedLinesByFile[relPath] || new Set(); + + const filteredMessages = result.messages.filter((msg: EslintMessage) => + changedLines.has(msg.line), + ); + + if (filteredMessages.length > 0) { + console.log(`\n${relPath}`); + for (const msg of filteredMessages) { + const severity = msg.severity === 2 ? "error" : "warning"; + console.log(` ${msg.line}:${msg.column} ${severity} ${msg.message} ${msg.ruleId}`); + if (msg.severity === 2) { + errorCount++; + } + } + } + } - if (ignoredFiles.length) { + if (errorCount > 0) { + console.error(`\nFound ${errorCount} errors on changed lines.`); + throw new LintError("filtered"); + } else { + console.log("\nNo errors found on changed lines."); + } +} + +/** + * Main function of the script. + */ +async function main(): Promise { + const args = process.argv.slice(2); + const onlyChangedLines = args.includes("--only-changed-lines"); + const otherArgs = args.filter((a) => a !== "--only-changed-lines"); + + const cmpBranch = process.env.CI ? "origin/main" : "main"; + + const { files, ignored } = getChangedFiles(cmpBranch); + + if (ignored.length) { console.log("Ignoring changed files:"); - for (const f of ignoredFiles) { + for (const f of ignored) { console.log(` - ${f}`); } console.log(); @@ -65,16 +179,20 @@ function main(): void { return; } - try { - execSync(`eslint ${otherArgs.join(" ")} ${files.join(" ")}`, { - cwd: root, - stdio: ["pipe", process.stdout, process.stderr], - }); - } catch (e: any) { - console.error("eslint failed, see errors above."); - console.error(); - process.exit(e.status); + const { results, eslint } = await runLint(files, otherArgs); + + if (onlyChangedLines) { + const changedLines = getChangedLines(cmpBranch); + reportFiltered(results, changedLines); + } else { + await reportStandard(results, eslint); } } -main(); +main().catch((e) => { + if (e instanceof LintError) { + process.exit(1); + } + console.error("Script failed:", e); + process.exit(1); +}); From 223a1e80a2beef1df51410b897426a2673d4768d Mon Sep 17 00:00:00 2001 From: kevmoo Date: Wed, 1 Apr 2026 13:37:03 -0700 Subject: [PATCH 2/3] fixes --- scripts/lint-changed-files.ts | 139 +++++++++++++++++++++++++++------- 1 file changed, 113 insertions(+), 26 deletions(-) diff --git a/scripts/lint-changed-files.ts b/scripts/lint-changed-files.ts index 6fb929bf7d2..91a13a46298 100644 --- a/scripts/lint-changed-files.ts +++ b/scripts/lint-changed-files.ts @@ -3,8 +3,9 @@ * working branch and runs the linter on them. */ -import { execSync } from "child_process"; +import { execSync, spawn } from "child_process"; import { extname, relative, resolve } from "path"; +import * as readline from "readline"; interface EslintInstance { lintFiles(files: string[]): Promise; @@ -48,21 +49,19 @@ interface EslintResult { function getChangedFiles(cmpBranch: string): { files: string[]; ignored: string[] } { const files: string[] = []; const ignoredFiles: string[] = []; - const deletedFileRegex = /^D\s.+$/; const extensionsToCheck = [".js", ".ts"]; - const gitOutput = execSync(`git diff --name-status ${cmpBranch}`, { cwd: root }) + const gitOutput = execSync(`git diff --diff-filter=d --name-only ${cmpBranch}`, { cwd: root }) .toString() .trim(); + if (!gitOutput) { + return { files, ignored: ignoredFiles }; + } + for (const line of gitOutput.split("\n")) { - const l = line.trim(); - if (!l) continue; - if (deletedFileRegex.test(l)) { - continue; - } - const entries = l.split(/\s/); - const file = entries[entries.length - 1]; + const file = line.trim(); + if (!file) continue; if (extensionsToCheck.includes(extname(file))) { files.push(file); } else { @@ -72,13 +71,28 @@ function getChangedFiles(cmpBranch: string): { files: string[]; ignored: string[ return { files, ignored: ignoredFiles }; } -function getChangedLines(cmpBranch: string): Record> { - const diffOutput = execSync(`git diff -U0 ${cmpBranch}`, { cwd: root }).toString(); +async function getChangedLines( + cmpBranch: string, + files: string[], +): Promise>> { + const args = ["diff", "-U0", cmpBranch]; + if (files.length > 0) { + args.push("--", ...files); + } + + const git = spawn("git", args, { cwd: root }); + const rl = readline.createInterface({ + input: git.stdout, + terminal: false, + }); + const changedLinesByFile: Record> = {}; let currentFile = ""; - for (const line of diffOutput.split("\n")) { - if (line.startsWith("+++ b/")) { + for await (const line of rl) { + if (line.startsWith("diff --git")) { + currentFile = ""; + } else if (line.startsWith("+++ b/")) { currentFile = line.substring(6); changedLinesByFile[currentFile] = new Set(); } else if (line.startsWith("@@ ")) { @@ -92,7 +106,16 @@ function getChangedLines(cmpBranch: string): Record> { } } } - return changedLinesByFile; + + return new Promise((resolvePromise, reject) => { + git.on("close", (code) => { + if (code === 0) { + resolvePromise(changedLinesByFile); + } else { + reject(new Error(`git diff failed with code ${code ?? "unknown"}`)); + } + }); + }); } async function runLint( @@ -109,38 +132,79 @@ async function runLint( return { results, eslint }; } -async function reportStandard(results: EslintResult[], eslint: EslintInstance): Promise { +async function reportStandard( + results: EslintResult[], + eslint: EslintInstance, + quiet: boolean, + maxWarnings: number, +): Promise { + let processedResults = results; + if (quiet) { + processedResults = results + .map((r) => ({ + ...r, + messages: r.messages.filter((m) => m.severity === 2), + errorCount: r.messages.filter((m) => m.severity === 2).length, + warningCount: 0, + })) + .filter((r) => r.messages.length > 0 || r.errorCount > 0); + } + const formatter = await eslint.loadFormatter("stylish"); - const resultText = formatter.format(results); + const resultText = formatter.format(processedResults); console.log(resultText); - const errorCount = results.reduce((acc: number, r: EslintResult) => acc + r.errorCount, 0); + const errorCount = processedResults.reduce( + (acc: number, r: EslintResult) => acc + r.errorCount, + 0, + ); + const warningCount = processedResults.reduce( + (acc: number, r: EslintResult) => acc + r.warningCount, + 0, + ); + if (errorCount > 0) { throw new LintError("unfiltered"); } + + if (maxWarnings >= 0 && warningCount > maxWarnings) { + console.error( + `\nFound ${warningCount} warnings, which exceeds the max-warnings limit of ${maxWarnings}.`, + ); + throw new LintError("unfiltered"); + } } function reportFiltered( results: EslintResult[], changedLinesByFile: Record>, + quiet: boolean, + maxWarnings: number, ): void { let errorCount = 0; + let warningCount = 0; + let filesWithIssues = 0; for (const result of results) { const relPath = relative(root, result.filePath); const changedLines = changedLinesByFile[relPath] || new Set(); - const filteredMessages = result.messages.filter((msg: EslintMessage) => - changedLines.has(msg.line), - ); + const filteredMessages = result.messages.filter((msg: EslintMessage) => { + const lineMatch = changedLines.has(msg.line); + const quietMatch = !quiet || msg.severity === 2; + return lineMatch && quietMatch; + }); if (filteredMessages.length > 0) { + filesWithIssues++; console.log(`\n${relPath}`); for (const msg of filteredMessages) { const severity = msg.severity === 2 ? "error" : "warning"; console.log(` ${msg.line}:${msg.column} ${severity} ${msg.message} ${msg.ruleId}`); if (msg.severity === 2) { errorCount++; + } else { + warningCount++; } } } @@ -149,8 +213,15 @@ function reportFiltered( if (errorCount > 0) { console.error(`\nFound ${errorCount} errors on changed lines.`); throw new LintError("filtered"); + } else if (maxWarnings >= 0 && warningCount > maxWarnings) { + console.error( + `\nFound ${warningCount} warnings on changed lines, which exceeds the max-warnings limit of ${maxWarnings}.`, + ); + throw new LintError("filtered"); + } else if (filesWithIssues > 0) { + console.log(`\nNo errors found on changed lines (found ${warningCount} warnings).`); } else { - console.log("\nNo errors found on changed lines."); + console.log("\nClean on changed lines."); } } @@ -160,7 +231,23 @@ function reportFiltered( async function main(): Promise { const args = process.argv.slice(2); const onlyChangedLines = args.includes("--only-changed-lines"); - const otherArgs = args.filter((a) => a !== "--only-changed-lines"); + + let quiet = false; + let maxWarnings = -1; + const otherArgs: string[] = []; + + for (let i = 0; i < args.length; i++) { + const arg = args[i]; + if (arg === "--only-changed-lines") { + continue; + } else if (arg === "--quiet") { + quiet = true; + } else if (arg === "--max-warnings") { + maxWarnings = parseInt(args[++i], 10); + } else { + otherArgs.push(arg); + } + } const cmpBranch = process.env.CI ? "origin/main" : "main"; @@ -182,10 +269,10 @@ async function main(): Promise { const { results, eslint } = await runLint(files, otherArgs); if (onlyChangedLines) { - const changedLines = getChangedLines(cmpBranch); - reportFiltered(results, changedLines); + const changedLines = await getChangedLines(cmpBranch, files); + reportFiltered(results, changedLines, quiet, maxWarnings); } else { - await reportStandard(results, eslint); + await reportStandard(results, eslint, quiet, maxWarnings); } } From e31071dd534547f2237115693ac556437367cfd4 Mon Sep 17 00:00:00 2001 From: kevmoo Date: Wed, 1 Apr 2026 14:09:11 -0700 Subject: [PATCH 3/3] better? --- scripts/lint-changed-files.ts | 87 ++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 37 deletions(-) diff --git a/scripts/lint-changed-files.ts b/scripts/lint-changed-files.ts index 91a13a46298..62760f9b532 100644 --- a/scripts/lint-changed-files.ts +++ b/scripts/lint-changed-files.ts @@ -3,7 +3,7 @@ * working branch and runs the linter on them. */ -import { execSync, spawn } from "child_process"; +import { spawn, spawnSync } from "child_process"; import { extname, relative, resolve } from "path"; import * as readline from "readline"; @@ -51,9 +51,13 @@ function getChangedFiles(cmpBranch: string): { files: string[]; ignored: string[ const ignoredFiles: string[] = []; const extensionsToCheck = [".js", ".ts"]; - const gitOutput = execSync(`git diff --diff-filter=d --name-only ${cmpBranch}`, { cwd: root }) - .toString() - .trim(); + const gitDiff = spawnSync("git", ["diff", "--diff-filter=d", "--name-only", cmpBranch], { + cwd: root, + }); + if (gitDiff.status !== 0) { + throw new Error(`git diff failed: ${gitDiff.stderr.toString()}`); + } + const gitOutput = gitDiff.stdout.toString().trim(); if (!gitOutput) { return { files, ignored: ignoredFiles }; @@ -175,50 +179,52 @@ async function reportStandard( } } -function reportFiltered( +async function reportFiltered( results: EslintResult[], + eslint: EslintInstance, changedLinesByFile: Record>, quiet: boolean, maxWarnings: number, -): void { - let errorCount = 0; - let warningCount = 0; - let filesWithIssues = 0; - - for (const result of results) { - const relPath = relative(root, result.filePath); - const changedLines = changedLinesByFile[relPath] || new Set(); - - const filteredMessages = result.messages.filter((msg: EslintMessage) => { - const lineMatch = changedLines.has(msg.line); - const quietMatch = !quiet || msg.severity === 2; - return lineMatch && quietMatch; - }); +): Promise { + const filteredResults: EslintResult[] = results + .map((r) => { + const relPath = relative(root, r.filePath); + const changedLines = changedLinesByFile[relPath] || new Set(); + const messages = r.messages.filter((msg) => { + const lineMatch = changedLines.has(msg.line); + const quietMatch = !quiet || msg.severity === 2; + return lineMatch && quietMatch; + }); + return { + ...r, + messages, + errorCount: messages.filter((m) => m.severity === 2).length, + warningCount: messages.filter((m) => m.severity === 1).length, + }; + }) + .filter((r) => r.messages.length > 0); - if (filteredMessages.length > 0) { - filesWithIssues++; - console.log(`\n${relPath}`); - for (const msg of filteredMessages) { - const severity = msg.severity === 2 ? "error" : "warning"; - console.log(` ${msg.line}:${msg.column} ${severity} ${msg.message} ${msg.ruleId}`); - if (msg.severity === 2) { - errorCount++; - } else { - warningCount++; - } - } - } + const formatter = await eslint.loadFormatter("stylish"); + const resultText = formatter.format(filteredResults); + if (resultText) { + console.log(resultText); } + const errorCount = filteredResults.reduce((acc, r) => acc + r.errorCount, 0); + const warningCount = filteredResults.reduce((acc, r) => acc + r.warningCount, 0); + if (errorCount > 0) { - console.error(`\nFound ${errorCount} errors on changed lines.`); throw new LintError("filtered"); - } else if (maxWarnings >= 0 && warningCount > maxWarnings) { + } + + if (maxWarnings >= 0 && warningCount > maxWarnings) { console.error( `\nFound ${warningCount} warnings on changed lines, which exceeds the max-warnings limit of ${maxWarnings}.`, ); throw new LintError("filtered"); - } else if (filesWithIssues > 0) { + } + + if (filteredResults.length > 0) { console.log(`\nNo errors found on changed lines (found ${warningCount} warnings).`); } else { console.log("\nClean on changed lines."); @@ -243,7 +249,14 @@ async function main(): Promise { } else if (arg === "--quiet") { quiet = true; } else if (arg === "--max-warnings") { - maxWarnings = parseInt(args[++i], 10); + const nextArg = args[i + 1]; + if (nextArg && /^\d+$/.test(nextArg)) { + maxWarnings = parseInt(nextArg, 10); + i++; + } else { + console.error("Error: --max-warnings requires a numeric value."); + process.exit(1); + } } else { otherArgs.push(arg); } @@ -270,7 +283,7 @@ async function main(): Promise { if (onlyChangedLines) { const changedLines = await getChangedLines(cmpBranch, files); - reportFiltered(results, changedLines, quiet, maxWarnings); + await reportFiltered(results, eslint, changedLines, quiet, maxWarnings); } else { await reportStandard(results, eslint, quiet, maxWarnings); }