{{ message }}
Repository navigation
ci: add breaking change check workflow for public TypeScript interfaces - #9583
Draft
danieljbruce wants to merge 1 commit into
Draft
danieljbruce wants to merge 1 commit into
danieljbruce wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a tool and test suite to detect breaking changes in public TypeScript interfaces by comparing ASTs and types across git revisions. The review feedback identifies critical improvements for handling renamed files in the compiler host, updating class expressions in the AST transformer, and ensuring test robustness in shallow-cloned CI environments.
Comment on lines
+1475
to
+1489
Contributor
There was a problem hiding this comment.
When a file is renamed, both its old and new paths need to be tracked in changedSet and headToEntry so that the custom compiler host can correctly resolve and read the base version of the file from git history.
const headToEntry = new Map();
for (const e of entries) {
headToEntry.set(e.filePath, e);
headToEntry.set(e.oldFilePath, e);
}
const headIndex = path.join(srcDir, 'index.ts');
const hasIndex =
fs.existsSync(headIndex) ||
getFileAtRev(
baseRev,
path.relative(REPO_ROOT, headIndex).split(path.sep).join('/'),
) !== undefined;
const changedSet = new Set();
for (const e of entries) {
changedSet.add(e.filePath);
changedSet.add(e.oldFilePath);
}
if (hasIndex) changedSet.add(headIndex);
Comment on lines
+1578
to
+1615
| resolveModuleNameLiterals( | ||
| moduleLiterals, | ||
| containingFile, | ||
| redirectedReference, | ||
| options, | ||
| ) { | ||
| const resolvedContaining = path.resolve(containingFile); | ||
| const isFromBase = | ||
| resolvedContaining.startsWith(baseSrcDir + path.sep) || | ||
| resolvedContaining === baseSrcDir; | ||
| const realContaining = toRealPath(resolvedContaining); | ||
|
|
||
| return moduleLiterals.map(lit => { | ||
| const res = ts.resolveModuleName( | ||
| lit.text, | ||
| realContaining, | ||
| options, | ||
| this, | ||
| undefined, | ||
| redirectedReference, | ||
| ); | ||
| if ( | ||
| isFromBase && | ||
| res.resolvedModule && | ||
| changedSet.has(path.resolve(res.resolvedModule.resolvedFileName)) | ||
| ) { | ||
| return { | ||
| resolvedModule: { | ||
| ...res.resolvedModule, | ||
| resolvedFileName: toBasePath( | ||
| res.resolvedModule.resolvedFileName, | ||
| ), | ||
| }, | ||
| }; | ||
| } | ||
| return res; | ||
| }); | ||
| }, |
Contributor
There was a problem hiding this comment.
Using realContaining (which maps to the head/working directory path) for resolving module names in the base program prevents renamed files from being resolved. Since the old file name no longer exists in the head directory, the resolution fails. Passing containingFile directly allows TypeScript to resolve modules relative to the virtual __base_src__ directory, which correctly triggers the custom host's git-backed file checks.
resolveModuleNameLiterals(
moduleLiterals,
containingFile,
redirectedReference,
options,
) {
return moduleLiterals.map(lit => {
return ts.resolveModuleName(
lit.text,
containingFile,
options,
this,
undefined,
redirectedReference,
);
});
},
Comment on lines
+260
to
+269
| if (ts.isClassDeclaration(node)) { | ||
| return ts.factory.updateClassDeclaration( | ||
| node, | ||
| node.modifiers, | ||
| node.name, | ||
| node.typeParameters, | ||
| node.heritageClauses, | ||
| newMembers, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
The AST transformer currently handles ClassDeclaration but misses updating ClassExpression nodes, even though both are checked in the outer if condition. This causes private/protected members of class expressions to not be stripped, which can lead to nominal type check failures during comparison.
if (ts.isClassDeclaration(node)) {
return ts.factory.updateClassDeclaration(
node,
node.modifiers,
node.name,
node.typeParameters,
node.heritageClauses,
newMembers,
);
} else {
return ts.factory.updateClassExpression(
node,
node.modifiers,
node.name,
node.typeParameters,
node.heritageClauses,
newMembers,
);
}
Comment on lines
+344
to
+367
Contributor
There was a problem hiding this comment.
This test relies on historical commits being present in the git history. In environments using shallow clones (such as default GitHub Actions checkouts with fetch-depth: 1), these commits will be missing, causing the test suite to crash. Wrapping the check in a try-catch block to gracefully skip the test when the commits are unavailable ensures robust CI runs.
it('detects the breaking change in PR #9539 (4443d6d6b1) and passes on its revert PR #9549 (d62a64f338)', async (t) => {
let breakingErrors;
try {
breakingErrors = await checkBreakingChanges({
diffRevArgs: ['4443d6d6b1^', '4443d6d6b1'],
baseRev: '4443d6d6b1^',
headRev: '4443d6d6b1',
});
} catch (err) {
if (err.message.includes('bad object') || err.message.includes('fatal')) {
t.skip('Historical commits not available in this clone (shallow clone).');
return;
}
throw err;
}
assert.ok(
breakingErrors.some(
e =>
e.includes("Interface/Type 'ClientConfig': property 'port'") &&
e.includes("was 'string | number | undefined'") &&
e.includes("now 'number | undefined'"),
),
`Expected PR #9539 ClientConfig.port breaking change, got: ${breakingErrors.join('\n')}`,
);
const revertErrors = await checkBreakingChanges({
diffRevArgs: ['d62a64f338^', 'd62a64f338'],
baseRev: 'd62a64f338^',
headRev: 'd62a64f338',
});
assert.deepEqual(revertErrors, []);
});
danieljbruce
added a commit
that referenced
this pull request
Oct 15, 2026
## 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 in `bin/`: 1. **Workspace dependent compilation via `pnpm --filter "...[HEAD^1...HEAD]"`**: Passes `--config.link-workspace-packages=deep --config.prefer-workspace-packages=true` to `pnpm` so modified `core/*` and `handwritten/*` packages are linked and compiled alongside their downstream workspace dependents (catching cross-package breakages like #9496 where a `google-gax` change broke `@google-cloud/pubsub`). 2. **Declaration comparison via `pnpm dlx @api-extractor-tools/change-detector`**: Emits `.d.ts` declarations for modified TypeScript files (`HEAD^1` vs `HEAD`) and runs `pnpm 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 - **CI / Pull Requests**: Automatically checks `core/*` and `handwritten/*` TypeScript changes for breaking public API changes and cross-package compilation regressions. - **Maintainability**: Only adds `.github/workflows/breaking-change-check.yaml` (113 lines) with zero custom `bin/breaking-change-check.mjs` code to maintain. ## Changes - [`.github/workflows/breaking-change-check.yaml`](.github/workflows/breaking-change-check.yaml): Compiles affected packages and workspace dependents via `pnpm --filter "...[HEAD^1...HEAD]"` and checks modified declarations using `pnpm dlx @api-extractor-tools/change-detector`. ## Testing - Verified against #9539 (`4443d6d6b1`) that `pnpm dlx @api-extractor-tools/change-detector` detects `Narrowed type of 'ClientConfig.port' from 'string | number' to 'number'` (`impact: "narrowing"`). - Verified against the revert in #9549 (`d62a64f338`) that widening `ClientConfig.port` back to `string | number` is classified as `impact: "widening"` and passes with zero errors. - Verified `pnpm --config.link-workspace-packages=deep --config.prefer-workspace-packages=true --filter "...[17978e7^1...17978e7]" --filter "!./packages/*"` discovers `@google-cloud/pubsub` and all other `core/*` and `handwritten/*` dependents of `google-gax` (#9496). ## Alternatives - **1,764-line custom AST/TypeChecker script in `bin/breaking-change-check.mjs` (#9583)**: Requires maintaining custom AST transformation passes, custom module resolution, and undocumented internal TypeScript `TypeChecker` calls inside this repository.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a GitHub Actions workflow and TypeScript Compiler API checker that analyzes modified TypeScript source files on pull requests and fails if any public interface, type alias, class, constructor, function, or method signature becomes more restrictive.
Impact
Catches accidental breaking changes in public TypeScript interfaces (such as narrowing
ClientConfig.portfromstring | numbertonumberin #9539) during pull request review as a non-required status check and inline diff annotation without blocking intentional breaking changes from merging.Changes
.github/workflows/breaking-change-check.yamltriggered onpull_requestandworkflow_dispatchto run the breaking change checker againstHEAD^1...HEAD.bin/breaking-change-check.mjs, which uses the TypeScript Compiler API (ts.createProgramandTypeChecker) with an in-memory base/head overlayCompilerHostto compare exported types, interfaces, classes, constructors, functions, methods, and values across modified.tsfiles and each affected package'ssrc/index.tsentrypoint.::error file=...annotations and a$GITHUB_STEP_SUMMARYMarkdown report when breaking changes are detected so PR readers immediately see which public interfaces became more restrictive.bin/test/breaking-change-check.test.mjs.Testing
bin/test/breaking-change-check.test.mjscovering interface property narrowing/widening, optional/required property changes, function/constructor parameter and overload changes, class member visibility restrictions, generic type parameter constraints, and export removals.4443d6d6b1(fix(pubsub): align ClientConfig.port type with gax.GrpcClientOptions #9539) that narrowingClientConfig.portfromstring | numbertonumberfails the check, and verified againstd62a64f338(chore(pubsub): revert port types to reflect numbers and strings #9549) and 6 other recent commits that non-breaking changes pass with zero false positives.Alternatives
tsc --declarationbuild on base and head checkouts: Compiling.d.tsfiles for both revisions across the monorepo is much slower in CI and requires two separate worktrees, whereas an in-memory overlayCompilerHostwithin a singlets.Programper affected package checks only modified packages in a few seconds.