feat: Support named ESM exports across all drivers - #11838
Conversation
`import { PostgresDriver } from '@cubejs-backend/postgres-driver'` threw
`does not provide an export named 'PostgresDriver'`. Node discovers a CJS
module's named exports with cjs-module-lexer, a static source scan, and the
hand-written root `index.js` shim hides them behind a runtime loop:
const toExport = PostgresDriver;
for (const [key, module] of Object.entries(fromExports)) {
toExport[key] = module;
}
module.exports = toExport;
so ESM saw only a synthetic `default`. 27 of the 28 drivers in
DriverDependencies were affected, in three shapes:
| Shape | Packages | Change |
| --- | --- | --- |
| Root `index.js` shim (TS) | 20 | `exports` map routing `import` at the tsc output |
| `main` at dist, no named re-export | 1 (druid) | `export { DruidDriver };` |
| Plain JS, bare `module.exports = Class` | 6 | `module.exports.XDriver = XDriver;` |
| Already correct | 1 (databricks-jdbc) | none |
The tsc-emitted `dist/src/index.js` is already lexer-friendly, so the 20 shim
drivers need no wrapper file — only a conditional `exports` map, matching the
shape cubejs-client-core already uses. The plain-JS one-liner is the idiom
DremioDriver.js already used for `applyParams`.
CommonJS still resolves to the same `index.js`, and both conditions load one
underlying module instance, so `require(pkg) === (await import(pkg)).XDriver`
continues to hold.
Note: on the 20 shim drivers `import X from '@cubejs-backend/x-driver'` now
yields the module namespace rather than the driver class, because Node always
sets a CJS module's ESM `default` to `module.exports` and ignores `__esModule`.
This is not treated as a breaking change: the ESM surface of these packages was
not usable before — no export could be imported by name, and the class only
arrived as `default` as a side effect of the shim assigning it to
`module.exports`, never as a designed entry point. Nothing in the repo or the
docs imports a driver that way; every documented sample uses `require`, which is
unaffected. Named imports are now the one consistent path across all 28 drivers.
Extension-less deep imports (`.../dist/src/PostgresDriver`) also stop resolving
now that subpaths are explicit; `./dist/*` keeps the extension-ful form working,
and nothing in the repo, docs, or examples deep-imports a driver.
Verified by probing every driver in its own subprocess, deriving the expected
class name from CommonJS and asserting ESM exposes the same object: 27 of 28
packages fail before this change, all 28 pass after.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @ovr's task in 4m 57s —— View job Review complete — sound approach, 1 medium + 2 low. 2 inline comments posted.Full review
VerdictThe diagnosis is correct and the fix is the right one. The runtime I verified the mechanical parts against the tree rather than the diff:
The 6 plain-JS one-liners are exactly right, non-breaking in every direction (the class stays FindingsMedium — Low — no Low — no regression test lands with the fix. The subprocess probe described in the commit message ("27 of 28 fail before, 28 pass after") is the right verification, but it lives outside the repo. This is a packaging invariant that stayed broken for years precisely because nothing checked it, and any future edit to a Nits (no action needed)
|
| ".": { | ||
| "types": "./dist/src/index.d.ts", | ||
| "import": "./dist/src/index.js", | ||
| "require": "./index.js" | ||
| }, |
There was a problem hiding this comment.
The types condition now describes a default export that the import condition does not deliver. (Applies to all 20 packages that got this block; postgres is the exemplar.)
dist/src/index.d.ts is emitted from src/index.ts, which ends with export default PostgresDriver;. TypeScript will therefore type-check this under moduleResolution: node16/nodenext/bundler:
import PostgresDriver from '@cubejs-backend/postgres-driver';
new PostgresDriver({ ... }); // typed as the classAt runtime the import condition resolves ./dist/src/index.js, a CJS file, and Node sets a CJS module's ESM default to module.exports — i.e. the namespace object, not the class. The call becomes TypeError: PostgresDriver is not a constructor, with no compile-time signal. Before this PR the same import resolved through main → index.js, whose module.exports is the class, so it worked.
The PR body treats this as acceptable because "the ESM surface of these packages was not usable before". That is true of named imports, but the default import worked and is the form an ESM consumer would already have reached for — and it now fails silently rather than loudly. Since this ships under feat: (a minor), it's worth either preserving the default or calling it out explicitly as breaking.
One file per package preserves both shapes and keeps the whole win:
// index.mjs
import mod from './dist/src/index.js';
export const PostgresDriver = mod.PostgresDriver;
export const PgClient = mod.PgClient;
export default mod.default ?? mod;with "import": "./index.mjs". That is 20 new files; the cheaper alternative is to keep the current shape and document the default change in the release notes, since the type-level mismatch is the part that will actually cost users debugging time. Fix this →
| "exports": { | ||
| ".": { | ||
| "types": "./dist/src/index.d.ts", | ||
| "import": "./dist/src/index.js", | ||
| "require": "./index.js" | ||
| }, | ||
| "./dist/*": "./dist/*", | ||
| "./package.json": "./package.json" | ||
| }, |
There was a problem hiding this comment.
Consider adding a "default" condition as the last key in the "." object (again, applies to all 20):
".": {
"types": "./dist/src/index.d.ts",
"import": "./dist/src/index.js",
"require": "./index.js",
"default": "./index.js"
}Once exports exists, main is no longer consulted, and a resolver whose condition set contains neither import nor require gets no match at all — ERR_PACKAGE_PATH_NOT_EXPORTED — where previously it fell back to main. Node and mainstream bundlers always request one of the two, so this is insurance rather than a known break; the cost is one line, and it restores the pre-PR fallback for anything with a hand-configured conditionNames/resolve.conditions.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11838 +/- ##
==========================================
- Coverage 60.16% 60.16% -0.01%
==========================================
Files 239 239
Lines 19204 19206 +2
Branches 3886 3886
==========================================
+ Hits 11555 11556 +1
- Misses 7099 7100 +1
Partials 550 550
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
import { PostgresDriver } from '@cubejs-backend/postgres-driver'threwdoes not provide an export named 'PostgresDriver'. Node discovers a CJS module's named exports with cjs-module-lexer, a static source scan, and the hand-written rootindex.jsshim hides them behind a runtime loop:so ESM saw only a synthetic
default. 27 of the 28 drivers in DriverDependencies were affected, in three shapes:index.jsshim (TS)exportsmap routingimportat the tsc outputThe tsc-emitted
dist/src/index.jsis already lexer-friendly, so the 20 shim drivers need no wrapper file — only a conditionalexportsmap, matching the shape cubejs-client-core already uses. The plain-JS one-liner is the idiom DremioDriver.js already used forapplyParams.CommonJS still resolves to the same
index.js, and both conditions load one underlying module instance, sorequire(pkg) === (await import(pkg)).XDrivercontinues to hold.Note: on the 20 shim drivers
import X from '@cubejs-backend/x-driver'now yields the module namespace rather than the driver class, because Node always sets a CJS module's ESMdefaulttomodule.exportsand ignores__esModule. This is not treated as a breaking change: the ESM surface of these packages was not usable before — no export could be imported by name, and the class only arrived asdefaultas a side effect of the shim assigning it tomodule.exports, never as a designed entry point. Nothing in the repo or the docs imports a driver that way; every documented sample usesrequire, which is unaffected. Named imports are now the one consistent path across all 28 drivers.Extension-less deep imports (
.../dist/src/PostgresDriver) also stop resolving now that subpaths are explicit;./dist/*keeps the extension-ful form working, and nothing in the repo, docs, or examples deep-imports a driver.Verified by probing every driver in its own subprocess, deriving the expected class name from CommonJS and asserting ESM exposes the same object: 27 of 28 packages fail before this change, all 28 pass after.