refactor(proxy): change database driver packages to optional peer dependency#2388
refactor(proxy): change database driver packages to optional peer dependency#2388jiashengguo wants to merge 1 commit intodevfrom
Conversation
|
Claude finished @jiashengguo's task —— View job PR Review AnalysisI've completed my analysis of the refactor to make database driver packages optional peer dependencies. Todo List:
Summary of Changespackage.json: Added Code Review Feedback✅ Strengths
|
📝 WalkthroughWalkthroughThe pull request refactors database driver dependencies in the CLI package from required runtime dependencies to optional peer dependencies, with corresponding updates to dynamically import these drivers at runtime using proper error handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/package.json`:
- Line 93: The file ends with a closing brace '}' but lacks a trailing newline;
update the packages/cli package.json by adding a single newline character after
the final '}' so the file ends with a POSIX-compliant trailing newline.
In `@packages/cli/src/actions/proxy.ts`:
- Around line 143-150: The catch blocks for dynamic imports (the SQLite import
assigning to SQLite: typeof BetterSqlite3 and the similar pg and mysql2 import
blocks) currently swallow all errors and always surface a "package missing"
CliError; change each catch to capture the error (e.g., catch (err)) and inspect
err.code (or err.message) — if it indicates module-not-found
(ERR_MODULE_NOT_FOUND / MODULE_NOT_FOUND) then throw the existing CliError
telling the user to install the package, otherwise rethrow the original error or
wrap it in a CliError that includes the original error message so native
addon/ABI failures are not misreported as "not installed". Ensure this change is
applied to the better-sqlite3, pg, and mysql2 dynamic import blocks.
| "node": ">=20" | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Missing trailing newline at end of file.
POSIX convention and most linter/formatter configs expect a trailing newline. This appears to have been removed.
Fix
Ensure the file ends with a newline character after the closing }.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/package.json` at line 93, The file ends with a closing brace '}'
but lacks a trailing newline; update the packages/cli package.json by adding a
single newline character after the final '}' so the file ends with a
POSIX-compliant trailing newline.
| let SQLite: typeof BetterSqlite3; | ||
| try { | ||
| SQLite = (await import('better-sqlite3')).default; | ||
| } catch { | ||
| throw new CliError( | ||
| `Package "better-sqlite3" is required for SQLite support. Please install it with: npm install better-sqlite3`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Catch-all hides real errors (e.g., native binary ABI mismatch).
The bare catch block assumes any import failure means the package is missing. For better-sqlite3 in particular, a common failure mode is a native addon compilation/ABI mismatch — the package is installed but fails to load. The current error message would mislead the user into re-installing a package that's already present.
The same pattern applies to the pg (lines 167-171) and mysql2 (lines 183-187) catch blocks, though native addon issues are less common there.
Proposed fix: inspect the error code before assuming "not installed"
try {
SQLite = (await import('better-sqlite3')).default;
- } catch {
- throw new CliError(
- `Package "better-sqlite3" is required for SQLite support. Please install it with: npm install better-sqlite3`,
- );
+ } catch (err: any) {
+ if (err?.code === 'ERR_MODULE_NOT_FOUND' || err?.code === 'MODULE_NOT_FOUND') {
+ throw new CliError(
+ `Package "better-sqlite3" is required for SQLite support. Please install it with: npm install better-sqlite3`,
+ );
+ }
+ throw new CliError(
+ `Failed to load "better-sqlite3": ${err instanceof Error ? err.message : String(err)}`,
+ );
}Apply the same pattern to the pg and mysql2 blocks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/actions/proxy.ts` around lines 143 - 150, The catch blocks
for dynamic imports (the SQLite import assigning to SQLite: typeof BetterSqlite3
and the similar pg and mysql2 import blocks) currently swallow all errors and
always surface a "package missing" CliError; change each catch to capture the
error (e.g., catch (err)) and inspect err.code (or err.message) — if it
indicates module-not-found (ERR_MODULE_NOT_FOUND / MODULE_NOT_FOUND) then throw
the existing CliError telling the user to install the package, otherwise rethrow
the original error or wrap it in a CliError that includes the original error
message so native addon/ABI failures are not misreported as "not installed".
Ensure this change is applied to the better-sqlite3, pg, and mysql2 dynamic
import blocks.
1 participant
Summary by CodeRabbit