Skip to content

chore(deps): migrate to TypeScript 6.0.3 - #11767

Open
ovr wants to merge 10 commits into
masterfrom
migrate-typescript-6-0-3
Open

chore(deps): migrate to TypeScript 6.0.3#11767
ovr wants to merge 10 commits into
masterfrom
migrate-typescript-6-0-3

Conversation

@ovr

@ovr ovr commented Sep 4, 2026

Copy link
Copy Markdown
Member

Check List

  • Tests have been run in packages where changes have been made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Description of Changes Made

Bumps typescript from ~5.2.2 to ~6.0.3 in all 43 packages that pin it (cubejs-client-ngx stays on ~5.4.5, pinned by Angular 18) and clears the tsconfig options TypeScript 6.0 turns into hard errors: moduleResolution: node and baseUrl are gone in favour of module/moduleResolution: nodenext, types: ["*"] restores the previous @types/* auto-include now that the default is [], and rootDir is stated explicitly where TS 6 requires it (TS5011). Two packages need a different mode — cubejs-schema-compiler uses moduleResolution: bundler with module: commonjs because antlr4's .d.cts re-exports .d.ts files that are ESM under its own type: module (TS2834, so antlr4 resolves to nothing), and the bundled client-react/playground use bundler too. Because nodenext emits import() verbatim instead of downlevelling it, the two live dynamic-import sites (container.ts loading the user's cube.js, getDriver.ts) are back on require — jest cannot execute a native dynamic import in its CJS VM, an absolute-path ESM import is fatal on Windows, and a native import would put the whole CommonJS module on .default. The remaining source changes are a handful of root causes rather than a long tail (getEnv's Parameters<Vars[T]> alone accounted for 1592 of the 2074 initial errors), @typescript-eslint moves to ^8 with eslint ^8.57.1 and @stylistic/eslint-plugin-ts for the four rules v8 dropped, and cubejs-testing moves to the end of the root reference list because TS 6 keeps a failed module resolution in the cache shared across tsc --build, which broke materialize/crate/redshift on cold builds only.

Verified with a clean yarn tsc, yarn build, yarn lint, packages/cubejs-playground yarn build:lib and per-package yarn unit; the only failures are pre-existing (schema-compiler error-reporter, raw-time-dimension-timezone, client-vue3) or environmental locally (backend-native port conflict, sqlite3 native binding).

🤖 Generated with Claude Code

Bumps `typescript` from `~5.2.2` to `~6.0.3` across all 43 packages that pin it, and
pays off the tsconfig debt TypeScript 6.0 turns into hard errors.
`packages/cubejs-client-ngx` stays on `~5.4.5` — Angular 18 pins it, and the package is
nohoisted with its own copy.

## TypeScript 6.0 changes that hit us

| Change | Action |
| --- | --- |
| `moduleResolution: node` (node10) deprecated → error | `module` + `moduleResolution: nodenext` in `tsconfig.base.json` and the standalone CJS configs |
| `baseUrl` deprecated → error | removed from the base config and all 41 package configs; the `paths` block it anchored mapped `@cubejs-backend/*` to substitutions with no `*` and was inert |
| default `types` changed from every `@types/*` to `[]` | `"types": ["*"]` restores the previous behaviour |
| `rootDir` must now be explicit (TS5011) | set to the previously inferred common source dir in client-react, client-ws-transport and rust/cubestore |
| default `noUncheckedSideEffectImports: true` | disabled in cubejs-playground, whose `.css` side-effect imports have no ambient declaration |

Two packages need a resolution mode other than `nodenext`:

- **schema-compiler** uses `moduleResolution: bundler` (with `module: commonjs`, a pair 6.0
  newly permits, so emit is unchanged): antlr4 ships `types: index.d.cts` re-exporting `.d.ts`
  files that are ESM under its own `type: module`, so every re-export fails with TS2834 and
  the module resolves to nothing.
- **client-react** and **playground** use `bundler`; they are bundled, not run by Node.

## `module: nodenext` emit

nodenext emits `import()` verbatim instead of downlevelling it to `require()`. Both live
sites are converted back to `require`, because jest cannot execute a native dynamic import
in its CJS VM and most packages test against `dist/`:

- `container.ts` loaded the user's `cube.js` through `import()` of an absolute path — fatal on
  Windows via the ESM loader. The `esModuleInterop` default-wrapping it relied on is now
  explicit.
- `getDriver.ts` would have received the whole CommonJS module on `.default`.

`BaseDriver`'s three lazy cloud-SDK loads become typed `require`s so they stay lazy.

## Other source fixes

| Site | Cause |
| --- | --- |
| `env.ts` `getEnv` | `Parameters<Vars[T]>` now yields `any[]`, so every `getEnv('x', { dataSource })` call failed (1592 errors); the signature meant `Parameters<Vars[T]>[0]` all along |
| `LocalQueueDriverConnection` | `@types/ramda` 0.27's `R.filter` returns a union that `R.pipe` cannot thread; replaced the two pipes with array methods |
| `CubeSymbols` | the `Proxy` factories masquerade as cube definitions, now stated in their types |
| `templates/utils.ts` | `reduce` seed `[]` infers `never[]` |
| `native/js/index.ts` | `parseCubestoreResultMessage` is declared `ArrayBuffer` but the neon binding takes a `JsBuffer` |
| `client-core/test/data-blending` | named imports from JSON are not allowed in an ES module (TS1544) |
| `playground` ui-kit types | `@cube-dev/ui-kit` exports only `.`, so the deep `types/shared` import is unreachable and `tasty()`'s inferred type has no portable name (TS2883) |

`cubejs-testing` moves to the end of the root reference list: its tests resolve the driver
packages by name, and TypeScript 6 keeps that failed resolution in the cache shared across
`tsc --build`, which broke materialize/crate/redshift on a cold build.

## Toolchain

`@typescript-eslint` 6.12 does not support TypeScript 6 (8.x peers `>=4.8.4 <6.1.0`), so it
moves to `^8.46.0` with `eslint` at `^8.57.1` — no flat-config migration. The four stylistic
rules v8 dropped (`semi`, `no-extra-semi`, `type-annotation-spacing`, `space-infix-ops`) come
from `@stylistic/eslint-plugin-ts@^3`, the last line supporting ESLint 8. `ts-jest` widens to
`^29.4.12`, which peers `typescript <7`.

Verified: `yarn tsc` from clean, `yarn build`, `yarn lint`, `packages/cubejs-playground`
`yarn build:lib`, and `yarn unit` per package — the only reds are pre-existing
(schema-compiler `error-reporter`, `raw-time-dimension-timezone`, client-vue3) or
environmental (backend-native port conflict, sqlite3 native binding).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ovr
ovr requested review from a team as code owners September 4, 2026 14:40
@github-actions github-actions Bot added driver:mongodb Issues relating to the MongoBI driver driver:redshift Issues relating to the Redshift driver driver:mysql Issues relating to the MySQL/MariaDB driver driver:bigquery Issues related to the BigQuery driver driver:snowflake Issues relating to the Snowflake driver client:core Issues relating to the JavaScript client SDK driver:clickhouse Issues related to the ClickHouse driver driver:athena Issues related to the AWS Athena driver driver:mssql Issues relating to the MSSQL driver driver:prestodb Issues relating to the PrestoDB driver driver:postgres Issues relating to the Postgres driver client:react Issues relating to the React client SDK client:playground Issues relating to the Developer Playground cube store Issues relating to Cube Store backend:cli Issues relating to the CLI utility driver:druid Issues relating to the Druid driver driver:crate rust Pull requests that update Rust code driver:dremio driver:questdb javascript Pull requests that update Javascript code driver:firebolt data source driver driver:databricks labels Sep 4, 2026
);
// `module: nodenext` emits import() verbatim, handing an absolute path to the ESM loader.
// eslint-disable-next-line global-require, import/no-dynamic-require
const file = require(path.join(process.cwd(), 'cube.js'));

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.

Switching to require() drops support for an ESM cube.js.

await import() handled every shape of user config. require() does not:

  • a project with "type": "module" whose cube.js uses export default throws ERR_REQUIRE_ESM on Node 20.0–20.18 (engines here is >=20.0.0, and require(esm) only landed in 20.19/22.12);
  • a cube.js with top-level await throws ERR_REQUIRE_ASYNC_MODULE on every Node version, including 24.

Both used to work. Since the stated blocker is that nodenext hands a raw absolute path to the ESM loader, pathToFileURL fixes that directly and can be used as a fallback:

let file: any;
try {
  // eslint-disable-next-line global-require, import/no-dynamic-require
  file = require(configPath);
} catch (e: any) {
  if (e.code !== 'ERR_REQUIRE_ESM' && e.code !== 'ERR_REQUIRE_ASYNC_MODULE') throw e;
  file = await import(pathToFileURL(configPath).href);
}

If dropping ESM configs is intentional, it's a breaking change and should be called out in the PR description / release notes rather than land inside a TypeScript bump.

Comment thread packages/cubejs-schema-compiler/tsconfig.json Outdated
Comment on lines 686 to +716
@@ -698,7 +700,9 @@ export abstract class BaseDriver implements DriverInterface {
tableName: string
): Promise<string[]> {
// Lazy loading, because it's using azure SDK, which is quite heavy.
return (await import('./storage-fs/gcs.fs')).extractFilesFromGCS(gcsConfig, bucketName, tableName);
// eslint-disable-next-line global-require
const { extractFilesFromGCS } = require('./storage-fs/gcs.fs') as typeof import('./storage-fs/gcs.fs.js');
return extractFilesFromGCS(gcsConfig, bucketName, tableName);
}

protected async extractFilesFromAzure(
@@ -707,6 +711,8 @@ export abstract class BaseDriver implements DriverInterface {
tableName: string
): Promise<string[]> {
// Lazy loading, because it's using azure SDK, which is quite (extremely) heavy.
return (await import('./storage-fs/azure.fs')).extractFilesFromAzure(azureConfig, bucketName, tableName);
// eslint-disable-next-line global-require
const { extractFilesFromAzure } = require('./storage-fs/azure.fs') as typeof import('./storage-fs/azure.fs.js');
return extractFilesFromAzure(azureConfig, bucketName, tableName);

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.

These three are the only sites where await import() was load-bearing for something other than Node: the comments say "Lazy loading, because it's using azure SDK, which is quite (extremely) heavy", and a static require('./storage-fs/azure.fs') is statically analyzable, so webpack/esbuild/ncc will now pull @azure/storage-blob, @aws-sdk/* and @google-cloud/storage into the bundle unconditionally. In plain Node it's still lazy, so this only bites bundled consumers — but the comment now promises something the code no longer delivers for them.

Unlike container.ts and getDriver.ts, these are relative, statically-known specifiers, so await import('./storage-fs/azure.fs.js') would work under nodenext — is the blocker only that jest can't run a native dynamic import in its CJS VM? If so, that's worth stating in the comment (and is fixable with transpileOnly-style module override in the jest tsconfig) rather than changing the shipped emit.

Minor: all three methods stay async while no longer awaiting anything.

export const MemberLabelText = tasty({
// ui-kit re-exports neither `VariantMap` nor `WithVariant`, so the inferred type has no
// portable name (TS2883).
export const MemberLabelText: ComponentType<Props> = tasty({

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.

ComponentType<Props> compiles, but it erases the tasty() return type rather than naming it: callers pass mods={{ missing }}, data-member and data-size (MemberLabel.tsx:70, FilterLabel.tsx:73), so Props has to be permissive enough that typos in any of those now go unchecked too.

If ui-kit genuinely can't name the type, ReturnType<typeof tasty<...>> or an explicit local prop interface ({ mods?: Record<string, boolean> } & Props) keeps more of the signal. Worth an upstream issue on ui-kit to re-export VariantMap/WithVariant so this can go back to inference.

Comment thread tsconfig.base.json
},
"module": "nodenext",
"moduleResolution": "nodenext",
// TypeScript 6.0 defaults `types` to [], which would drop the ambient @types/* globals

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.

"types": ["*"] restores the old behaviour, which is the safe migration move — but it also re-arms the exact footgun TS 6 changed the default for: any @types/* package that lands in the hoisted root node_modules (a transitive dep of any workspace) injects its globals into all 43 packages, so a package can compile against globals it doesn't declare a dependency on. That's roughly how src/... imports ended up resolving via the paths block you're deleting here.

Not something to fix in this PR, but worth a follow-up issue to enumerate types per package.

"noFallthroughCasesInSwitch": true,
"moduleResolution": "node",
"moduleResolution": "bundler",
"noUncheckedSideEffectImports": false,

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.

This turns off a new TS 6 check for the whole package to satisfy (presumably) a handful of CSS/asset side-effect imports. A declare module '*.css';-style ambient declaration in a src/globals.d.ts keeps the check on for everything else. Low priority, but the flag as written is permanent and invisible.

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.67%. Comparing base (9237c26) to head (37e25ff).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11767      +/-   ##
==========================================
- Coverage   84.67%   84.67%   -0.01%     
==========================================
  Files         261      261              
  Lines       86451    86451              
==========================================
- Hits        73204    73203       -1     
- Misses      13247    13248       +1     
Flag Coverage Δ
cubesql 84.67% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

`tsc --build` only knew the dependency order from the order of the 43 entries in the root
`tsconfig.json`, because almost no package declared its own `references`. A sequential builder
tolerates that; anything that builds projects in parallel does not, and it was already leaking
into TS 6 as the cold-build-only failure that the "keep cubejs-testing last" workaround papered
over. References are now generated from each package's workspace dependencies (142 edges, no
cycles), so the root list is back to its original order and the workaround is gone.

`rust/cubestore` and `cubejs-client-ws-transport` gain `composite: true` — both are referenced
but were never valid reference targets (TS6306).

Three inferred types in schema-compiler's `.js` were wrong rather than merely loose, and the
generated declarations spread them across every consumer:

| Member | Emitted before | Now |
| --- | --- | --- |
| `BaseQuery.resolveFullMemberPathFn` | `(arg0: member) => string`, where `member` names nothing | `(member: string) => string` |
| `BaseQuery.collectLeafMeasures` | `unknown[]` | `string[]` |
| `TestUtil.debugLog` | `(...args: any[]) => void` inferred from `arguments` | declared rest parameter |

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ovr's task in 4m 26s —— View job


Re-review of 94276df (project references) — 1 medium-low, 3 low. The 6 prior threads all still apply.

Full review — what changed, findings, and what I verified
  • Diff the new commit against the previously reviewed state
  • Validate the generated reference graph (targets exist, composite, cycles, missing edges)
  • Review the schema-compiler .js type fixes
  • Check prior threads for staleness
  • Post review

Scope

Only 94276df is new since the last review. It replaces the "keep cubejs-testing last" ordering workaround with real references on 43 packages, adds composite: true to the two projects that were referenced but not valid targets, and fixes three inferred types in schema-compiler's .js.

This is the right fix for the right reason — the root tsconfig.json ordering was load-bearing state that nothing enforced, and the cold-build-only failure was a symptom rather than a quirk. Deriving edges from workspace dependencies is also the only maintainable way to do this at 142 edges.

Findings

# Severity Location Issue
1 Medium-low packages/cubejs-materialize-driver/tsconfig.json:8-13 Missing ../cubejs-testing-sharedtest/MaterializeDriver.test.ts:1 imports it and include covers test, but package.json declares @cubejs-backend/testing (unimported) instead, so generation missed it. Works only because the spurious ../cubejs-testing edge supplies it transitively — the obvious cleanup reintroduces the exact ordering bug this commit removes the workaround for.
2 Low packages/cubejs-dbt-schema-extension/tsconfig.json:13 Same unused ../cubejs-testing edge: two published packages now put the cypress-typed integration harness on the critical path of a cold build for no types. Prune together with #1.
3 Low packages/cubejs-schema-compiler/src/adapter/BaseQuery.js:5665 Second JSDoc block stacked on resolveFullMemberPathFn; TS reads only the nearest one, so the existing description is dropped from the .d.ts. Fix the broken @returns tag in the first block instead.
4 Nit packages/cubejs-client-ws-transport/tsconfig.json:11 /* Generates a sourcemap … '.d.ts' file. */ got carried from declarationDir onto the new composite line, where it's wrong.

