Conversation
branch name: fix_dymanic_package_loading_in_neko_sql
There was a problem hiding this comment.
Pull request overview
This PR aims to make the SQL connection implementations compatible with optional/peer-installed database drivers by dynamically loading drivers at runtime instead of importing them statically.
Changes:
- Added a
loadPackage()helper tosrc/helper.mtsfor runtime driver resolution with a friendlier missing-driver error. - Updated MySQL and PostgreSQL connection implementations to use dynamically loaded driver modules (cached on static properties).
- Refactored the SQLite connection to avoid a static
better-sqlite3import and bumped the package version.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| neko-sql/src/helper.mts | Introduces loadPackage() to dynamically require() optional drivers. |
| neko-sql/src/databases/mysql/MysqlConnection.mts | Switches MySQL driver usage to a lazily loaded mysql2/promise module. |
| neko-sql/src/databases/postgresql/PostgresqlConnection.mts | Attempts to lazily load pg/pg-cursor, but still has a static runtime pg import. |
| neko-sql/src/databases/sqlite/SqliteConnection.mts | Replaces static better-sqlite3 import with runtime loading, but uses require directly in ESM. |
| neko-sql/tests/long_test.spec.ts | Minor refactor (let → const) in MySQL test config. |
| neko-sql/package.json | Version bump to 0.1.39. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,13 +1,14 @@ | |||
| import { connection_events, Connection as ConnectionAbs } from '../../Connection.mjs'; | |||
| import { Client, PoolClient, PoolConfig } from 'pg'; | |||
There was a problem hiding this comment.
pg is still imported as a runtime dependency (import { Client, PoolClient, PoolConfig } from 'pg'). This defeats the goal of making it an optional peer dependency and will still throw at module load time when pg isn't installed. Convert these imports to type-only and rely on loadPackage('pg') for runtime access (e.g., PostgresqlConnection.pg.Client, PostgresqlConnection.pg.Pool).
| import { Client, PoolClient, PoolConfig } from 'pg'; | |
| import type { Client, PoolClient, PoolConfig } from 'pg'; |
| const conn = new Client({ | ||
| ...PostgresqlConnection.pool.options, | ||
| database: 'postgres', | ||
| }); | ||
| await conn.connect(); |
There was a problem hiding this comment.
This block constructs a Client via the statically imported Client symbol. Once pg is truly dynamically loaded, this should be created from the dynamically loaded module (e.g., new PostgresqlConnection.pg.Client(...)) so the class works when pg is an optional peer dependency.
| if (!SqliteConnection.sqlite) { | ||
| SqliteConnection.sqlite = require('better-sqlite3'); | ||
| } |
There was a problem hiding this comment.
require('better-sqlite3') is used inside an .mts (ESM) module. In ESM output (dist/esm), require is not defined and this will throw before any helpful error can be surfaced. Use the same dynamic loader approach as the other connections (e.g., loadPackage('better-sqlite3') via createRequire) so SQLite remains an optional peer dependency in ESM too.
neko-sql/src/helper.mts
Outdated
| } catch { | ||
| throw new Error( | ||
| `${name} is not installed. Please follow documentation to install required packages.` | ||
| ); |
There was a problem hiding this comment.
loadPackage currently catches all errors from require(), which can mask real runtime issues inside the loaded package (syntax errors, initialization errors, etc.) and incorrectly report them as "not installed". Consider catching the error value, only translating MODULE_NOT_FOUND for the requested package into the friendly message, and rethrowing other errors (optionally preserving the original error as cause).
| } catch { | |
| throw new Error( | |
| `${name} is not installed. Please follow documentation to install required packages.` | |
| ); | |
| } catch (error: unknown) { | |
| const err = error as { code?: unknown; message?: unknown }; | |
| const isModuleNotFound = | |
| err && | |
| err.code === 'MODULE_NOT_FOUND' && | |
| typeof err.message === 'string' && | |
| err.message.includes(`'${name}'`); | |
| if (isModuleNotFound) { | |
| throw new Error( | |
| `${name} is not installed. Please follow documentation to install required packages.`, | |
| // Preserve original error when supported | |
| { cause: error as Error } | |
| ); | |
| } | |
| throw error; |
branch name: fix_dymanic_package_loading_in_neko_sql
This pull request introduces dynamic loading for database driver packages in the MySQL, PostgreSQL, and SQLite connection classes. Instead of importing these drivers statically, the code now loads them at runtime only when needed, improving flexibility and potentially reducing issues with optional dependencies. A new helper function,
loadPackage, was added to handle this dynamic loading and provide clear error messages if a required driver is missing.Dynamic driver loading and initialization:
loadPackagefunction inhelper.mtsto dynamically load database driver packages and provide informative errors if drivers are not installed.MysqlConnection,PostgresqlConnection, andSqliteConnectionclasses to useloadPackage(orrequirefor SQLite) to load their respective drivers (mysql2/promise,pg,pg-cursor,better-sqlite3) only when needed, storing the loaded module in a static property. [1] [2] [3] [4] [5] [6] [7] [8]Refactoring to use dynamically loaded drivers:
mysql.createPool,new Pool,new Database) with calls using the dynamically loaded driver stored in the static property, ensuring consistent use throughout each connection class. [1] [2] [3] [4] [5] [6] [7] [8]Minor improvements:
These changes make the codebase more robust and modular, allowing projects to include only the database drivers they require and providing clearer error handling when a required driver is missing.branch name: fix_dymanic_package_loading_in_neko_sql
Description
Checklist