fix: avoid module-scoped DB clients in workers#964
fix: avoid module-scoped DB clients in workers#964Sigmabrogz wants to merge 2 commits intoAmanVarshney01:mainfrom
Conversation
Signed-off-by: Sigma <bot@example.com>
|
@rhoggs-bot-test-account is attempting to deploy a commit to the Better T Stack Team on Vercel. A member of the Team first needs to authorize it. |
|
Hi, the PR is ready for review. Thanks! |
WalkthroughModified SQLite Drizzle template to export a factory function Changes
Possibly related issues
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66adbe197c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
|
|
||
| export const db = drizzle({ client, schema }); | ||
| export const createDb = (env?: any) => drizzle({ client, schema }); |
There was a problem hiding this comment.
db export available for generated consumers
This change renames the package-level export from db to createDb, but the rest of the templates still import db directly (for example auth/better-auth/server/base/src/index.ts.hbs lines 81 and 153, and examples/todo/server/drizzle/base/src/routers/todo.ts.hbs lines 4 and 45). Any generated drizzle+sqlite project that includes those flows will fail type-check/build with “no exported member db” until every consumer is migrated in the same change.
Useful? React with 👍 / 👎.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/template-generator/templates/db/drizzle/sqlite/src/index.ts.hbs (1)
30-37:⚠️ Potential issue | 🟠 MajorWorkers/LibSQL branch does not fix the stale TCP connection issue.
The PR objective is to avoid module-scoped clients causing stale TCP connections in Workers. However, in this branch,
clientis still created at module scope (lines 30-35). ThecreateDbfunction merely wraps this same module-scoped client, so the underlying TCP connection is still shared across requests.To properly fix this, the
createClientcall should move insidecreateDb:🛠️ Proposed fix: Instantiate client per request
import { drizzle } from "drizzle-orm/libsql"; import { env } from "@{{projectName}}/env/server"; import { createClient } from "@libsql/client"; -const client = createClient({ - url: env.DATABASE_URL || "", -{{`#if` (eq dbSetup "turso")}} - authToken: env.DATABASE_AUTH_TOKEN, -{{/if}} -}); - -export const createDb = (env?: any) => drizzle({ client, schema }); +export const createDb = () => { + const client = createClient({ + url: env.DATABASE_URL || "", +{{`#if` (eq dbSetup "turso")}} + authToken: env.DATABASE_AUTH_TOKEN, +{{/if}} + }); + return drizzle({ client, schema }); +};
🧹 Nitpick comments (1)
packages/template-generator/templates/db/drizzle/sqlite/src/index.ts.hbs (1)
14-14: Unusedenvparameter in non-D1 branches creates a confusing API.In lines 14 and 37,
createDbacceptsenv?: anybut the parameter is never used—the function still relies on the module-scopedclient(which uses the importedenv). This inconsistency makes the API contract unclear.If the parameter isn't needed for these branches, consider removing it to avoid confusion. If you want a uniform signature across all branches, document why it's optional here.
♻️ Option: Remove unused parameter for clarity
-export const createDb = (env?: any) => drizzle({ client, schema }); +export const createDb = () => drizzle({ client, schema });Also applies to: 37-37
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea04c72e-8323-49e4-ab86-dd86e8eeac83
📒 Files selected for processing (1)
packages/template-generator/templates/db/drizzle/sqlite/src/index.ts.hbs
| }); | ||
|
|
||
| export const db = drizzle({ client, schema }); | ||
| export const createDb = (env?: any) => drizzle({ client, schema }); |
There was a problem hiding this comment.
Breaking change: Renaming db to createDb breaks downstream imports.
The context snippets show that other templates (todo.ts.hbs, better-auth/server/base/src/index.ts.hbs) import db as a named import:
import { db } from "@{{projectName}}/db";These imports will fail after this change. Either update all consuming templates to use createDb, or re-export a compatibility alias.
🔧 Option: Add backward-compatible re-export (all three branches)
-export const createDb = (env?: any) => drizzle({ client, schema });
+export const createDb = (env?: any) => drizzle({ client, schema });
+export const db = createDb();Or update all downstream templates to call createDb() where db was used.
#!/bin/bash
# Find all templates that import `db` from the db package
rg -n "import.*\bdb\b.*from.*@.*db" packages/template-generator/templates --glob '*.hbs'Also applies to: 24-24, 37-37
| @@ -21,7 +21,7 @@ import * as schema from "./schema"; | |||
| import { drizzle } from "drizzle-orm/d1"; | |||
| import { env } from "@{{projectName}}/env/server"; | |||
There was a problem hiding this comment.
Unused import: env is imported but shadowed by function parameter.
Line 22 imports env from the env package, but line 24 declares a function parameter also named env. The imported env is never used in the D1 branch.
🧹 Proposed fix: Remove unused import
{{`#if` (eq dbSetup "d1")}}
import { drizzle } from "drizzle-orm/d1";
-import { env } from "@{{projectName}}/env/server";
export const createDb = (env: any) => drizzle(env.DB, { schema });Also applies to: 24-24
2 participants
Addresses #935. This draft PR starts changing DB clients to be instantiated per request (via
createDb) in Cloudflare Workers templates to avoid stale TCP connections. Needs further updates in TRPC/Hono contexts.Summary by CodeRabbit