fix: lazily load database clients via dynamic import()#465
Conversation
|
@elrrrrrrr could you give a bit more information? which bundler couldn't handle static require(var)? |
|
Thanks for asking. To be precise, the concrete case is With the dynamic This happens even when So the change is specifically for this server-bundling case, not a claim about every bundler. Leoric's existing SQLCipher integration test already covers the normal configured-client behavior; I will tighten the PR description accordingly. |
|
One further clarification: esbuild has the same problem when it emits an ESM server bundle. It preserves Minimal reproduction: import { EventEmitter } from 'node:events';
class Pool extends EventEmitter {
constructor(options: { client: string }) {
super();
this.client = require(options.client);
}
}
new Pool({ client: 'node:events' });esbuild pool.ts --bundle --platform=node --format=esm --outfile=pool.mjs
node pool.mjs
# Error: Dynamic require of "node:events" is not supportedThe longer-term direction should therefore be native ESM loading rather than any async loadClient() {
const mod = await import(this.options.client);
this.client = mod.default ?? mod;
}The same fixture bundles and runs successfully with esbuild ESM output. That requires an async initialization boundary for the SQLite client; the |
|
BTW, is there any plan to provide a native ESM entry for Leoric (for example, through conditional exports) alongside the CommonJS build? It would let ESM server bundles load Leoric without a CommonJS |
|
Thanks for the detailed investigation and the reproduction case — very helpful. We're planning to add a first-class Node ESM entry via conditional exports shortly (dual CJS + ESM). Given that direction, we think the better fix for the dynamic The concrete drivers already load clients inside an async This would also cover the mysql driver ( Would you be interested in updating this PR to use
We'll handle the |
|
Thanks — I updated the PR in commit 7785871.
Local validation: the three paths passed both CJS and ESM esbuild bundles at runtime. The ESM bundles externalize package dependencies, matching the intended conditional-exports setup. I left One intentional behavior change to call out: for MySQL/PostgreSQL, |
|
Follow-up: the first upstream CI run exposed two SQLite lazy-initialization regressions. Fixed in 0b3ac8f:
Validated on the fork with the same Node 20 + MySQL 5.7 + PostgreSQL full coverage workflow: https://github.com/elrrrrrrr/leoric/actions/runs/29738803369 |
There was a problem hiding this comment.
Pull request overview
This PR aims to keep optional database client packages (SQLite/MySQL/Postgres) out of static bundler dependency graphs by deferring client/pool creation to runtime, while preserving support for custom client modules/paths.
Changes:
- Updated SQLite pool to lazily load the configured client once and reuse it across concurrent connection requests.
- Refactored MySQL and Postgres drivers to lazily create pools and dynamically load the configured client.
- Updated/added unit tests and fixtures to cover dynamic client loading, concurrency, and deferred “module not found” failures.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/realm.test.js | Adjusts expectations to reflect deferred client resolution (error now occurs on connection acquisition). |
| test/unit/drivers/dynamic-client.test.js | Adds coverage for lazy/dynamic client loading across SQLite/MySQL/Postgres and concurrent calls. |
| test/fixtures/dynamic-client.js | Provides a lightweight fake client module used by the new dynamic-loading tests. |
| src/drivers/sqlite/pool.ts | Implements lazy client loading with a cached promise and retries on failure. |
| src/drivers/postgres/index.ts | Makes pool creation lazy and loads pg (or a custom client) at runtime. |
| src/drivers/mysql/index.ts | Makes pool creation lazy and loads the configured MySQL client at runtime. |
| src/drivers/mysql/attribute.ts | Switches sqlstring usage to a default import and destructuring for escape helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.clientPromise = import(this.options.client ?? 'sqlite3').then((module) => { | ||
| const client = module.default ?? module; | ||
| // Turn on stack trace capturing otherwise the output is useless | ||
| // - https://github.com/mapbox/node-sqlite3/wiki/Debugging | ||
| if (this.options.trace) client.verbose(); | ||
| this.client = client; | ||
| return client; | ||
| }); |
| const module = await import(client); | ||
| const mysql = module.default ?? module; | ||
| return mysql.createPool({ |
| const client = opts.client || 'pg'; | ||
| if (client === 'pg') await import('./type_parser'); | ||
| const module = await import(client); | ||
| const pg = module.default ?? module; | ||
| return new pg.Pool({ host, port, user, password, database }); |
|
copilot seems taking pr desc as the source of truth instead of the adjusted implementation. @elrrrrrrr plz update pr title/desc accordingly |
Summary
getConnection()call across all driversrequire(client)withawait import(client)(ESM-compatible)module.default ?? moduleWhy
Driver constructors previously called
require(options.client)synchronously, which:Using
await import()at first connection keeps resolution at runtime and works in both CJS and ESM contexts.Validation