Update lint:tsc to run against all packages & remove it from CI - #10215
Update lint:tsc to run against all packages & remove it from CI#10215mcmire wants to merge 5 commits into
Conversation
Currently, many test files across the monorepo exhibit type errors. These errors are not checked by `yarn build` (as test files are not production code), and so they have been allowed to accumulate. To help work through these errors, a package script, `lint:tsc`, was introduced and added to CI. This script calls `tsc` to typecheck files as `yarn build` does, but writes the output files to a hidden directory. To prevent from having to address all type errors at once, this typechecking step was initially applied not to the entire monorepo, but only to a small list of packages. Unfortunately, because of the complex nature of the monorepo, this strategy has accidentally created an an obstacle for other engineers. If a package introduces a dependency that is _not_ within the `lint:tsc` allowlist, there is a chance that it will accidentally get included anyway, thus forcing engineers to employ less-than-ideal practices to work around type errors. It's still worth it to take an incremental approach to resolve type errors, but we can do so in a less invasive way. This commit accomplishes this by changing `lint:tsc` to cover _all_ packages, and at the same time, it removes the package script from CI. This increases the potential for type errors to be introduced; however, it gives everyone freedom to fix these errors as they have time, and it allows engineers (especially agents) to use `yarn lint:tsc` as a way to programmatically verify fixes.
| packageNames: expectedPackageNames, | ||
| workspaces, | ||
| }); | ||
| const lintWorkspaces = await filterWorkspacesWithTsconfig({ |
There was a problem hiding this comment.
Previously, this code allowed a subset of packages to contain a tsconfig.lint.json. If this file was present for a package then the references field in the config file would be checked against the dependencies of the package (omitting any dependencies that did not have a tsconfig.lint.json). This logic allowed lint:tsc to be progressively rolled out across the monorepo; but it was not a good idea in hindsight, as it would inevitably force engineers to add tsconfig.lint.json for new dependencies (but then this would force them to fix existing type errors).
These changes remove that logic and simplify this file so that every package must now have a tsconfig.lint.json, and references must perfectly reflect dependencies (just like tsconfig.json and tsconfig.build.json).
|
Adding |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 223f14f. Configure here.
| }): Promise<TsconfigLintMetaReport> { | ||
| const lintTsconfig = await readTsconfig(repoRoot, 'tsconfig.lint.json'); | ||
| // This allows us to increase linting for the whole repo incrementally. | ||
| const lintWorkspaces = await filterWorkspacesWithTsconfig({ |
There was a problem hiding this comment.
We don't need this either, for the same reason as I noted in lint-package-tsconfigs.mts.
1 participant
Explanation
Currently, many test files across the monorepo exhibit type errors. These errors are not checked by
yarn build(as test files are not production code), and so they have been allowed to accumulate.To help work through these errors, a package script,
lint:tsc, was introduced and added as a step to thelintjob in CI. This script callstscto typecheck files asyarn builddoes, but writes the output files to a hidden directory. To prevent from having to address all type errors at once, this typechecking step was initially applied not to the entire monorepo, but only to a small list of packages.Unfortunately, because of the complex nature of the monorepo, this strategy has accidentally created an an obstacle for other engineers. If a package introduces a dependency that is not within the
lint:tscallowlist, there is a chance that it will accidentally get included anyway, thus forcing engineers to employ less-than-ideal practices to work around type errors.It's still worth it to take an incremental approach to resolve type errors, but we can do so in a less invasive way. This commit accomplishes this by changing
lint:tscto cover all packages, and at the same time, it removes the package script from CI. This increases the potential for type errors to be introduced; however, it gives everyone freedom to fix these errors as they have time, and it allows engineers — particularly agents — to useyarn lint:tscas a way to programmatically verify fixes.References
Related to https://consensyssoftware.atlassian.net/browse/WPC-1275.
Checklist
Note
Medium Risk
CI no longer enforces monorepo TypeScript checks, so type regressions can land unless teams run
lint:tsclocally; the change is tooling-only but affects how quality gates work.Overview
yarn lint:tscnow typechecks the whole monorepo, and it is no longer run in CI or in the rootyarn lintchain so PRs are not blocked by existing test-file type debt.This is done by adding
tsconfig.lint.jsonfor many packages (with project references to workspace deps and.tsc-lint-cacheoutput), wiring them into the roottsconfig.lint.json, and updatinglint-tsconfigsscripts so lint configs must reference expected dependencies instead of using an incremental allowlist that skipped packages without a lint tsconfig.Developers and agents can still run
yarn lint:tsclocally to verify fixes incrementally without the old cross-package dependency footgun.Reviewed by Cursor Bugbot for commit 6df99ad. Bugbot is set up for automated code reviews on this repo. Configure here.