Verified as correct (no action needed)

  • Root tsconfig.json is now byte-identical to master (git diff origin/master...HEAD -- tsconfig.json is empty), so the ordering workaround really is gone rather than relocated.
  • No cycles on the edges I traced by hand, including the ones most likely to close: schema-compiler → mssql-driver → base-driver, postgres-driver → testing-shared → schema-compiler, materialize-driver → cubejs-testing → postgres-driver. Nothing references back into schema-compiler or cubejs-testing.
  • Every reference target is composite: all packages/* targets inherit composite: true from tsconfig.base.json, and the two standalone ones (client-core, and now client-ws-transport / rust/cubestore) set it directly. rust/cubestore is in the root list (tsconfig.json:32), so --build reaches it.
  • cubejs-testing's own edges are complete — I grepped every from '@cubejs-backend/*' in its src and test: only shared, testing-shared, base-driver, cubestore-driver, schema-compiler, postgres-driver, all present. The removed "keep last" comment isn't papering over anything that remains.
  • testing-shared edges elsewhere — athena, bigquery, clickhouse, crate, dremio, firebolt, mysql, postgres, questdb, duckdb all have it; materialize (ReferenceError: self is not defined - Issue with whatwg-fetch when using the core lib with Node.js #1) is the only outlier, and crate/redshift (the other two cold-build failures named in the description) are correctly covered.
  • collectLeafMeasuresstring[] — the pipe maps to false | string then R.filter(R.identity), so string[] matches runtime; TS genuinely can't infer through Ramda's pipe here.
  • TestUtil.debugLogexports.debugLog(...) → direct call is safe (no caller monkeypatches it), the rest parameter replaces arguments, and the two commented-out debugLog lines went with it.

Prior threads

All six are still unresolved and still applicable — none of the files they sit on changed in a way that addresses them (container.ts, BaseDriver.ts, MemberLabelText.tsx, tsconfig.base.json, playground/tsconfig.json are untouched by this commit; schema-compiler/tsconfig.json:10 still has moduleResolution: bundler). Nothing resolved, no duplicates posted.

Not checked

Still no yarn tsc / yarn build here — a cold install of 43 packages isn't feasible in this job, and my sandbox blocked the script I wanted to use to machine-verify the full 142-edge graph, so findings #1 and #2 come from hand-tracing the edges most likely to be wrong rather than an exhaustive pass. CI remains the authority on the build claim.

· branch migrate-typescript-6-0-3 ·

Comment thread packages/cubejs-materialize-driver/tsconfig.json
Comment thread packages/cubejs-dbt-schema-extension/tsconfig.json Outdated
Comment thread packages/cubejs-client-ws-transport/tsconfig.json Outdated
Comment on lines +5665 to +5667
/**
* @returns {(member: string) => string}
*/

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 JSDoc blocks now stack on one member. TypeScript reads only the one nearest the declaration, so the fix works — but it also silently drops the description just above it ("Returns a function that constructs the full member path based on the query's join structure") from the emitted .d.ts and from hover, which is the part worth keeping. The broken {(function(member: string): (string))} tag in the first block can just be fixed in place instead, leaving one block:

  /**
   * Returns a function that constructs the full member path
   * based on the query's join structure.
   * @returns {(member: string) => string}
   */

