Skip to content

ci(integration-tests): type-check in CI, and set a tsconfig target - #7273

Open
corneliusroemer-agent wants to merge 1 commit into
mainfrom
ci/integration-tests-type-check
Open

ci(integration-tests): type-check in CI, and set a tsconfig target#7273
corneliusroemer-agent wants to merge 1 commit into
mainfrom
ci/integration-tests-type-check

Conversation

@corneliusroemer-agent

@corneliusroemer-agent corneliusroemer-agent commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Split out of #7267, where the tsconfig change would have sat unenforced.

Nothing checks types for integration-tests today. Playwright transpiles the tests with its bundled Babel, which strips types without checking them, and it reads only paths, baseUrl and allowJs from tsconfig.json. CI runs format:check — eslint and prettier — and no tsc. The website next door already does both: check-types: astro check && tsc --noEmit, as its own step in website-tests.yml.

eslint here is type-aware (recommendedTypeChecked), which is probably why this hasn't hurt, but it reports rule violations rather than compiler diagnostics. Concretely, while writing #7267 a String.replaceAll call passed npm run format:check and failed tsc with TS2550.

target: ES2022 rather than a lib entry is deliberate — a bare "lib": ["ES2021"] drops the DOM library and produces 142 errors, because playwright-core's own type definitions reference Node and HTMLElementTagNameMap. Setting target derives lib and DOM together.

🚀 Preview: Add preview label to enable

Nothing checked types for this package. Playwright transpiles the tests
with its bundled Babel, which strips types without checking them, and it
reads only paths, baseUrl and allowJs from tsconfig — so nothing ever
told us a type was wrong. CI ran eslint and prettier only.

eslint is type-aware here, but that catches rule violations rather than
compiler diagnostics: a `String.replaceAll` call passed eslint and failed
tsc with TS2550 while this was being written.

Setting `target` rather than `lib` matters. A bare `"lib": ["ES2021"]`
drops the DOM library and produces 142 errors, because playwright-core's
own types reference Node and HTMLElementTagNameMap.

typescript becomes a direct devDependency: it was only present
transitively via typescript-eslint, which is thin ice for a command CI
now depends on.

tsc is clean on main today (84 test files checked), so this starts green.

The website already does this — `check-types: astro check && tsc --noEmit`,
run as its own step in website-tests.yml.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FYXTUKbrzdozhBzYrwVu2V
@corneliusroemer
corneliusroemer marked this pull request as ready for review September 8, 2026 21:25
Copilot AI lite review requested due to automatic review settings September 8, 2026 21:25
@claude

claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @corneliusroemer's task in 2m 51s —— View job


Review: ci(integration-tests): type-check in CI

  • Read PR diff and changed files
  • Verify the new step passes (green "Format Check - integration tests" run on this head, which now includes npm run check-types)
  • Check tsconfig / workflow details
  • Post review

The change does what it says and the gap it closes is real: Playwright's Babel transform strips types without checking them, so integration-tests had no compiler pass at all. target: ES2022 is the right lever — the tests themselves use window.open and window.location inside page.evaluate, so any DOM-less lib would fail on first-party code, not just on playwright-core's declarations. Naming the script check-types to match the website is a nice touch, and pulling typescript up from a hoisted peer into an explicit devDependency makes tsc deterministic.

Nothing here is blocking. Four comments inline:

  • skipLibCheck: true — without it this step checks every .d.ts in node_modules, so a dependency bump can turn it red for reasons unrelated to this repo. Also affects how the "142 errors" number in the description should be read.
  • strict is off — limits what the new check actually catches, and weakens the type-aware eslint rules that read this same tsconfig. Follow-up sized.
  • TypeScript version^5.8.3 here vs ^5.9.3 in the website.
  • Workflow naming + AGENTS.md — the check is still labelled "Format Check", and the local checklist doesn't mention check-types.

No security, performance, or test-coverage concerns: the change adds no runtime code and touches no test behaviour. I couldn't run tsc locally (no network for npm ci in this sandbox), so I relied on the passing CI run for this head commit.
· branch ci/integration-tests-type-check

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes are small, scoped to integration-test tooling/CI, and appear consistent with the stated goal of enforcing TypeScript type checking.

Pull request overview

This PR adds an explicit TypeScript compiler type-check step for the integration-tests package and adjusts its TypeScript configuration so CI enforces type correctness (rather than relying on Playwright’s transpilation and ESLint’s type-aware rules alone).

