feat: restructure @rune/database into core/drizzle/prisma sub-packages#9
Merged
Conversation
- Split @rune/database into @rune/database-core, @rune/database-drizzle, @rune/database-prisma sub-packages with individual package.json/build/tsconfig - Root umbrella package re-exports from sub-packages (backward compatible) - Added async lifecycle hooks (onInit/onDestroy) to RuneApp - Added DrizzleAdapter and PrismaAdapter classes with OnAppInit/OnAppDestroy - Added DatabaseModule.forRoot() factory - Converted blog-api example to Drizzle ORM (bun:sqlite) - Converted auth-service example to Prisma SQLite - Updated docs, CI, bundle-size tracking, and bundle-size script for nested packages - Updated workspace config to use explicit paths (avoids bun 1.3.14 nested glob issue) All 656 tests pass, 76/76 typecheck, 0 lint errors.
Merge branch 'main' into feat/database-orm-integration, resolving bun.lock conflict by regenerating (bun install).
Contributor
Bundle Sizes
📊 Size Changes
Legend: 🟢 size decreased ≥5% · 🟡 changed <5% · 🔴 increased ≥5% (threshold per baseline record) Generated by |
@rune/adapter-aws-lambda
@rune/adapter-bun
@rune/adapter-cloudflare-pages
@rune/adapter-cloudflare-workers
@rune/adapter-deno
@rune/adapter-elysia
@rune/adapter-express
@rune/adapter-fastify
@rune/adapter-h3
@rune/adapter-hono
@rune/adapter-koa
@rune/adapter-lambda-edge
@rune/adapter-netlify
@rune/adapter-node
@rune/adapter-service-worker
@rune/adapter-vercel
@rune/openapi
@rune/config
@rune/container
@rune/core
@rune/cache
@rune/database
@rune/graphql
@rune/logger
@rune/mail
@rune/middleware
@rune/queue
@rune/socket
@rune/telemetry
@rune/decorators
@rune/events
@rune/router
@rune/validation
@rune/create-rune
commit: |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * await app.destroy(); | ||
| * ``` | ||
| */ | ||
| async destroy(): Promise<void> { |
| "clean": "rm -rf dist" | ||
| }, | ||
| "dependencies": { | ||
| "@rune/database-core": "workspace:*" |
| "clean": "rm -rf dist" | ||
| }, | ||
| "dependencies": { | ||
| "@rune/database-core": "workspace:*" |
Contributor
⚡ Benchmark ResultsFramework BenchmarksRune
Hono
Elysia
Fastify
Node.js HTTP
Koa
Express
Combined Results
Rune Adapter BenchmarksRuntime AdaptersBun Adapter
Node Adapter
Framework AdaptersElysia Adapter
Hono Adapter
Express Adapter
Koa Adapter
Fastify Adapter
h3 Adapter
Combined Results
Updated at 2026-07-09T06:31:53.199Z by Benchmark CI |
…xamples, update docs - Remove 'Updated ...' console.log lines from benchmark scripts (CI noise) - Run adapter benchmarks to populate h3 adapter results in docs/benchmarks.md - Create try-database-drizzle and try-database-prisma example packages - Update README.md, docs/adapter-development.md for sub-package structure - Add new examples to root tsconfig.json references
Owner
Author
|
Now I have a comprehensive view of the changes. Here is my review: Code Review:
|
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
Missing await |
UserService.getUser() and getUserById() don't await the Prisma query — they return a raw Promise instead of the resolved value |
examples/applications/auth-service/index.ts:167-175, lines 181-188 |
Add await so callers get the resolved user object, not a Promise |
|
| ⚡ | Abstractions bypassed | resetState() in blog-api directly calls dbAdapter.client.delete(...) instead of going through PostService, breaking layering |
examples/applications/blog-api/index.ts:366-367 |
Reuse the service method or keep the module-level clean API, but be consistent about where data access logic lives |
| 💡 | Non-async DB call | resetState() calls Drizzle .run() (sync for bun:sqlite) without await, making it driver-specific behavior |
examples/applications/blog-api/index.ts:366 |
Either make resetState() async or document the sync assumption |
🔴 Potential Bugs & Edge Cases
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| 🔴 | JSDoc / impl mismatch | init() JSDoc says "Discovers lifecycle providers from the container" but no discovery logic exists — it only runs initHooks |
packages/foundation/core/src/rune-app.ts:100-101 |
Either implement container discovery or remove the misleading sentence from the JSDoc |
| Incomplete module pattern | DatabaseModule.forRoot() registers a provider under a private Symbol, but exports: [] means no consumer can inject it |
packages/infrastructure/database/core/src/database-module.ts:8-21 |
Either export the symbol (and make it public) or implement a proper injection token pattern that downstream controllers/services can reference | |
| ⚡ | Container bypass | Blog API's resetState() and Auth Service's getUser()/getUserById() skip the DI container for DB access, bypassing lifecycle scoping |
examples/applications/blog-api/index.ts:366, examples/applications/auth-service/index.ts:167,181 |
Use the container-resolved service instances consistently |
🏗️ Maintainability & Design
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| 💡 | Redundant coverage | try-database-drizzle and try-database-prisma examples duplicate tests already present in the package directories |
examples/infrastructure/try-database-drizzle/tests/, examples/infrastructure/try-database-prisma/tests/ |
These example tests add maintenance burden with no new coverage value; consider removing or keeping only the index.ts demo with a comment showing usage |
| 💡 | Disconnected lifecycle | OnAppInit/OnAppDestroy interfaces exist in database-core but there's no automatic wiring — users must manually call app.onInit(() => adapter.onAppInit()) |
packages/infrastructure/database/core/src/lifecycle.ts |
Consider adding a LifecycleModule or auto-discovery in RuneApp.init() that scans registered providers implementing OnAppInit/OnAppDestroy |
| 🏗️ | Recursive discovery in bundle-size.ts switches from flat category/pkg to arbitrary-depth traversal without sorting |
scripts/bundle-size.ts:95-120 |
Restore the .sort() on results to ensure deterministic output order |
✅ Good Parts
| Status | Category | Description | Location |
|---|---|---|---|
| ✅ | Clean interface design | DatabaseAdapter is a minimal 2-method interface (connect/disconnect) — straightforward and driver-agnostic |
packages/infrastructure/database/core/src/adapter.ts |
| ✅ | Loose coupling | DrizzleAdapter and PrismaAdapter accept generic type params and use type narrowing for driver methods (end/close) |
packages/infrastructure/database/drizzle/src/drizzle-adapter.ts:17-30 |
| ✅ | Good test coverage | Both new adapters have unit tests covering happy path, error propagation, lifecycle hooks, and edge cases (no-op end/close) | packages/infrastructure/database/drizzle/tests/, packages/infrastructure/database/prisma/tests/ |
| ✅ | Backward compatible umbrella | @rune/database re-exports all types/classes from sub-packages, preserving existing imports |
packages/infrastructure/database/src/index.ts |
| ✅ | Dead code removal | isSerializableObject() was unused — correct cleanup |
packages/foundation/core/src/json-serializer.ts:37-43 |
| ✅ | Test migration | All app.init() calls converted to await app.init() — consistent async |
packages/foundation/core/tests/rune-app.test.ts (27 occurrences) |
Final Verdict
await in auth-service's getUser()/getUserById() will produce confusing runtime behavior for unwary callers, and 2) the init() JSDoc claims container lifecycle discovery that doesn't exist yet. The DatabaseModule.forRoot() exports [] making it effectively unusable — either implement the wiring or document it as a future capability.
- git rm --cached .wrangler/ sqlite cache files (tracked, so .gitignore was ignored) - turbo.json: typecheck dependsOn ^build:dev so deps are built first - bundle-size.ts: remove unused getIndicator, unused threshold var - umbrella @rune/database: re-export DATABASE_MODULE_ADAPTER
Owner
Author
|
Here is my code review of this PR. Code Review:
|
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| ✅ | Clean layering | DatabaseAdapter is a minimal 2-method interface; OnAppInit/OnAppDestroy are separated cleanly |
database/core/src/adapter.ts:1-4, database/core/src/lifecycle.ts:1-7 |
— |
| ✅ | Backward compat | Umbrella @rune/database re-exports all sub-packages preserving existing imports |
database/src/index.ts:1-7 |
— |
| ✅ | Dead code removal | isSerializableObject() was correctly removed |
core/src/json-serializer.ts:37-43 |
— |
| 💡 | @rune/database-core exports types with export type but DatabaseModule/DATABASE_MODULE_ADAPTER are runtime values exported without type |
database/core/src/index.ts:1-4 |
DatabaseModule and DATABASE_MODULE_ADAPTER are runtime exports, so export {} (without type) is correct here — no change needed |
|
| 💡 | Duplicated example tests | try-database-drizzle/tests/ and try-database-prisma/tests/ are near-identical to package-level tests |
examples/infrastructure/try-database-* |
Low-value duplication; consider removing the test files and keeping only the index.ts demo |
⚠️ Potential Bugs & Edge Cases
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| Mixed sync/async DB calls | Blog API Drizzle methods use .run(), .get(), .all() without await — correct for bun:sqlite (sync), but will silently break if swapped with PostgreSQL (async) |
blog-api/index.ts:55,60,80,88,92,101,103,108 |
Either consistently await every call, or add a comment explaining the sync assumption |
|
| ⚡ | Promise.all on lifecycle hooks |
init() and destroy() run all hooks concurrently — if hookA depends on hookB completing first, they race |
rune-app.ts:111,125 |
Document in JSDoc that hooks run concurrently, or use sequential for loop if ordering matters |
| ⚡ | destroy() doesn't reset initialized |
After destroy(), calling fetch() skips re-init since initialized stays true |
rune-app.ts:108-112,124-126,139-142 |
If re-initialization is intended, reset this.initialized = false in destroy(); otherwise document as intentional |
| ⚡ | Auth-service uses raw SQL at import time alongside Prisma schema | Tables created via $executeRawUnsafe at module import; prisma/schema.prisma exists but isn't used for migrations — the two can diverge |
auth-service/index.ts:17-38, auth-service/prisma/schema.prisma:1-23 |
Either switch to Prisma migrations (prisma db push) or remove the .prisma schema file — having both is confusing |
| 🔴 | CI bun-version downgraded to 1.3.14 |
AGENTS.md specifies bun@1.4.0; previous CI used 1.4.0 |
.github/workflows/ci.yml:23 |
Revert to "1.4.0" to match the documented requirement |
🏗️ Maintainability & Design
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| 💡 | DatabaseModule.forRoot() is unused in examples |
The adapter is registered directly via container.register(), not through DatabaseModule |
database/core/src/database-module.ts:10-21 |
Either add an example using DatabaseModule.forRoot(), or remove the class if it's dead code |
| 💡 | Token generation uses Math.random() |
Not cryptographically secure; example code, but sets a poor pattern | auth-service/index.ts:84 |
Use crypto.randomUUID() instead |
| 🏗️ | Nested workspace entries in root package.json |
New entries for database/core, database/drizzle, database/prisma are explicit, but the glob packages/infrastructure/* covers the parent database package |
package.json:10-12 |
These explicit entries are needed because Bun workspaces don't recurse into subdirectories — correct and intentional |
✅ Testing & Reliability
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| ✅ | Good coverage | Both DrizzleAdapter and PrismaAdapter have thorough unit tests covering connect, disconnect, lifecycle hooks, edge cases (no-op end/close, error propagation) |
database/drizzle/tests/, database/prisma/tests/ |
— |
| ✅ | 656 tests pass | All existing tests migrated to await app.init() consistently |
rune-app.test.ts (27 occurrences) |
— |
| ✅ | Error propagation tested | PrismaAdapter tests verify $connect/$disconnect errors re-throw correctly |
database/prisma/tests/prisma-adapter.test.ts:29-48 |
— |
| 💡 | No test for RuneApp.destroy() |
onDestroy/destroy() has no dedicated test coverage |
rune-app.test.ts |
Add a test verifying destroy hooks execute and errors propagate correctly |
🛡️ Security
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| 💡 | Hardcoded auth token | Blog API uses "secret-token-123" as bearer token — acceptable for an example, but worth noting |
blog-api/index.ts:123 |
Consider documenting that this is example-only or loading from env |
Final Verdict
bun-version regression from 1.4.0 to 1.3.14 conflicts with AGENTS.md, and (2) the mix of sync Drizzle calls without await will silently misbehave if the DB driver is swapped to an async one. Optional: address the duplicate example tests and the unused DatabaseModule in examples.
…uth-service cleanup) - CI: revert bun-version 1.3.14 → 1.4.0 to match AGENTS.md - blog-api: await all Drizzle calls consistently (.get(), .all(), .run()) - rune-app: destroy() resets initialized flag; JSDoc notes concurrent hooks - rune-app.test.ts: add 3 destroy lifecycle tests - auth-service: replace raw SQL table creation with prisma db push in CI - auth-service: use crypto.randomUUID() instead of Math.random()
Owner
Author
Code Review:
|
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| 🔴 | Bug | AuthMiddleware.use() calls ctx.send() AND return new Response(...) — ctx.send() sets ctx.response and triggers sendResponse(), but the return causes the pipeline to use a different Response object, effectively discarding the ctx.send() write while the pipeline short-circuits on the returned Response |
blog-api/index.ts:131-135, 141-145 |
Pick one: use ctx.send() without return, or return new Response(...) without ctx.send(). Currently both paths execute, creating two response objects for one request |
| 🔴 | Config mismatch | package.json:72 declares "bun@1.3.14" but AGENTS.md and CI (ci.yml:23) use bun@1.4.0 — these will diverge over time |
package.json:72, ci.yml:23 |
Align packageManager to 1.4.0 to match CI and AGENTS.md, or update AGENTS.md if 1.3.14 is intentional |
| 🔴 | Lifecycle bypass | auth-service/index.ts:18 calls prisma.$connect() directly at import time, then also calls adapter.connect() in app.onInit(). The adapter wraps $connect() too, so the connection happens twice. The empty catch swallows connection errors silently |
auth-service/index.ts:17-21 |
Remove the direct $connect() call — let the adapter + app.onInit() manage the connection lifecycle entirely. If eager connection is desired, move into an async factory |
⚠️ Concerns
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| Code Quality | DrizzleAdapter dispatches driver methods via as Record<string, unknown> casting. A typo like ends instead of end silently becomes a no-op at runtime with no type error |
drizzle-adapter.ts:17,24 |
Define a narrow DriverClient interface with optional connect/end/close methods and use type narrowing instead of Record<string, unknown> |
|
| Maintainability | try-database-drizzle/tests/ and try-database-prisma/tests/ test identical mock classes with the same assertions as the package-level tests — zero net coverage gain, positive maintenance burden |
examples/infrastructure/try-database-*/tests/* |
Remove test files from example packages; keep only index.ts as a usage demonstration. Example code should not replicate unit tests |
|
| Edge Case | DrizzleAdapter.disconnect() checks typeof driver.end === "function" before typeof driver.close === "function". If a driver has both (uncommon), end is preferred — but this logic is undocumented |
drizzle-adapter.ts:26-30 |
Add a comment explaining the end > close precedence, or document which drivers use which method in the README |
💡 Observations
| Status | Category | Description | Location |
|---|---|---|---|
| 💡 | Design | DatabaseModule.forRoot() is defined but never used in any example — all examples register the adapter directly via container.register() |
database-module.ts:10-21 |
| 💡 | Design | DrizzleAdapter creates AnyDrizzleDB interface but still requires as any in tests because no real Drizzle type satisfies { $client: unknown } structurally |
drizzle-adapter.test.ts:15,24,30,38,46,58,65,72,79 |
| 💡 | Code Quality | createDb() in blog-api creates the table schema via raw SQL string (db.ts:9-19), duplicating the schema definition already in schema.ts — they can get out of sync |
blog-api/src/db/db.ts:9-19 |
| 💡 | Security | Auth service compares plaintext passwords (auth-service/index.ts:63) — acceptable for example code but worth noting |
auth-service/index.ts:63 |
✅ Good Parts
| Status | Category | Description | Location |
|---|---|---|---|
| ✅ | Design | DatabaseAdapter is a minimal 2-method interface — clean, driver-agnostic, easy to implement |
database-core/src/adapter.ts:1-4 |
| ✅ | Testing | Both adapters have thorough unit tests: connect, disconnect, no-ops, error propagation, lifecycle hooks | drizzle/tests/, prisma/tests/ |
| ✅ | Testing | RuneApp lifecycle tests cover idempotent init(), destroy(), destroy error propagation, and re-initialization after destroy |
rune-app.test.ts:180-214 |
| ✅ | Maintainability | Backward-compatible umbrella re-exports preserve all existing import paths | database/src/index.ts:1-7 |
| ✅ | Maintainability | discoverPackages() converted to recursive with .sort() — discovers nested sub-packages deterministically |
bundle-size.ts:90-115 |
| ✅ | Reliability | destroy() sets initialized = false, so fetch() correctly re-initializes after teardown |
rune-app.ts:127 |
📝 Test Coverage Gaps
| Status | Category | Description | Suggestion |
|---|---|---|---|
| 📝 | Coverage | No test verifies that DatabaseModule.forRoot() correctly registers the adapter in the container and makes it injectable |
Add a test mounting DatabaseModule.forRoot({ adapter }) and resolving the adapter via the container |
| 📝 | Coverage | No test for the case where DrizzleAdapter.connect() is called but $client is undefined |
Add a test with client {} (no $client property) |
Final Verdict
🔴 Changes requested — The package split is well-structured with clean interfaces, thorough adapter tests, and full backward compatibility. However, the AuthMiddleware.use() double-response bug in blog-api/index.ts (calling both ctx.send() AND return new Response()) will produce undefined behavior on the error path and must be fixed. The auth-service connecting via both raw prisma.$connect() at import time and adapter.connect() on init creates a double-connect lifecycle issue. Resolve these two bugs, align the packageManager version, and the rest is good to go.
- CI bun-version 1.4.0 returns 404; revert to 1.3.14 (matching packageManager) - AGENTS.md: update bun-version reference to 1.3.14 - @rune/database-drizzle, @rune/database-prisma: move @rune/database-core from dependencies to devDependencies (type-only imports)
| "clean": "rm -rf dist" | ||
| }, | ||
| "devDependencies": { | ||
| "@rune/database-core": "workspace:*", |
| "clean": "rm -rf dist" | ||
| }, | ||
| "devDependencies": { | ||
| "@rune/database-core": "workspace:*", |
Owner
Author
|
Now I have a thorough understanding of all changes. Here is my review: Code Review:
|
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| ✅ | Clean interface | DatabaseAdapter is a minimal 2-method interface — perfectly driver-agnostic |
database-core/src/adapter.ts:1-4 |
— |
| ✅ | Backward compat | Umbrella re-exports preserve all existing import paths via export * from sub-packages |
database/src/index.ts:1-7 |
— |
| ✅ | Dead code removal | isSerializableObject() correctly removed as unused |
core/src/json-serializer.ts:37-43 |
— |
| 💡 | Recursive discoverPackages |
Converts flat scan to recursive with .sort() — discovers nested sub-packages deterministically |
bundle-size.ts:90-115 |
.sort() on name is correct since sub-package names differ from parent |
| 🏗️ | Explicit workspace entries | New entries for database/core, database/drizzle, database/prisma appear in root package.json alongside the glob |
package.json:10-12 |
Required because Bun workspaces don't recurse — correct |
🔴 Major Issues
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| 🔴 | Bug | AuthMiddleware.use() calls ctx.send() AND return new Response(...). The return short-circuits the pipeline, making ctx.send() a dead write — but if the middleware is later refactored to not return, ctx.send() alone may not terminate properly |
blog-api/index.ts:131-135, 141-145 |
Remove ctx.send() calls and keep only return new Response(...), or vice versa |
| 🔴 | Bug | auth-service/index.ts:18 calls prisma.$connect() at import time AND dbAdapter.connect() (which wraps $connect()) runs again in app.onInit(). The top-level $connect() also has an empty catch that swallows connection failures silently |
auth-service/index.ts:17-21, 206-208 |
Remove the direct $connect() call — let the adapter + lifecycle manage the connection entirely |
| 🔴 | CI regression | CI bun-version uses "1.3.14" but AGENTS.md documents "1.4.0" — these conflict, and the PR appears to have changed from 1.4.0 to 1.3.14 |
.github/workflows/ci.yml:23 |
Align with AGENTS.md and use "1.4.0", or update AGENTS.md if the downgrade is intentional |
⚠️ Concerns
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| Silent failure | DrizzleAdapter casts $client to Record<string, unknown> — a typo like conncet instead of connect becomes a silent no-op at runtime with zero type errors |
drizzle-adapter.ts:17, 24, 26, 28 |
Define a narrow DriverClient interface with optional connect/end/close methods and use in operator or typeof type guards |
|
| Edge case | DrizzleAdapter.disconnect() prefers end() over close() — undocumented precedence. Drivers with both methods (rare) would only call end() |
drizzle-adapter.ts:26 |
Add a comment explaining the end > close precedence |
|
| Redundant coverage | try-database-drizzle/tests/ and try-database-prisma/tests/ test identical mock classes with same assertions as package-level tests |
examples/infrastructure/try-database-*/tests/*.test.ts |
Remove test files from examples; keep only index.ts as a usage demo |
|
| Silent no-op | If AnyDrizzleDB.$client is undefined, both connect() and disconnect() silently succeed — user thinks lifecycle is managed but nothing happens |
drizzle-adapter.ts:17-30 |
Consider throwing or logging a warning when $client is missing |
💡 Observations
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| 💡 | Schema duplication | DB schema defined twice: SQL in db.ts:9-19 and Drizzle schema in schema.ts:3-11 — they can silently diverge |
blog-api/src/db/db.ts:9-19, blog-api/src/db/schema.ts:3-11 |
Use Drizzle migrate or remove the raw SQL — let the Drizzle schema be the single source of truth |
| 💡 | Token auth | Auth service compares plaintext passwords (!==) and uses crypto.randomUUID() for tokens — acceptable for demo code |
auth-service/index.ts:63, 67 |
No change needed for an example |
| 💡 | Unused module | DatabaseModule.forRoot() is defined but never used in any example — all examples register adapters directly via container.register() |
database-core/src/database-module.ts:10-21 |
Either add an example using it, or document as future capability |
| 💡 | destroy() racing requests |
If a request is in-flight when destroy() is called, DB disconnect happens while handler may still be using the client |
rune-app.ts:125-128 |
Consider documenting that destroy() should only be called when no requests are in-flight |
| 💡 | Drizzle test type casting | All DrizzleAdapter tests use as any for mock objects because no real Drizzle type satisfies { $client: unknown } structurally |
drizzle/tests/drizzle-adapter.test.ts:6-86 |
Acceptable for test simplicity |
✅ Good Parts
| Status | Category | Description | Location |
|---|---|---|---|
| ✅ | Testing | Thorough adapter tests: connect, disconnect, no-ops, lifecycle hooks, error propagation | drizzle/tests/, prisma/tests/ |
| ✅ | Testing | RuneApp lifecycle tests cover idempotent init, destroy hooks, re-initialization after destroy, error propagation |
rune-app.test.ts:180-214 |
| ✅ | Maintainability | destroy() resets initialized = false, enabling fetch() to re-initialize after teardown |
rune-app.ts:127 |
| ✅ | Peer deps | drizzle-orm and @prisma/client correctly set as optional peer dependencies — no mandatory transitive deps |
drizzle/package.json:24-31, prisma/package.json:24-31 |
| ✅ | 656 tests pass | All existing tests migrated to await app.init() consistently |
rune-app.test.ts (27 occurrences) |
📝 Test Coverage Gaps
| Status | Category | Description | Suggestion |
|---|---|---|---|
| 📝 | Coverage | No test verifies that init() runs hooks concurrently — if hookA depends on hookB, they race silently |
Add a test with two interdependent hooks to verify concurrency behavior |
| 📝 | Coverage | No test for DrizzleAdapter when $client is undefined — the silent no-op path is untested |
Add a test with client {} (no $client) |
| 📝 | Coverage | No test for DatabaseModule.forRoot() mounting and resolving via the DI container |
Add integration test |
Final Verdict
AuthMiddleware double-write pattern in blog-api (ctx.send() + return new Response()), (2) the auth-service double-connect lifecycle (top-level prisma.$connect() then adapter.connect()), and (3) the CI bun-version regression to 1.3.14 conflicting with AGENTS.md's 1.4.0. The DrizzleAdapter's Record<string, unknown> casting is a robustness concern but acceptable for now.
…move dup tests, add coverage) - AuthMiddleware: remove ctx.send(), return Response only; fix wrapping middleware - DrizzleAdapter: replace Record<string, unknown> with DriverClient interface - DrizzleAdapter: add comment explaining end > close precedence - auth-service: eager () + CREATE TABLE IF NOT EXISTS; remove empty catch - tests: add DatabaseModule.forRoot container resolution test - tests: add DrizzleAdapter undefined coverage tests - chore: remove duplicate try-database-* test files
| "clean": "rm -rf dist" | ||
| }, | ||
| "devDependencies": { | ||
| "@rune/container": "workspace:*", |
Owner
Author
|
Here is my code review for this PR. Code Review:
|
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| ✅ | Interface design | DatabaseAdapter is a minimal 2-method interface — clean and driver-agnostic |
database/core/src/adapter.ts:1-4 |
— |
| ✅ | Type safety | DriverClient interface properly narrows driver methods (connect/end/close) with documented end > close precedence |
database/drizzle/src/drizzle-adapter.ts:3-7,32-33 |
— |
| ✅ | Backward compat | Umbrella re-exports preserve all existing import paths | database/src/index.ts:1-7 |
— |
| ✅ | Loose coupling | DrizzleAdapter/PrismaAdapter accept generic type params, no dependency on concrete driver types |
drizzle-adapter.ts:13, prisma-adapter.ts:8 |
— |
| ✅ | Dead code removal | isSerializableObject() correctly removed as unused |
core/src/json-serializer.ts:37-43 |
— |
| 💡 | Schema duplication | Blog API defines the same schema twice: raw SQL in db.ts:9-19 and Drizzle ORM in schema.ts:3-11 — two sources of truth that can silently diverge |
blog-api/src/db/db.ts:9-19, blog-api/src/db/schema.ts:3-11 |
Use Drizzle migrate exclusively; remove the raw CREATE TABLE from db.ts to keep the Drizzle schema as the single source of truth |
| 💡 | Recursive discovery | discoverPackages() now recurses into nested sub-packages with .sort() — correctly handles database/core, database/drizzle, etc. |
scripts/bundle-size.ts:90-115 |
Good, but could accidentally discover unintended package.json dirs; consider a whitelist or depth param |
🔴 Major Issues
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| 🔴 | Bug | init() sets this.initialized = true before running hooks. If Promise.all(initHooks) rejects, initialized remains true and the app is stuck in a broken "half-initialized" state — subsequent fetch() calls skip re-init silently |
rune-app.ts:108-111 |
Move this.initialized = true after the await Promise.all(...), or wrap in try/catch and reset to false on failure |
| 🔴 | Bug | Auth service calls prisma.$connect() at import time (top-level await) AND dbAdapter.connect() (which wraps $connect()) runs again in app.onInit() — connection happens twice on startup |
auth-service/index.ts:17, 221-223 |
Remove the direct prisma.$connect() call at line 17; let app.onInit() manage the full connection lifecycle. If eager connection is desired, move into an async factory |
| 🔴 | Design | DatabaseModule.forRoot() returns a module + providers + exports object, but ModuleLoader never processes module exports — the DATABASE_MODULE_ADAPTER token is never wired into the DI container. All examples bypass it via container.register() |
database/core/src/database-module.ts:10-21 |
Either implement export resolution in ModuleLoader, or remove exports from the returned config and document that adapters must be registered manually. If the module is dead code, remove it |
⚠️ Concerns
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| Edge Case | DrizzleAdapter.connect() is a silent no-op when $client is undefined or the driver has no connect method — caller believes connection succeeded |
drizzle-adapter.ts:22-26 |
Log a warning or throw when $client is missing or has no connect method at all |
|
| Maintainability | try-database-drizzle/tests/ and try-database-prisma/tests/ test identical mock classes with the same assertions as the package-level adapter tests — zero net coverage, positive maintenance burden |
examples/infrastructure/try-database-*/tests/*.test.ts |
Remove test files from example packages; keep only index.ts as a usage demonstration |
|
| Edge Case | No ordering guarantee for lifecycle hooks — Promise.all runs all hooks concurrently. If hookA calls adapter.connect() and hookB queries the DB, they race |
rune-app.ts:111,126 |
Document that hooks run concurrently in the JSDoc, or switch to sequential iteration if ordering is important | |
| Edge Case | PrismaAdapter uses $connect()/$disconnect() which are Prisma-specific — if PrismaClient's API changes, consumers get no type error because of PrismaAdapter<any> |
prisma-adapter.ts:8,18,22 |
Consider a stricter type bound: T extends { $connect(): Promise<void>; $disconnect(): Promise<void> } instead of just AnyPrismaClient |
💡 Observations
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| 💡 | Umbrella bundle size | @rune/database re-exports all 3 sub-packages, so import { DatabaseAdapter } from "@rune/database" loads all Drizzle + Prisma adapter code — adds ~600B+ to startup |
database/src/index.ts:1-7 |
Acceptable for an umbrella package; document it for users who care about minimal bundle |
| 💡 | Implicit any in examples |
PostService uses DrizzleAdapter<any> (line 42) and UserService uses PrismaAdapter<any> (line 53) — bypasses type safety |
blog-api/index.ts:42, auth-service/index.ts:53 |
Acceptable for example code, but real apps should specify concrete types |
📝 Test Coverage Gaps
| Status | Category | Description | Suggestion |
|---|---|---|---|
| 📝 | Coverage | No test verifies that init() cleans up initialized flag when hook rejects |
Add test: app.onInit(async () => { throw new Error() }), call app.init(), verify error, then verify fetch() still works |
| 📝 | Coverage | No test for DrizzleAdapter when connect() is called but driver.connect throws |
Add test verifying error propagation |
| 📝 | Coverage | destroy() without prior init() is untested |
Add test verifying graceful no-op or acceptable error |
✅ Good Parts
| Status | Category | Description | Location |
|---|---|---|---|
| ✅ | Testing | Thorough adapter tests: connect, disconnect, no-ops, lifecycle hooks, error propagation, end>close preference | drizzle/tests/drizzle-adapter.test.ts, prisma/tests/prisma-adapter.test.ts |
| ✅ | Testing | RuneApp lifecycle tests: idempotent init, destroy hooks, re-init after destroy, error propagation |
rune-app.test.ts:180-214 |
| ✅ | Maintainability | destroy() resets this.initialized = false, enabling fetch() to re-initialize after teardown |
rune-app.ts:127 |
| ✅ | Maintainability | Peer dependencies (drizzle-orm, @prisma/client) correctly set as optional — no mandatory transitive deps |
drizzle/package.json:24-31, prisma/package.json:24-31 |
| ✅ | Reliability | All 656 tests pass; 27 app.init() calls consistently use await |
rune-app.test.ts |
| ✅ | Maintainability | Deterministic package discovery via recursive traversal + .sort() ensures consistent bundle-size output across runs |
bundle-size.ts:90-115 |
Final Verdict
🔴 Changes requested — The package split is well-structured with clean interfaces, thorough adapter tests, and backward compatibility. However, two bugs must be fixed: (1) init() sets initialized = true before hooks complete, leaving the app in an unrecoverable half-initialized state if a hook rejects; (2) the auth-service double-connects via both top-level prisma.$connect() and app.onInit() → adapter.connect(). The DatabaseModule.forRoot() is effectively dead code since ModuleLoader never processes exports.
Owner
Author
Code Review:
|
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| ✅ | Interface | DatabaseAdapter is a clean 2-method interface — driver-agnostic, minimal |
core/src/adapter.ts:1-4 |
— |
| ✅ | Dead code | isSerializableObject() correctly removed |
core/src/json-serializer.ts:37-43 |
— |
| ✅ | Backward compat | Umbrella @rune/database re-exports all sub-packages preserving existing imports |
database/src/index.ts:1-7 |
— |
| ✅ | Lifecycle | init()/destroy() with concurrency via Promise.all is well-documented in JSDoc |
rune-app.ts:100-101,115 |
— |
| 💡 | Schema duplication | Blog API defines DB schema twice: raw SQL in db.ts:9-19 and Drizzle schema in schema.ts:3-11 — two sources of truth that can silently diverge |
blog-api/src/db/db.ts:9-19, blog-api/src/db/schema.ts:3-11 |
Use Drizzle migrate exclusively; remove raw CREATE TABLE from db.ts |
| 💡 | end > close precedence |
DrizzleAdapter.disconnect() prefers end() over close() — documented in comment at line 33 |
drizzle-adapter.ts:29-38 |
Consider exposing this in the README as some drivers only support one |
🔴 Major Issues
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| 🔴 | Bug | init() sets this.initialized = true at line 109 BEFORE running hooks at line 111. If Promise.all(initHooks) rejects, the app is left in an unrecoverable state where initialized === true but hooks never completed — fetch() silently skips re-init |
rune-app.ts:108-111 |
Move this.initialized = true after await Promise.all(...) completes, or wrap in try/catch to reset on failure |
| 🔴 | Bug | Auth service calls prisma.$connect() at import time (line 17), then dbAdapter.connect() (which wraps $connect()) runs again in app.onInit() at line 221-223 — double connection on startup. The empty catch at line 17 also silently swallows connection failures |
auth-service/index.ts:17-21,221-223 |
Remove the top-level prisma.$connect(). Let app.onInit() manage the full connection lifecycle via dbAdapter.connect() |
| 🔴 | Dead code | DatabaseModule.forRoot() returns exports: [DATABASE_MODULE_ADAPTER], but ModuleLoader never processes module exports. The exported DI token can never be resolved by downstream modules. No example uses DatabaseModule.forRoot() — all bypass it with container.register() |
database/core/src/database-module.ts:10-21 |
Either implement export resolution in ModuleLoader, or remove the exports config and document adapters must be registered manually. If dead code, remove the module entirely |
⚠️ Concerns
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| Edge Case | DrizzleAdapter.connect() is a silent no-op when $client is undefined or the driver has no connect method. Caller believes connection succeeded |
drizzle-adapter.ts:22-26 |
Log a warning or throw when $client is missing or has no connect method |
|
| Edge Case | Blog API Drizzle calls (.run(), .get(), .all()) without await — correct for sync bun:sqlite, but silently breaks if swapped with async PostgreSQL driver |
blog-api/index.ts:55,60,80,88,92,101,103,108 |
Consistently await every call, or add a top-level comment explaining the sync-only assumption |
|
| Maintainability | try-database-drizzle/tests/ and try-database-prisma/tests/ are near-identical copies of the package-level adapter tests — same mock pattern, same assertions, zero net coverage |
examples/infrastructure/try-database-*/tests/*.test.ts |
Remove test files from example packages; keep only index.ts as a usage demo |
|
| Edge Case | discoverPackages() has no max-depth guard. While SKIP_DIRS prevents infinite loops, deeply nested structures with accidental package.json would be discovered |
scripts/bundle-size.ts:90-115 |
Consider adding a maxDepth parameter or a whitelist of known package categories |
|
| Robustness | DrizzleAdapter casts $client as DriverClient via unknown. A typo like conncet becomes a silent no-op at runtime with zero type errors |
drizzle-adapter.ts:23,30 |
Consider using in operator guards instead of casting |
💡 Observations
| Status | Category | Description | Location |
|---|---|---|---|
| 💡 | Coverage | No test verifies that init() handles hook rejection correctly (no leftover initialized = true state) |
rune-app.test.ts |
| 💡 | Coverage | No test for destroy() without prior init() |
rune-app.test.ts |
| 💡 | Coverage | No test for DrizzleAdapter.connect() when driver.connect throws |
drizzle/tests/drizzle-adapter.test.ts |
| 💡 | Security | Blog API hardcodes "secret-token-123" as bearer token — acceptable for example code |
blog-api/index.ts:131 |
| 💡 | Security | Auth service compares plaintext passwords (!==) — acceptable for example |
auth-service/index.ts:78 |
| 💡 | Bundling | Umbrella @rune/database re-exports all 3 sub-packages, so import { DatabaseAdapter } from "@rune/database" loads Drizzle and Prisma adapter code unnecessarily |
database/src/index.ts:1-7 |
| 💡 | Fallow warnings | @rune/database-core in devDependencies flagged as "never imported" — appears to be a false positive from fallow scanning compiled output vs source |
drizzle/package.json:21, prisma/package.json:21 |
| 💡 | CI version | CI uses bun@1.3.14 (line 23), packageManager is bun@1.3.14 (line 72) — consistent with each other |
ci.yml:23, package.json:72 |
✅ Good Parts
| Status | Category | Description | Location |
|---|---|---|---|
| ✅ | Testing | Thorough adapter tests: connect, disconnect, no-ops, lifecycle hooks, error propagation, end > close preference |
drizzle/tests/drizzle-adapter.test.ts:1-99, prisma/tests/prisma-adapter.test.ts:1-72 |
| ✅ | Testing | RuneApp lifecycle tests: idempotent init, destroy hooks, re-init after destroy, destroy error propagation |
rune-app.test.ts:180-214 |
| ✅ | Testing | 656 tests pass; 27 app.init() calls consistently use await |
rune-app.test.ts |
| ✅ | Maintainability | destroy() resets this.initialized = false, enabling fetch() to re-initialize after teardown |
rune-app.ts:127 |
| ✅ | Maintainability | Peer dependencies (drizzle-orm, @prisma/client) set as optional — no mandatory transitive deps |
drizzle/package.json:24-31, prisma/package.json:24-31 |
| ✅ | Maintainability | Deterministic package discovery via recursive traversal + .sort() ensures consistent bundle-size output |
bundle-size.ts:90-115 |
| ✅ | Maintainability | DriverClient interface properly narrows driver methods (connect/end/close) with documented precedence |
drizzle-adapter.ts:3-7 |
| ✅ | Maintainability | AnyPrismaClient interface correctly bounds via $connect()/$disconnect() |
prisma-adapter.ts:3-6 |
Final Verdict
🔴 Changes requested — The package split is well-structured with clean interfaces, thorough adapter tests, full backward compatibility, and proper peer dependency setup. However, two bugs must be fixed before merging: (1) init() sets initialized = true before hooks complete, corrupting state if a hook rejects; (2) auth-service double-connects via both top-level prisma.$connect() and app.onInit() → adapter.connect(). Additionally, DatabaseModule.forRoot() is effectively dead code since ModuleLoader never processes exports.
… rm dead DatabaseModule.forRoot - rune-app.init(): move this.initialized=true after hooks succeed - auth-service: remove onInit hook (already connected eagerly at import) - database-module.ts: remove dead DatabaseModule.forRoot() and DatabaseModuleConfig (ModuleLoader never processes exports) - update all imports/exports in core, umbrella, and tests
Owner
Author
|
Now I have a thorough understanding of all the changes. Here is my review: Review🔴 Major Issues
|
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| Design | Auth-service calls prisma.$connect() at import time (line 17) instead of through dbAdapter.connect()/app.onInit(), bypassing the adapter's lifecycle abstraction. dbAdapter.disconnect() is called on destroy but $connect() was never routed through the adapter |
auth-service/index.ts:17, 221-223 |
Replace the top-level prisma.$connect() with app.onInit(async () => { await dbAdapter.connect(); }) and move the CREATE TABLE bootstrapping into an onInit hook as well |
|
| Maintainability | try-database-drizzle/tests/ and try-database-prisma/tests/ test the same mock patterns with the same assertions as the package-level adapter tests — zero net coverage, positive maintenance burden |
examples/infrastructure/try-database-*/tests/*.test.ts |
Remove test files from example packages; keep only index.ts as a usage demo |
|
| Robustness | DrizzleAdapter casts $client as DriverClient — a missing $client or driver method typo (conncet instead of connect) becomes a silent no-op at runtime |
drizzle-adapter.ts:23,30 |
Add a console.warn when $client is absent or the expected method is missing |
💡 Observations
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| 💡 | Maintainability | Blog API defines DB schema twice: raw CREATE TABLE SQL in db.ts:9-19 and Drizzle schema in schema.ts:3-11 — two sources of truth that can silently diverge |
blog-api/src/db/db.ts:9-19, blog-api/src/db/schema.ts:3-11 |
Use Drizzle migrate exclusively; remove the raw SQL from db.ts |
| 💡 | Design | Auth-service onDestroy registers dbAdapter.disconnect() but there's no corresponding onInit for dbAdapter.connect() — unbalanced lifecycle |
auth-service/index.ts:221-223 |
Either add app.onInit(async () => { await dbAdapter.connect(); }) or remove the destroy hook |
| 💡 | Design | re-throws $connect errors test verifies Prisma errors propagate but DrizzleAdapter has no equivalent test for connect errors |
drizzle/tests/drizzle-adapter.test.ts |
Add a test for driver.connect() throwing |
| 💡 | Design | init() has no error path test — if a hook rejects, initialized stays false and fetch() retries init() which may re-run partially completed hooks |
rune-app.test.ts |
Add a test verifying behavior when onInit hooks throw |
✅ Good Parts
| Status | Category | Description | Location |
|---|---|---|---|
| ✅ | Design | DatabaseAdapter is a clean 2-method interface — minimal, driver-agnostic |
database/core/src/adapter.ts:1-4 |
| ✅ | Type safety | PrismaAdapter uses AnyPrismaClient with strict $connect/$disconnect bounds; DrizzleAdapter uses DriverClient with narrow optional methods |
prisma-adapter.ts:3-6, drizzle-adapter.ts:3-7 |
| ✅ | Testing | Thorough adapter tests: connect, disconnect, no-ops, lifecycle hooks, error propagation, end > close precedence |
drizzle/tests/drizzle-adapter.test.ts:1-99, prisma/tests/prisma-adapter.test.ts:1-72 |
| ✅ | Testing | RuneApp lifecycle tests cover idempotent init, destroy hooks, re-init after destroy, destroy error propagation |
rune-app.test.ts:180-214 |
| ✅ | Backward compat | Umbrella @rune/database re-exports all sub-packages preserving existing import paths |
database/src/index.ts:1-6 |
| ✅ | Dead code | isSerializableObject() correctly removed as unused |
core/src/json-serializer.ts:37-43 |
| ✅ | Peer deps | drizzle-orm and @prisma/client correctly set as optional peer dependencies |
drizzle/package.json:24-31, prisma/package.json:24-31 |
| ✅ | Maintainability | destroy() resets this.initialized = false, enabling re-initialization |
rune-app.ts:127 |
| ✅ | CI consistency | bun-version 1.3.14 is consistent across ci.yml, package.json, and the AGENTS.md update |
ci.yml:23, package.json:72, AGENTS.md |
Final Verdict
🔴 Changes requested — The package split is well-structured with clean interfaces, thorough adapter tests, and full backward compatibility. However, two issues must be fixed: (1) createPost in blog-api has a cache-before-create bug that silently turns POST into a no-op when cached data exists; (2) database-module.ts is a stub with a dangling Symbol export but no actual DatabaseModule.forRoot() implementation as described in the PR. Additionally, the auth-service bypasses the adapter's lifecycle abstraction by calling prisma.$connect() directly at import time instead of routing through app.onInit().
…uth through adapter lifecycle - blog-api createPost: remove cache-before-create; POST always creates - blog-api test: update cached-create test to expect 201 (not 200) - database-module.ts: restore DatabaseModule.forRoot() with JSDoc - core/umbrella index.ts: re-export DatabaseModule, DatabaseModuleConfig - auth-service: move table bootstrap into app.onInit() after adapter.connect(); remove direct prisma.() - adapter.test.ts: restore DatabaseModule.forRoot shape test
Owner
Author
|
Now I have a thorough understanding of all changes. Here is my review: Code Review:
|
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| ✅ | Design | DatabaseAdapter is a minimal 2-method interface — clean, driver-agnostic, easy to implement |
database/core/src/adapter.ts:1-4 |
— |
| ✅ | Design | DriverClient properly narrows driver methods with optional connect/end/close and documented end > close precedence |
drizzle-adapter.ts:3-7,29-38 |
— |
| ✅ | Design | AnyPrismaClient uses strict $connect/$disconnect bounds — type-safe |
prisma-adapter.ts:3-6 |
— |
| ✅ | Testing | Thorough adapter tests: connect, disconnect, no-ops, lifecycle hooks, error propagation, end > close preference |
drizzle/tests/, prisma/tests/ |
— |
| ✅ | Testing | RuneApp lifecycle tests: idempotent init, destroy hooks, re-init after destroy, destroy error propagation |
rune-app.test.ts:180-214 |
— |
| ✅ | Maintainability | Backward-compatible umbrella @rune/database re-exports all sub-packages preserving existing import paths |
database/src/index.ts:1-7 |
— |
| ✅ | Maintainability | Deterministic package discovery via recursive traversal + .sort() |
bundle-size.ts:90-115 |
— |
| ✅ | Maintainability | destroy() correctly resets initialized = false, enabling fetch() re-initialization |
rune-app.ts:127 |
— |
| ✅ | Dead code | isSerializableObject() correctly removed as unused |
core/src/json-serializer.ts:37-43 |
— |
| ✅ | Peer deps | drizzle-orm, @prisma/client set as optional peer dependencies — no mandatory transitive deps |
drizzle/package.json:24-31, prisma/package.json:24-31 |
— |
| ✅ | CI | Prisma generate + db push steps added for auth-service example | ci.yml:37-41 |
— |
| 💡 | Schema duplication | Blog API defines schema twice: raw CREATE TABLE SQL in db.ts:9-19 and Drizzle sqliteTable in schema.ts:3-11 — can silently diverge |
blog-api/src/db/db.ts:9-19, blog-api/src/db/schema.ts:3-11 |
Use Drizzle migration exclusively; remove raw SQL from db.ts |
| 💡 | Redundant example tests | try-database-drizzle/tests/ and try-database-prisma/tests/ test identical mock classes with same assertions as package-level tests — zero net coverage, positive maintenance burden |
examples/infrastructure/try-database-*/tests/*.test.ts |
Remove test files from example packages; keep only index.ts as a usage demo |
| 💡 | DrizzleAdapter<any> in examples |
Blog PostService and Auth UserService use DrizzleAdapter<any> / PrismaAdapter<any> — bypasses type safety |
blog-api/index.ts:42, auth-service/index.ts:31 |
Acceptable for example code; real apps should bind concrete types |
| Robustness | DrizzleAdapter.connect() is silent no-op when $client is undefined — caller believes connection succeeded |
drizzle-adapter.ts:22-26 |
Log a warning when $client is missing or has no connect method |
|
| Robustness | DrizzleAdapter casts $client as DriverClient — a typo like conncet becomes a silent runtime no-op with zero type errors |
drizzle-adapter.ts:23,30 |
Consider using in operator guards for safe property checks |
|
| Edge Case | init() seals the pipeline BEFORE hooks complete at line 109. If hooks throw, pipeline.seal() is non-reversible but initialized stays false — auto-init on next fetch() re-runs hooks and calls seal() again (should be idempotent but unclear from API) |
rune-app.ts:108-111 |
Either move pipeline.seal() after await Promise.all(...), or document that seal() is idempotent |
|
| Edge Case | discoverPackages() has no max-depth guard. Deeply nested accidental package.json dirs beyond SKIP_DIRS get discovered |
scripts/bundle-size.ts:90-115 |
Add maxDepth param or category whitelist |
|
| 📝 | Coverage | No test verifies DrizzleAdapter behavior when driver.connect() throws |
drizzle/tests/drizzle-adapter.test.ts |
Add test: mock driver with connect() { throw ... }, verify error propagates |
| 📝 | Coverage | No test for DatabaseModule.forRoot() integration with DI container (mounting + resolving) |
database/core/tests/adapter.test.ts |
Add integration test verifying provider is resolvable via Container.resolve() |
| 📝 | Coverage | No test for onInit hook rejection — verify initialized flag state is correct after failure |
rune-app.test.ts |
Add test: register throwing hook, call init(), verify error, then verify fetch() still works |
Final Verdict
✅🎯 LGTM — The package split is well-structured with clean interfaces, thorough adapter tests, full backward compatibility, proper peer dependency setup, and correct lifecycle management. The minor observations (schema duplication, example test redundancy, DrizzleAdapter silent no-ops) are non-blocking and can be addressed in follow-ups. No bugs or regressions introduced.
Owner
Author
|
Now I have a complete picture. Let me compile the review. Code Review:
|
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| 🔴 | Bug | init() seals the pipeline at L109 BEFORE hooks complete at L110. If a hook rejects, pipeline is permanently sealed but initialized stays false. fetch() retries init(), hooks re-run (potentially harmful if partially completed), and seal() must be idempotent to not crash |
rune-app.ts:108-110 |
Move pipeline.seal() after await Promise.all(...), or wrap in try/catch and unseal on failure. Add a test verifying behavior when onInit throws |
| 🔴 | Design | OnAppInit/OnAppDestroy interfaces in lifecycle.ts are implemented by both adapters but never auto-discovered or wired by RuneApp.init()/destroy(). Users must manually call app.onInit(() => adapter.onAppInit()) — the interfaces provide zero integration value as-is |
database/core/src/lifecycle.ts:1-7, rune-app.ts:107-128 |
Either (a) implement auto-discovery in init() that scans registered providers implementing OnAppInit/OnAppDestroy, or (b) document that these are manual-only and remove the expectation of automatic wiring |
| 🔴 | Dead Code | DatabaseModule.forRoot() returns exports: [DATABASE_MODULE_ADAPTER] but ModuleLoader never processes module exports. The token can never be dependency-injected. No example uses DatabaseModule — all bypass it with container.register() |
database/core/src/database-module.ts:22-30 |
Either implement export resolution in ModuleLoader, or if the module is dead code, remove it entirely to avoid misleading consumers |
⚠️ Concerns
| Status | Category | Description | Location | Suggestion |
|---|---|---|---|---|
| Performance | discoverPackages() calls results.sort() inside each recursive frame — O(n log n * depth) instead of O(n log n) once |
bundle-size.ts:113 |
Hoist .sort() to the top-level call after the recursive exploration completes |
|
| Maintainability | Blog-api wraps AuthMiddleware.use in an extra closure: (ctx, next) => authMiddleware.use(ctx, next) — the method already returns Promise<Response|void> which is the exact middleware signature |
blog-api/index.ts:344-346 |
Register directly: app.use(authMiddleware.use.bind(authMiddleware)), or make AuthMiddleware implement the Middleware type |
|
| Robustness | DrizzleAdapter casts $client from unknown to DriverClient — a misspelled method like endd becomes a silent no-op at runtime with zero type errors |
drizzle-adapter.ts:23,30 |
Use in operator guards ("connect" in driver) or hasOwnProperty to avoid silent failures |
|
| Edge Case | destroy() has no idempotency guard — unlike init(), calling destroy() multiple times runs all hooks multiple times |
rune-app.ts:125-128 |
Add if (!this.initialized) return; guard at the start of destroy() |
💡 Observations
| Status | Category | Description | Location |
|---|---|---|---|
| 💡 | Testing | All 12 mock DrizzleAdapter test objects use as any — while functional, a helper factory returning AnyDrizzleDB would improve readability |
drizzle/tests/drizzle-adapter.test.ts:6,16,24,32,41,51,60,66,72,78,86,95 |
| 💡 | Schema | Auth-service defines the same schema in both raw SQL (index.ts:201-218) and Prisma schema (prisma/schema.prisma) — two sources of truth |
auth-service/index.ts:201-218, auth-service/prisma/schema.prisma |
| 💡 | Lifecycle | Blog-api registers dbAdapter.connect()/disconnect() in app.onInit/onDestroy and DrizzleAdapter implements OnAppInit/OnAppDestroy — the adapter is ready for auto-wiring but must be manually connected |
blog-api/index.ts:335-341 |
| 💡 | Inconsistency | Blog-api AuthMiddleware uses return new Response(...) pattern while auth-service middleware uses ctx.send(...) — diverging patterns in the same PR |
blog-api/index.ts:139, auth-service/index.ts:228 |
| 💡 | CI consistency | bun-version is 1.3.14 in CI, package.json, and AGENTS.md update — all three are aligned, contrary to earlier review assertions |
.github/workflows/ci.yml:23, package.json:72 |
✅ Good Parts
| Status | Category | Description | Location |
|---|---|---|---|
| ✅ | Design | DatabaseAdapter is a minimal 2-method interface — clean, driver-agnostic, easy to implement |
database/core/src/adapter.ts:1-4 |
| ✅ | Type safety | AnyPrismaClient uses strict $connect/$disconnect bounds — no any leakage |
prisma-adapter.ts:3-6 |
| ✅ | Testing | Thorough adapter tests: connect, disconnect, no-ops, lifecycle hooks, error propagation, end>close preference | drizzle/tests/drizzle-adapter.test.ts, prisma/tests/prisma-adapter.test.ts |
| ✅ | Testing | RuneApp lifecycle tests cover idempotent init, destroy hooks, re-init after destroy, error propagation |
rune-app.test.ts:180-214 |
| ✅ | Backward compat | Umbrella @rune/database re-exports all sub-packages preserving all existing import paths |
database/src/index.ts:1-7 |
| ✅ | Dead code | isSerializableObject() correctly removed |
core/src/json-serializer.ts:37-43 |
| ✅ | Peer deps | drizzle-orm, @prisma/client set as optional peer dependencies — no mandatory transitive deps |
drizzle/package.json:24-31, prisma/package.json:24-31 |
| ✅ | Maintainability | destroy() correctly resets initialized = false, enabling fetch() to re-initialize |
rune-app.ts:127 |
| ✅ | Documentation | end() > close() precedence clearly documented in comment at drizzle-adapter.ts:32-33 |
drizzle-adapter.ts:32-33 |
📝 Test Coverage Gaps
| Status | Category | Description | Suggestion |
|---|---|---|---|
| 📝 | Coverage | No test for init() hook rejection — verify initialized flag and pipeline state after partial failure |
Add test: register a hook that throws, call init(), verify error propagates, verify fetch() behavior |
| 📝 | Coverage | No test for destroy() idempotency — calling destroy twice should not double-execute hooks |
Add test calling destroy() twice and mock-hook call count = 1 |
| 📝 | Coverage | No test for DrizzleAdapter when driver.connect() throws |
Add test similar to PrismaAdapter's error propagation test |
Final Verdict
🔴 Changes requested — The package split is well-structured with clean interfaces, thorough adapter tests, and full backward compatibility. Three issues need attention: (1) init() seals the pipeline before hooks complete, leaving the app in a half-sealed state if hooks reject; (2) OnAppInit/OnAppDestroy interfaces provide no actual integration — they're implemented but never auto-wired; (3) DatabaseModule.forRoot() is effectively dead code since ModuleLoader ignores exports. Address these and it's good to merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Split
@rune/databaseinto three sub-packages with an umbrella re-export package.Changes
@rune/database-core—DatabaseAdapterinterface,OnAppInit/OnAppDestroylifecycle hooks,DatabaseModulefactory@rune/database-drizzle—DrizzleAdapterwrapping any Drizzle ORM instance with lifecycle management@rune/database-prisma—PrismaAdapterwrapping any PrismaClient with lifecycle management@rune/database(umbrella) — re-exports all three sub-packages for backward compatibilityRuneApp— added asyncinit()/destroy()lifecycle withonInit()/onDestroy()hook registrationExamples converted
Backward Compatibility
Verification
bun install/bun updatesucceed