Fix this →

Adding `composite` to cubejs-client-ws-transport carried `declarationDir`'s trailing comment
along with it, where it describes something else entirely.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ovr's task in 1m 5s —— View job


Re-review of 7a52717 — no new findings. Nit resolved; the 8 remaining threads still apply.

Full review — scope, what I checked, and thread status
  • Diff the new commit against the previously reviewed state
  • Review the change
  • Re-check the comment-trimming commit (5190821) against the comment guidelines
  • Resolve stale threads / dedupe against prior comments
  • Post review

Scope

Only 7a52717 is new since the last review, and it's a one-line change: dropping the /* Generates a sourcemap for each corresponding '.d.ts' file. */ comment that composite: true inherited from declarationDir in packages/cubejs-client-ws-transport/tsconfig.json:11. That's exactly the nit I raised, and dropping rather than relocating is the right call — the declarationMap line two above already says it. I've resolved that thread.

While re-diffing I also went back over 5190821 (the comment-trimming commit), which landed mid-way through my first review and so hasn't been explicitly covered:

  • container.ts:312, getDriver.ts:8-9, schema-compiler/tsconfig.json:5-7 — all now two to three lines, and each keeps the load-bearing part (the nodenext verbatim-emit constraint; TS2834 and why bundler doesn't change the emit). A later edit reverting any of these to import() would reintroduce a real failure, so they earn their place.
  • Dropping // A transpiled export defaultlands on.default was correct — it restated file?.__esModule ? file.default : file.
  • MemberLabelText.tsx:3-4 — trimmed, still names TS2883. The underlying typing concern is unchanged (see the open thread).

Findings

None new. Nothing in this commit changes behaviour, emit, or the reference graph.

Prior threads

Eight of the nine are still unresolved and still applicable — none of the files they sit on changed in this commit, and 5190821 only touched comments on two of them without addressing the substance:

Thread Status
container.ts:314require() drops ESM / top-level-await configs Still applies; comment shortened, code unchanged
schema-compiler/tsconfig.jsonbundler + commonjs split Still applies (the option moved to line 9 as the comment shrank; thread anchor now reads one line low)
BaseDriver.ts:716 — static require defeats lazy cloud-SDK loading for bundlers Untouched
MemberLabelText.tsx:6ComponentType<Props> erases prop typing Still applies; only the comment above it changed
tsconfig.base.json:14types: ["*"] re-arms the hoisting footgun Untouched
playground/tsconfig.json:27 — package-wide noUncheckedSideEffectImports: false Untouched
materialize-driver/tsconfig.json:13 — missing testing-shared edge Untouched
dbt-schema-extension/tsconfig.json:13 — unused cubejs-testing edge Untouched
BaseQuery.js:5667 — stacked JSDoc blocks Untouched

No duplicate inline comments posted.

Not checked

Still no yarn tsc / yarn build in this job — a cold install of 43 packages isn't feasible here. For a comment-only change that's not a meaningful gap; CI remains the authority on the build claim.

· branch [`migrate-typescript-6-0-3`](https://github.com/cube-js/cube/tree/migrate-typescript-6-0-3)

Answers a review question. Redirecting `antlr4` through `paths` and keeping nodenext was
measured: 280 errors, because the failing extensionless specifiers are inside antlr4's own
`.d.ts` files, whose ESM format comes from antlr4's `type: module` and cannot be changed from
the consumer side. The comment now also states the cost of `bundler` — type-checking against
the `import` condition while emitting require() — and the one dependency it currently affects.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ovr's task in 1m 29s —— View job


Re-review of 3e4eabc — comment-only commit; question answered, 1 nit on comment length. 8 prior threads still apply.

Full review — scope, findings, and thread status
  • Diff the new commit against the previously reviewed state
  • Review the change
  • Resolve stale threads / dedupe against prior comments
  • Post review

Scope

3e4eabc is the only new commit and it touches nothing but the comment block in packages/cubejs-schema-compiler/tsconfig.json:5-10. No option values changed (module: commonjs / moduleResolution: bundler are byte-identical), so there is no behavioural, emit, or reference-graph delta to review.

It answers the open question from my first review — whether the antlr4 workaround could be scoped via paths while keeping nodenext. The answer is measured (280 errors) and the reason is the right one: the extensionless specifiers that trip TS2834 are inside antlr4's own .d.ts files, and their ESM format comes from antlr4's type: module, which no consumer-side redirect can reach. That refutes the fix I suggested, so I've resolved that thread. The comment also now states the cost being accepted (bundler checks the import condition while commonjs emits require()), which is what I'd wanted recorded.

Findings

# Severity Location Issue
1 Nit packages/cubejs-schema-compiler/tsconfig.json:9-10 Six-line comment; the closing clause ("of the deps here only lru-cache splits types by condition…") is a point-in-time audit of the 16 direct deps that nothing enforces and that goes stale on the next dep bump — and it's narrower than the risk it reassures about, since bundler picks the import condition for transitively-resolved types too. Suggested a four-line version that keeps the shape of the tradeoff and drops the count.

Nothing else. The load-bearing parts — TS2834, why paths can't help, why module and moduleResolution disagree — all earn their place; a later edit that flipped either option back to nodenext would reintroduce a real build failure.

Prior threads

Eight of the ten are still unresolved and still applicable; none of the files they sit on changed in this commit.

Thread Status
container.ts:314require() drops ESM / top-level-await configs Untouched (medium; still the one behavioural risk in the PR)
BaseDriver.ts:716 — static require defeats lazy cloud-SDK loading for bundlers Untouched
MemberLabelText.tsx:6ComponentType<Props> erases prop typing Untouched
tsconfig.base.json:14types: ["*"] re-arms the hoisting footgun Untouched
playground/tsconfig.json:27 — package-wide noUncheckedSideEffectImports: false Untouched
materialize-driver/tsconfig.json:13 — missing testing-shared edge Untouched
dbt-schema-extension/tsconfig.json:13 — unused cubejs-testing edge Untouched
BaseQuery.js:5667 — stacked JSDoc blocks Untouched
schema-compiler/tsconfig.jsonbundler + commonjs split Resolved — answered by this commit
client-ws-transport/tsconfig.json — misplaced comment Resolved previously

No duplicate inline comments posted.

Not checked

No node_modules in this job, so I could not independently verify either the 280-error figure or the lru-cache declaration-file claim — both taken at face value. Still no yarn tsc / yarn build here; for a comment-only change that's not a meaningful gap, and CI remains the authority on the build claim.

· branch [`migrate-typescript-6-0-3`](https://github.com/cube-js/cube/tree/migrate-typescript-6-0-3)

Comment thread packages/cubejs-schema-compiler/tsconfig.json Outdated
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ovr's task in 32s —— View job


Re-review of 658d62d — suggestion applied verbatim; no new findings. 8 prior threads still apply.

Full review — scope, findings, and thread status
  • Diff the new commit against the previously reviewed state
  • Review the change
  • Resolve stale threads / dedupe against prior comments
  • Post review

Scope

658d62d is the only new commit since the last review, and it touches nothing but the comment block in packages/cubejs-schema-compiler/tsconfig.json:5-8. module: commonjs and moduleResolution: bundler are byte-identical, so there is no behavioural, emit, or reference-graph delta.

It applies my suggested four-line version verbatim, dropping the point-in-time lru-cache audit. That's the right trim: what stays in the file is the shape of the tradeoff (bundler checks the import condition, commonjs emits require()) plus the two facts that stop a later edit from reverting either option — TS2834, and why a paths redirect can't reach specifiers that live inside antlr4. The dep-by-dep audit is preserved in 3e4eabc's commit message, where it can go stale harmlessly.

That thread was already resolved when the previous commit answered the underlying question, so there's nothing further to resolve here.

Findings

None. Nothing in this commit changes code, config values, or the build graph.

Prior threads

Eight threads remain unresolved and still applicable — none of the files they sit on are touched by this commit.

Thread Status
container.ts:314require() drops ESM / top-level-await configs Untouched (medium; still the one behavioural risk in the PR)
BaseDriver.ts:716 — static require defeats lazy cloud-SDK loading for bundlers Untouched
MemberLabelText.tsx:6ComponentType<Props> erases prop typing Untouched
tsconfig.base.json:14types: ["*"] re-arms the hoisting footgun Untouched
playground/tsconfig.json:27 — package-wide noUncheckedSideEffectImports: false Untouched
materialize-driver/tsconfig.json:13 — missing testing-shared edge Untouched
dbt-schema-extension/tsconfig.json:13 — unused cubejs-testing edge Untouched
BaseQuery.js:5667 — stacked JSDoc blocks Untouched

No duplicate inline comments posted, and no new ones needed.

Not checked

No yarn tsc / yarn build in this job — a cold install of 43 packages isn't feasible here, and for a comment-only change that's not a meaningful gap. CI remains the authority on the build claim.

· branch [`migrate-typescript-6-0-3`](https://github.com/cube-js/cube/tree/migrate-typescript-6-0-3)

`docker build` failed with TS6053 for three of the references added in 94276df. The images
copy a subset of the workspace: `testing-drivers.Dockerfile` leaves out the frontend packages
and both it and `dev.Dockerfile` leave out `cubejs-testing`, so a reference to one of those
resolves to a directory with no tsconfig.json.

| Dropped reference | Why it is absent |
| --- | --- |
| server-core -> templates | not copied by testing-drivers.Dockerfile; `@cubejs-backend/templates` comes from the registry there |
| dbt-schema-extension -> testing | `cubejs-testing` is in no image, and it is a devDependency |
| materialize-driver -> testing | same |

Verified by hiding both packages' tsconfig.json and compiling the three packages, which is the
state the image build sees.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Re-reviewing fb5d2ea (Docker reference fix)

  • Diff the new commit against the previously reviewed state
  • Validate the dropped references against the Dockerfiles' copy sets
  • Check prior threads for staleness
  • Post review

View job run

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ovr's task in 2m 55s —— View job


Re-review of fb5d2ea + 6bc23a6 — 1 medium (materialize lost its last testing-shared edge), 1 low. 8 prior threads still apply; 1 resolved.

Full review — scope, findings, and thread status
  • Diff the new commits against the previously reviewed state
  • Validate the dropped references against both Dockerfiles' copy sets
  • Sweep every remaining reference for targets absent from an image
  • Resolve stale threads / dedupe against prior comments
  • Post review

Scope

Two new commits. fb5d2ea drops three references that resolved to directories the images don't copy (TS6053); 6bc23a6 removes the comment fb5d2ea left on the server-core drop.

The diagnosis is right and I verified the copy sets directly:

Dropped edge Image evidence
server-core → templates testing-drivers.Dockerfile has both cubejs-templates COPY lines commented out under # We dont need client libraries; dev.Dockerfile:82,158 does copy it
dbt-schema-extension → testing cubejs-testing is copied by neither image (dev.Dockerfile:79,155 commented, absent from testing-drivers)
materialize-driver → testing same

I also swept the rest of the graph for the same class of failure and found none: no "../cubejs-testing" references remain anywhere; the only client-core / client-ws-transport references live in cubejs-testing and cubejs-testing-drivers, neither of which is copied into any image; nothing references cubejs-backend-maven; and cubejs-cubestore-driver/tsconfig.json:14 → ../../rust/cubestore is safe because both Dockerfiles copy rust/cubestore/ in full.

Findings

# Severity Location Issue
1 Medium packages/cubejs-materialize-driver/tsconfig.json:11 The cubejs-testing edge was the only thing transitively supplying testing-shared, which test/MaterializeDriver.test.ts:1 imports and include covers. Materialize now declares no edge for a package it imports — the exact cold-build shape this PR fixed, on one of the three packages that were failing. cubejs-testing-shared is copied by both images, so adding it directly is Docker-safe.
2 Low packages/cubejs-server-core/tsconfig.json:25 Unlike the other two, templates is a real dependencies edge — src/core/DevServer.ts:9 imports four symbols from it. Dropping it under-declares the graph in every build to satisfy the one image that omits it; copying cubejs-templates in testing-drivers.Dockerfile keeps both.

Both currently work only because the root reference list happens to order cubejs-templates (index 4) and cubejs-testing-shared (index 13) ahead of their consumers — the implicit ordering 94276df deliberately removed.

dbt-schema-extension → testing is a clean drop: nothing under src imports it (src/Dbt.ts:1 only pulls schema-compiler), so that one resolves my earlier thread, which I've marked resolved.

On 6bc23a6

No finding — dropping the comment is defensible and the reasoning survives in fb5d2ea's commit message. Worth being aware that the constraint is now invisible in the file: the references were generated from workspace dependencies, and @cubejs-backend/templates is still a declared dependency of server-core, so a regeneration re-adds the edge and re-breaks docker build. That's an argument for finding #2's Dockerfile fix rather than for restoring the comment.

Prior threads

Eight remain unresolved and still applicable; none of the files they sit on are touched by these commits.

Thread Status
container.ts:314require() drops ESM / top-level-await configs Untouched (medium; still the one behavioural risk in the PR)
BaseDriver.ts:716 — static require defeats lazy cloud-SDK loading for bundlers Untouched
MemberLabelText.tsx:6ComponentType<Props> erases prop typing Untouched
tsconfig.base.json:14types: ["*"] re-arms the hoisting footgun Untouched
playground/tsconfig.json:27 — package-wide noUncheckedSideEffectImports: false Untouched
BaseQuery.js:5667 — stacked JSDoc blocks Untouched
materialize-driver/tsconfig.json:12 — missing testing-shared edge Still applies; superseded by finding #1 on the new line, since the fix direction shifted (was "correct by accident", now genuinely absent)
dbt-schema-extension/tsconfig.json:13 — unused cubejs-testing edge Resolved — the edge is gone

Not checked

No yarn tsc / yarn build / docker build in this job — a cold install of 43 packages plus the Rust toolchain isn't feasible here. The TS6053 claim and the "verified by hiding both packages' tsconfig.json" note in the commit message are taken at face value; CI remains the authority.

· branch migrate-typescript-6-0-3 ·

"references": [
{ "path": "../cubejs-backend-shared" },
{ "path": "../cubejs-base-driver" },
{ "path": "../cubejs-postgres-driver" }

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.

Dropping ../cubejs-testing here is right for the image, but it removes the only thing that was supplying testing-shared, so materialize now has no reference for a package it actually imports: test/MaterializeDriver.test.ts:1 does import { MaterializeDBRunner } from '@cubejs-backend/testing-shared', and include covers test. That's the exact shape of the cold-build failure this PR set out to fix, on one of the three packages that were failing (materialize/crate/redshift) — it now only survives because the root list happens to put cubejs-testing-shared (index 13) before cubejs-materialize-driver (index 24), which is the implicit ordering 94276df removed the workaround for.

cubejs-testing-shared is copied in full by both dev.Dockerfile (line 118) and testing-drivers.Dockerfile, so this edge does not reintroduce the TS6053 you just fixed:

Suggested change
{ "path": "../cubejs-postgres-driver" }
{ "path": "../cubejs-postgres-driver" },
{ "path": "../cubejs-testing-shared" }

…and package.json should say @cubejs-backend/testing-shared in devDependencies instead of @cubejs-backend/testing (line 38), which nothing under src or test imports — otherwise the next regeneration from workspace deps drops the edge again. Supersedes my earlier thread on the old line 12.

Fix this →

{ "path": "../cubejs-base-driver" },
{ "path": "../cubejs-cubestore-driver" },
{ "path": "../cubejs-query-orchestrator" },
{ "path": "../cubejs-schema-compiler" }

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.

This one is different from the other two drops, and worth a second look: ../cubejs-templates is not a devDependency edge that contributes nothing — src/core/DevServer.ts:9 imports AppContainer, DependencyTree, PackageFetcher and DevPackageFetcher from @cubejs-backend/templates, and it's a real dependencies entry (package.json:41). So the graph now under-declares a genuine type edge everywhere, to satisfy one image that omits the package.

Only testing-drivers.Dockerfile skips it; dev.Dockerfile copies packages/cubejs-templates/ in full (lines 82, 158). Uncommenting the two cubejs-templates lines in testing-drivers.Dockerfile (it's under a # We dont need client libraries banner, but templates is a backend package server-core depends on at runtime) keeps the edge declared and drops the divergence between what the image builds and what yarn tsc builds. Today the workspace build only works because the root reference list puts packages/cubejs-templates at index 4 — implicit ordering again.

If you'd rather keep the image lean, that's a reasonable call — but then server-core is type-checking against whatever @cubejs-backend/templates the registry resolved in that image rather than the tree, which is worth knowing.

Fix this →

`getDriver` threw `driverModule.default is not a constructor` in every drivers-tests job.
Replacing the dynamic import with `require` dropped the `esModuleInterop` wrapper that used to
put the module on `.default`, and the driver packages come in two shapes:

| Shape | Packages | Constructor |
| --- | --- | --- |
| `module.exports = DriverClass` from the `index.js` shim | athena, bigquery, postgres, trino, clickhouse, mssql, mysql, snowflake, redshift, oracle, … | the module itself |
| TypeScript `export default` | databricks-jdbc (`main: dist/src/index.js`) | `.default` |

Now branches on `__esModule`, which is what `__importStar` did. Verified by running the
compiled `getDriver('postgres')`, and the same interop in the `cube.js` loader was re-checked
against both a `module.exports = {...}` and an `export default {...}` config.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ovr's task in 3m 53s —— View job


Re-review of 37e25ff — fix is correct; 1 low (third divergent interop rule). 7 prior threads still apply, 1 resolved as superseded.

Full review — verification of every driver's export shape, findings, and thread status
  • Diff the new commit against the previously reviewed state
  • Review the change against each driver's actual export shape
  • Check prior threads for staleness
  • Post review

Scope

37e25ff is the only new commit: getDriver swaps new driverModule.default() for driverModule.__esModule ? driverModule.default : driverModule. This is fallout from the import()require() conversion in 9f0786f — the earlier version leaned on the __importStar wrapper that esModuleInterop used to put around the dynamic import.

Verified — the branch is right, and I checked it against every driver this suite constructs

Shape Packages __esModule Result
index.js shim ending in module.exports = DriverClass athena, bigquery, clickhouse, crate, mssql, mysql, pinot, postgres, questdb, redshift, snowflake, trino (+ duckdb, firebolt, materialize, prestodb) absent the module ✓
plain-JS module.exports = Class oracle (driver/OracleDriver.js), vertica (src/VerticaDriver.js), dremio (driver/DremioDriver.js) absent the module ✓
compiled export default databricks-jdbc (main: dist/src/index.js) true .default

Two things I specifically checked because they could have made this subtly wrong:

  • The shim can't leak __esModule. The for (const [key, module] of Object.entries(fromExports)) loop copies the compiled index's enumerable exports onto the class, but tsc emits __esModule via Object.defineProperty(..., { value: true }), which is non-enumerable — so it is never copied, and the shim drivers correctly take the else branch. It does copy default where the index has one (postgres, clickhouse, mssql, mysql, snowflake, redshift, trino, crate, questdb, pinot), but there default === the class, so both branches would agree.
  • Which packages actually broke. athena/src/index.ts and bigquery/src/index.ts have no export default at all, so the shim had no default to copy and new driverModule.default() was new undefined(). The commit message's table puts oracle under the shim shape — it has no index.js, it's plain-JS module.exports — but the outcome is the same either way, so the fix is unaffected.

The // eslint-disable-next-line new-cap drop is correct now that the value is named DriverClass, and I grepped the rest of cubejs-testing-drivers/src for other .default assumptions — getDriver.ts was the only one.

Findings

# Severity Location Issue
1 Low packages/cubejs-testing-drivers/src/helpers/getDriver.ts:13 Third hand-rolled interop in the repo, and the three disagree: DriverResolvers.ts:41 uses if (module.default), container.ts:320 and this use __esModule. lookupDriverClass is the production path for this exact question and predates the PR; exporting it from server-core (testing-drivers already depends on it) would collapse the divergence. The __esModule rule written here is the more correct one.

On the comment

Three lines, and it earns them — "nodenext emits import() verbatim, so this stays a require()" is the sentence that stops someone reverting to await import() and re-breaking every drivers-tests job, and the two-shape explanation is why the branch exists at all. No trim suggested.

Prior threads

Seven remain unresolved and still applicable; none of the files they sit on are touched by this commit.

Thread Status
container.ts:314require() drops ESM / top-level-await configs Untouched (medium; still the one behavioural risk in the PR)
BaseDriver.ts:716 — static require defeats lazy cloud-SDK loading for bundlers Untouched
MemberLabelText.tsx:6ComponentType<Props> erases prop typing Untouched
tsconfig.base.json:14types: ["*"] re-arms the hoisting footgun Untouched
playground/tsconfig.json:27 — package-wide noUncheckedSideEffectImports: false Untouched
materialize-driver/tsconfig.json:11 — no reference for the testing-shared it imports Untouched
server-core/tsconfig.json:25 — dropped templates edge vs. the Dockerfile Untouched
BaseQuery.js:5667 — stacked JSDoc blocks Untouched
materialize-driver/tsconfig.json:12 (old line) Resolved — superseded by the line-11 thread; its premise (the cubejs-testing edge supplying testing-shared transitively) no longer holds after fb5d2ea

No duplicate inline comments posted.

Not checked

No yarn tsc / yarn build / driver integration run in this job — a cold install of 43 packages plus Docker-backed databases isn't feasible here. The export-shape analysis above is from reading the packages' entry points directly; the "verified by running the compiled getDriver('postgres')" claim in the commit message is taken at face value, and CI on the drivers-tests jobs remains the authority.

· branch migrate-typescript-6-0-3 ·

// while databricks-jdbc compiles from TypeScript and exposes it as `default`.
// eslint-disable-next-line global-require, import/no-dynamic-require
const driverModule = require(`@cubejs-backend/${type}-driver`);
const DriverClass = driverModule.__esModule ? driverModule.default : driverModule;

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 branch itself is right — I checked every driver this suite constructs and the two shapes hold: the index.js shims (athena, bigquery, clickhouse, crate, mssql, mysql, pinot, postgres, questdb, redshift, snowflake, trino) end in module.exports = DriverClass and never get __esModule copied onto them (tsc defines it non-enumerable, so the shim's Object.entries loop skips it), oracle/vertica/dremio are plain-JS module.exports = Class, and databricks-jdbc is the only dist/src/index.js with export default. athena and bigquery are the ones that actually broke: their src/index.ts has no default export at all, so .default was undefined.

Worth noting this is now the third hand-rolled interop in the repo, and the three don't agree on the rule:

  • packages/cubejs-server-core/src/core/DriverResolvers.ts:41if (module.default) return module.default;
  • packages/cubejs-server/src/server/container.ts:320file?.__esModule ? file.default : file
  • here — driverModule.__esModule ? driverModule.default : driverModule

lookupDriverClass is the production path for exactly this question and predates the PR, so a driver package that changes shape has to be reasoned about twice, in two places with different failure modes (a package with __esModule: true and no default yields undefined here but resolves fine there). lookupDriverClass isn't in packages/cubejs-server-core/src/index.ts's export list, but testing-drivers already depends on @cubejs-backend/server-core, so exporting it and calling it here would collapse the divergence — the __esModule rule you've written is the more correct of the two, so it's the one to keep.

Not blocking; the fix as written is correct today.

Fix this →

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

Labels

backend:cli Issues relating to the CLI utility client:core Issues relating to the JavaScript client SDK client:playground Issues relating to the Developer Playground client:react Issues relating to the React client SDK cube store Issues relating to Cube Store cubejs-jdbc-driver data source driver driver:athena Issues related to the AWS Athena driver driver:bigquery Issues related to the BigQuery driver driver:clickhouse Issues related to the ClickHouse driver driver:crate driver:databricks driver:dremio driver:druid Issues relating to the Druid driver driver:duckdb driver:firebolt driver:materialize driver:mongodb Issues relating to the MongoBI driver driver:mssql Issues relating to the MSSQL driver driver:mysql Issues relating to the MySQL/MariaDB driver driver:pinot driver:postgres Issues relating to the Postgres driver driver:prestodb Issues relating to the PrestoDB driver driver:questdb driver:redshift Issues relating to the Redshift driver driver:snowflake Issues relating to the Snowflake driver driver:trino Issues relating to the Trino driver javascript Pull requests that update Javascript code rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants