Repository navigation
ci: leverage pnpm for breaking change check - #9634
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new tool, breaking-change-check.mjs, along with its test suite, to detect breaking changes in public TypeScript interfaces across git revisions. The feedback identifies three key areas for improvement: refining the regex used to strip private members to prevent syntax errors on inline types with semicolons, adding a safety check to ensure filePath is defined during git diff parsing to avoid runtime crashes, and improving the pre-compiled .d.ts lookup logic to support different build output structures with or without the src directory prefix.
There was a problem hiding this comment.
The regex private\s+[^;]+; is fragile and can cause syntax errors in the stripped .d.ts files. Specifically, if a private member has an inline type containing a semicolon (e.g., private config: { host: string; port: number; }; or a multi-line signature with inline types), [^;]+ will stop at the first semicolon inside the type definition. Because of the gm flags, the regex will match and replace only the portion up to that first semicolon, leaving the rest of the declaration (e.g., port: number; };) intact, which results in invalid TypeScript syntax and compiler errors.
To make this safer, we can use a greedy match for single-line private members, which will match up to the last semicolon on the line.
| const strippedDts = rawDts | |
| .replace(/^\s*(?:static\s+)?(?:#private;|private\s+[^;]+;)\s*$/gm, '') | |
| .replace(/^(\s*(?:static\s+|readonly\s+|abstract\s+)*)protected\s+/gm, '$1'); | |
| const strippedDts = rawDts | |
| .replace(/^\s*(?:static\s+)?(?:#private;|private\s+.*;)\s*$/gm, '') | |
| .replace(/^(\s*(?:static\s+|readonly\s+|abstract\s+)*)protected\s+/gm, '$1'); |
There was a problem hiding this comment.
| const filePath = status.startsWith('R') ? parts[2] : parts[1]; | ||
| const oldFilePath = parts[1]; | ||
| if (!filePath.endsWith('.ts') || isIgnoredSourceFile(filePath)) continue; |
There was a problem hiding this comment.
If git diff returns an unexpected format or if parts does not contain the expected number of elements, filePath could be undefined. Calling filePath.endsWith or passing it to isIgnoredSourceFile (which calls filePath.split) will then throw a TypeError and crash the script.
We should add a guard to ensure filePath is defined.
| const filePath = status.startsWith('R') ? parts[2] : parts[1]; | |
| const oldFilePath = parts[1]; | |
| if (!filePath.endsWith('.ts') || isIgnoredSourceFile(filePath)) continue; | |
| const filePath = status.startsWith('R') ? parts[2] : parts[1]; | |
| const oldFilePath = parts[1]; | |
| if (!filePath || !filePath.endsWith('.ts') || isIgnoredSourceFile(filePath)) continue; |
There was a problem hiding this comment.
Fixed in e382c51: added a guard in .github/workflows/breaking-change-check.yaml to ensure status, file, and oldPath are defined and that file ends with .ts (excluding .test.ts/.spec.ts) before proceeding.
There was a problem hiding this comment.
If the package's tsconfig.json specifies "rootDir": "src" (which is the default/common setup), the compiled .d.ts files will be emitted directly under build/ (e.g., build/index.d.ts) rather than preserving the src/ directory structure (e.g., build/src/index.d.ts). Since relInPkg includes the src/ prefix, compiledDts will look for build/src/..., which won't exist, causing the tool to silently fall back to transpiling the source file on the fly.
We can make this more robust by checking both paths.
const relInPkg = path.relative(pkgDir, real);
let compiledDts = path.join(
pkgDir,
'build',
relInPkg.replace(/\.ts$/, '.d.ts'),
);
if (!fs.existsSync(compiledDts) && relInPkg.startsWith('src' + path.sep)) {
const strippedRel = relInPkg.substring('src'.length + 1);
const alternativeDts = path.join(pkgDir, 'build', strippedRel.replace(/\.ts$/, '.d.ts'));
if (fs.existsSync(alternativeDts)) {
compiledDts = alternativeDts;
}
}There was a problem hiding this comment.
Fixed in e382c51: added findCompiledDts in .github/workflows/breaking-change-check.yaml to check both build/<relInPkg>.d.ts (e.g. build/src/index.d.ts) and build/<relWithoutSrc>.d.ts (e.g. build/index.d.ts) before falling back to ts.transpileDeclaration.
bc72deb to
7a9a8ed
Compare
danieljbruce
changed the title
2 participants
Description
This PR adds a lightweight,
pnpm-based breaking-change check workflow as an alternative to the 1,764-line custom TypeScript AST/TypeChecker script in #9583, without adding any custom script files inbin/:pnpm --filter "...[HEAD^1...HEAD]": Passes--config.link-workspace-packages=deep --config.prefer-workspace-packages=truetopnpmso modifiedcore/*andhandwritten/*packages are linked and compiled alongside their downstream workspace dependents (catching cross-package breakages like feat(gax): emit T4 attempt spans #9496 where agoogle-gaxchange broke@google-cloud/pubsub).pnpm dlx @api-extractor-tools/change-detector: Emits.d.tsdeclarations for modified TypeScript files (HEAD^1vsHEAD) and runspnpm dlx @api-extractor-tools/change-detector /tmp/base.d.ts /tmp/head.d.ts --json, flagging any removed exports/members (action === 'removed') or narrowed types (impact === 'narrowing').Impact
core/*andhandwritten/*TypeScript changes for breaking public API changes and cross-package compilation regressions..github/workflows/breaking-change-check.yaml(113 lines) with zero custombin/breaking-change-check.mjscode to maintain.Changes
.github/workflows/breaking-change-check.yaml: Compiles affected packages and workspace dependents viapnpm --filter "...[HEAD^1...HEAD]"and checks modified declarations usingpnpm dlx @api-extractor-tools/change-detector.Testing
4443d6d6b1) thatpnpm dlx @api-extractor-tools/change-detectordetectsNarrowed type of 'ClientConfig.port' from 'string | number' to 'number'(impact: "narrowing").d62a64f338) that wideningClientConfig.portback tostring | numberis classified asimpact: "widening"and passes with zero errors.pnpm --config.link-workspace-packages=deep --config.prefer-workspace-packages=true --filter "...[17978e7f9e^1...17978e7f9e]" --filter "!./packages/*"discovers@google-cloud/pubsuband all othercore/*andhandwritten/*dependents ofgoogle-gax(feat(gax): emit T4 attempt spans #9496).Alternatives
bin/breaking-change-check.mjs(ci: add breaking change check workflow for public TypeScript interfaces #9583): Requires maintaining custom AST transformation passes, custom module resolution, and undocumented internal TypeScriptTypeCheckercalls inside this repository.