Changes:

  • Set compilerOptions.target to ES2022 in integration-tests/tsconfig.json so the default lib selection includes both modern ES features and DOM types.
  • Add a check-types npm script (tsc --noEmit) and add typescript as a dev dependency for integration-tests.
  • Update the integration-tests formatting workflow to run the new type-check step in CI.
File summaries
File Description
integration-tests/tsconfig.json Sets TypeScript target to ES2022 to align standard libs with test/runtime expectations.
integration-tests/package.json Adds a check-types script and installs typescript so tsc --noEmit can run.
integration-tests/package-lock.json Locks the added TypeScript dependency and related lockfile metadata changes.
.github/workflows/format-integration-tests.yml Adds a CI step to run npm run check-types in the integration-tests workflow.
Review details

Files not reviewed (1)

  • integration-tests/package-lock.json: Generated file
  • Files reviewed: 3/4 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@@ -1,5 +1,6 @@
{
"compilerOptions": {
"target": "ES2022",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding skipLibCheck: true alongside target:

Suggested change
"target": "ES2022",
"target": "ES2022",
"skipLibCheck": true,

Without it, tsc --noEmit type-checks every .d.ts under node_modules, so a dependabot bump of @playwright/test, @types/node or @lokalise/xlsx can turn this new CI step red for reasons that have nothing to do with the code in this repo. The website inherits skipLibCheck via astro/tsconfigs/strict, so this would also align the two projects.

It's also worth noting for the PR description: the "142 errors" from a bare "lib": ["ES2021"] are errors inside playwright-core's declaration files, which skipLibCheck suppresses outright — so that count isn't really an argument for target over lib. The stronger argument is that the tests themselves need DOM types: window.open in tests/specs/features/search/linkout-modal.dependent.spec.ts:7 and window.location in multi-field-search.dependent.spec.ts:33 are in first-party code and would fail with any DOM-less lib. Either way target: ES2022 is the right call; just make sure that if anyone adds an explicit lib later it keeps "DOM".

"compilerOptions": {
"target": "ES2022",
"esModuleInterop": true,
"moduleResolution": "node"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two follow-ups worth a separate PR (not blockers here):

  • strict is off. With no strict/strictNullChecks, tsc --noEmit catches roughly: unknown identifiers, wrong arity, and lib/target mismatches like the replaceAll case. Implicit any parameters, possibly undefined locator results, and unchecked optional access all pass. This also degrades the eslint setup: typescript-eslint documents that its type-aware rules (no-unnecessary-condition, prefer-nullish-coalescing, …) give worse or disabled results without strictNullChecks, and eslint.config.mjs:19 points project: true at this very file. The website uses astro/tsconfigs/strict.
  • moduleResolution: node is the legacy node10 algorithm and module is left to default (ES2015, since target is now ES2022). node10 ignores exports maps, so tsc resolves dependencies differently from the Node/Playwright loader that actually runs the tests. "module": "NodeNext", "moduleResolution": "NodeNext" would model the runtime; it may need a few import-extension fixes, hence the separate PR.

"eslint-plugin-unicorn": "^74.0.0",
"papaparse": "^5.7.0",
"prettier": "^3.9.6",
"typescript": "^5.8.3",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The website pins typescript: ^5.9.3 (website/package.json:99), and the lockfile here resolves 5.8.3 — it was already present as a hoisted peer of typescript-eslint, which is why the diff is only 6 lines. Declaring it explicitly is right, but the repo now type-checks two projects with two different compiler versions. Suggest matching the website so a TS upgrade lands in one place:

Suggested change
"typescript": "^5.8.3",
"typescript": "^5.9.3",

(needs a npm install in integration-tests/ to refresh package-lock.json.)

run: npm run format:check

- name: Check types
run: npm run check-types

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workflow is name: Format Check - integration tests with job id format, but it now also fails on compiler diagnostics. A red check labelled "Format Check" for a TS2550 is misleading. Something like Checks - integration tests would read better; the cost is that any branch-protection rule referencing the old required-check name has to be updated, so it may not be worth it — your call.

Also worth mirroring on the website side of the fence: integration-tests/AGENTS.md still tells contributors to run only npm run format before committing. Adding npm run check-types there means the local checklist matches what CI enforces (the website's AGENTS.md already lists both).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants