ENG-815 DbDotenv.d.ts #407
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughStandardizes consumers to use CommonJS require for @repo/database/dbDotEnv, updates @repo/database export map to dist-built artifacts, adds explicit TypeScript types in dbDotEnv source, narrows tsconfig includes, introduces a lint tsconfig for declaration emit, adjusts internal scripts to import dbDotEnv locally, and removes legacy types file. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/database/src/dbDotEnv.mts (1)
13-19: Potential infinite loop if “database” segment isn’t found (bundled paths, atypical installs). Throw instead of looping to root.When the file is bundled (e.g., .next/server/chunks), the path may never include “database”, causing dirname(dir) to stabilize at root and loop forever.
Apply:
-const findRoot = () => { - let dir = fileURLToPath(import.meta.url); - while (basename(dir) !== "database") { - dir = dirname(dir); - } - return dir; -}; +const findRoot = () => { + let dir = dirname(fileURLToPath(import.meta.url)); + // Walk up until we hit the package root folder named "database". + while (basename(dir) !== "database") { + const parent = dirname(dir); + if (parent === dir) { + throw new Error( + `dbDotEnv: could not locate "database" package root from ${fileURLToPath( + import.meta.url, + )}`, + ); + } + dir = parent; + } + return dir; +};packages/database/package.json (1)
33-35: StaletypesVersionsentry for./dbDotEnvThis still targets the removed
src/dbDotEnv.d.ts. It will break type resolution for editors/older TS versions.Apply this diff:
"./dbDotEnv": [ - "src/dbDotEnv.d.ts" + "dist/src/dbDotEnv.d.mts" ],
🧹 Nitpick comments (8)
packages/database/tsconfig.json (1)
3-9: AlignrootDirwith package exports
- Change
"rootDir"in packages/database/tsconfig.json from"."to"src".- Update the
exports["./dbDotEnv"]paths in packages/database/package.json from./dist/src/dbDotEnv.*→./dist/dbDotEnv.*to match the new output location.- No changes needed for tsconfig_lint.json.
packages/database/src/dbDotEnv.mts (2)
21-36: Tighten typing for variant and simplify validation via Set to drop the cast.Avoid casting by tracking variant as Variant | undefined and validating with a Set.
Apply:
-export const getVariant = (): Variant => { +export const getVariant = (): Variant => { const processHasVars = !!process.env["SUPABASE_URL"] && !!process.env["SUPABASE_ANON_KEY"]; const useDbArgPos = (process.argv || []).indexOf("--use-db"); - let variant = - useDbArgPos > 0 - ? process.argv[useDbArgPos + 1] - : process.env["SUPABASE_USE_DB"]; + const allowed = new Set<Variant | undefined>([ + "local", + "branch", + "production", + "none", + "implicit", + undefined, + ]); + let variant: Variant | undefined = + useDbArgPos > 0 + ? (process.argv[useDbArgPos + 1] as Variant | undefined) + : (process.env["SUPABASE_USE_DB"] as Variant | undefined); - if ( - ["local", "branch", "production", "none", "implicit", undefined].indexOf( - variant, - ) === -1 - ) { + if (!allowed.has(variant)) { throw new Error("Invalid variant: " + variant); } @@ // avoid repeating warnings process.env["SUPABASE_USE_DB"] = variant; - return variant as Variant; + return variant; };Also applies to: 38-61
63-83: Minor: clarify return shape and keys; consider narrowing to known keys for stronger typing.If you want stricter typing at call sites, you can expose a dedicated Env map type and return that, while still allowing partials.
Example (no change required now):
export type SupabaseEnv = Partial<{ SUPABASE_URL: string; SUPABASE_ANON_KEY: string; NEXT_API_ROOT: string; }>; export const envContents = (): SupabaseEnv => { /* ... */ };packages/database/scripts/dev.ts (1)
3-3: Same resolution concern as build.ts: confirm ESM TS loader availability.If environments vary, consider lazy dynamic import with a helpful error or a small shim that resolves either local src or the published package.
Example shim (optional):
// dbDotEnvShim.ts export const getVariant = async () => (await import("../src/dbDotEnv.mts")).getVariant;apps/website/app/utils/supabase/server.ts (1)
4-4: Preserve types when using CommonJS require by annotating the shape.Current require erases types; add a minimal annotation to keep type-checks downstream.
Apply:
-const { envContents } = require("@repo/database/dbDotEnv"); +const { envContents } = require("@repo/database/dbDotEnv") as { + envContents: () => Partial<Record<string, string>>; +};packages/database/tsconfig_lint.json (1)
1-19: Ensure declaration emit actually happens; consider clarifying config intent.If base config doesn’t set
"declaration": true,"emitDeclarationOnly": truewon’t emit anything. Also, if this file is used for type-checking only,"noEmit": truemight be preferable; if it’s for .d.ts generation, consider renaming totsconfig.types.jsonfor clarity.If needed, add:
"compilerOptions": { "baseUrl": ".", "rootDir": ".", "outDir": "dist", "module": "esnext", "moduleResolution": "bundler", "allowImportingTsExtensions": true, + "declaration": true, "emitDeclarationOnly": true },apps/roam/scripts/compile.ts (1)
21-21: LGTM on require; add a guard to avoid embedding "undefined" strings into build.Validate presence of required envs before esbuild.define, and prefer JSON.stringify for safety.
Add after
const dbEnv = envContents();:if (!dbEnv.SUPABASE_URL || !dbEnv.SUPABASE_ANON_KEY) { throw new Error("Missing SUPABASE_URL or SUPABASE_ANON_KEY for compile"); }And adjust define:
define: { "process.env.SUPABASE_URL": JSON.stringify(dbEnv.SUPABASE_URL), "process.env.SUPABASE_ANON_KEY": JSON.stringify(dbEnv.SUPABASE_ANON_KEY), "process.env.NEXT_API_ROOT": JSON.stringify(dbEnv.NEXT_API_ROOT || ""), "window.__DISCOURSE_GRAPH_VERSION__": JSON.stringify(getVersion()), "window.__DISCOURSE_GRAPH_BUILD_DATE__": JSON.stringify(getBuildDate()), },packages/database/features/step-definitions/stepdefs.ts (1)
5-5: Keep type safety when switching to CommonJS require
require()erases types in TS. Cast the module to its declared type sogetVariant/configremain typed.Apply this diff:
-const { getVariant, config } = require("@repo/database/dbDotEnv"); +const { getVariant, config } = + require("@repo/database/dbDotEnv") as typeof import("@repo/database/dbDotEnv");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
apps/roam/scripts/compile.ts(1 hunks)apps/roam/scripts/deploy.ts(1 hunks)apps/website/app/utils/supabase/middleware.ts(1 hunks)apps/website/app/utils/supabase/server.ts(1 hunks)apps/website/next.config.ts(1 hunks)packages/database/features/step-definitions/stepdefs.ts(1 hunks)packages/database/package.json(2 hunks)packages/database/scripts/build.ts(1 hunks)packages/database/scripts/dev.ts(1 hunks)packages/database/src/dbDotEnv.mts(4 hunks)packages/database/tsconfig.json(1 hunks)packages/database/tsconfig_lint.json(1 hunks)packages/database/types/dbDotEnv.d.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/database/types/dbDotEnv.d.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#0
File: :0-0
Timestamp: 2025-08-31T17:38:52.730Z
Learning: The dbDotEnv functionality in packages/database/src/dbDotEnv.js is designed for Node.js environments only and will not be used in browser contexts.
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#400
File: packages/database/src/dbDotEnv.js:24-31
Timestamp: 2025-08-31T17:37:17.131Z
Learning: The "all" variant for SUPABASE_USE_DB should only be used in createEnv.mts and not in the dbDotEnv module. It was incorrectly added to dbDotEnv.d.ts but should be removed to maintain proper separation of concerns.
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#241
File: packages/database/tsconfig.json:3-7
Timestamp: 2025-06-25T18:03:52.669Z
Learning: The packages/database directory in the discourse-graph repository has a unique structure as a database schema/migration package. It contains doc/, scripts/, supabase/ directories and TypeScript files at the root level, but no typical src/, test/, dist/, or node_modules directories. The current tsconfig.json with "include": ["."] and "exclude": ["supabase"] is appropriate for this structure.
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#0
File: :0-0
Timestamp: 2025-07-22T01:50:20.253Z
Learning: In packages/database/scripts/create_env.ts, the Vercel CLI integration uses both vercel/sdk for deployment queries and vercel CLI for environment variable pulling, with support for different variants (local, branch, production, all) and proper team/project configuration with discourse-graphs defaults.
📚 Learning: 2025-06-25T18:03:52.669Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#241
File: packages/database/tsconfig.json:3-7
Timestamp: 2025-06-25T18:03:52.669Z
Learning: The packages/database directory in the discourse-graph repository has a unique structure as a database schema/migration package. It contains doc/, scripts/, supabase/ directories and TypeScript files at the root level, but no typical src/, test/, dist/, or node_modules directories. The current tsconfig.json with "include": ["."] and "exclude": ["supabase"] is appropriate for this structure.
Applied to files:
packages/database/tsconfig.jsonpackages/database/tsconfig_lint.jsonpackages/database/package.json
📚 Learning: 2025-08-31T17:37:17.131Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#400
File: packages/database/src/dbDotEnv.js:24-31
Timestamp: 2025-08-31T17:37:17.131Z
Learning: The "all" variant for SUPABASE_USE_DB should only be used in createEnv.mts and not in the dbDotEnv module. It was incorrectly added to dbDotEnv.d.ts but should be removed to maintain proper separation of concerns.
Applied to files:
apps/website/app/utils/supabase/server.tspackages/database/features/step-definitions/stepdefs.tspackages/database/scripts/build.tsapps/website/app/utils/supabase/middleware.tsapps/roam/scripts/compile.tspackages/database/scripts/dev.tspackages/database/package.jsonpackages/database/src/dbDotEnv.mts
📚 Learning: 2025-08-27T02:23:45.632Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#377
File: packages/ui/tsconfig.json:5-7
Timestamp: 2025-08-27T02:23:45.632Z
Learning: When reviewing TypeScript configuration changes, check if settings like "declaration" are inherited from base configs before suggesting to add them explicitly. Many monorepo packages extend base configurations that already include necessary compiler options.
Applied to files:
packages/database/tsconfig_lint.json
📚 Learning: 2025-07-19T22:34:08.725Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-07-19T22:34:08.725Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use Prettier with the project's configuration
Applied to files:
packages/database/tsconfig_lint.json
📚 Learning: 2025-06-25T18:11:58.352Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#241
File: apps/website/eslint.config.mjs:6-13
Timestamp: 2025-06-25T18:11:58.352Z
Learning: In TypeScript ESLint parser options, `parserOptions.project: true` is a valid boolean value that has been supported since version 5.52.0. When set to `true`, it automatically finds the nearest `tsconfig.json` file for each source file being linted, eliminating the need to specify explicit paths like `"./tsconfig.json"`. This is especially useful for monorepos and projects with multiple TypeScript configurations.
Applied to files:
packages/database/tsconfig_lint.json
📚 Learning: 2025-08-31T17:38:52.730Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#0
File: :0-0
Timestamp: 2025-08-31T17:38:52.730Z
Learning: The dbDotEnv functionality in packages/database/src/dbDotEnv.js is designed for Node.js environments only and will not be used in browser contexts.
Applied to files:
apps/website/next.config.tspackages/database/scripts/build.tsapps/roam/scripts/compile.tsapps/roam/scripts/deploy.tspackages/database/scripts/dev.tspackages/database/package.json
📚 Learning: 2025-07-22T01:50:20.253Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#0
File: :0-0
Timestamp: 2025-07-22T01:50:20.253Z
Learning: In packages/database/scripts/create_env.ts, the Vercel CLI integration uses both vercel/sdk for deployment queries and vercel CLI for environment variable pulling, with support for different variants (local, branch, production, all) and proper team/project configuration with discourse-graphs defaults.
Applied to files:
packages/database/scripts/build.tsapps/roam/scripts/deploy.tspackages/database/scripts/dev.ts
📚 Learning: 2025-05-22T23:48:45.450Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#165
File: packages/database/scripts/deploy.ts:0-0
Timestamp: 2025-05-22T23:48:45.450Z
Learning: The database deployment script in packages/database/scripts/deploy.ts requires that the branch is 'main' and pristine (no uncommitted or unpushed changes) before allowing deployment to proceed.
Applied to files:
apps/roam/scripts/compile.tsapps/roam/scripts/deploy.ts
🔇 Additional comments (7)
packages/database/src/dbDotEnv.mts (1)
6-11: Good: Variant excludes the “all” value per ENG-815 constraints.packages/database/scripts/build.ts (1)
4-4: Verify ESM support for .mts imports
Importing "../src/dbDotEnv.mts" requires an ESM-aware TS runner (e.g. ts-node --esm or tsx). CI/dev currently has no ts-node/tsx references—confirm your script-runner setup supports loading .mts files.apps/website/app/utils/supabase/middleware.ts (1)
3-11: IgnoredbDotEnvimport in middleware.ts:updateSessionin apps/website/app/utils/supabase/middleware.ts isn’t referenced anywhere in the repo, so there’s no Edge-runtime use of this Node-only module.Likely an incorrect or invalid review comment.
apps/roam/scripts/deploy.ts (1)
5-10: LGTM: CommonJS require for dbDotEnv in a Node script is appropriate.config() gated by NODE_ENV !== "production" is sensible; no issues.
apps/website/next.config.ts (1)
2-5: LGTM—No remaining ES imports fordbDotEnv
Search forimport … from '@repo/database/dbDotEnv'returned no matches; it’s safe to load the env only innext.config.ts.packages/database/package.json (2)
50-50: Lint config covers non-src TS tsconfig_lint.json’s include array already lists "./scripts//*" and "./features/step-definitions//*", ensuring step definitions and scripts are type-checked.
13-16: Ensurerequireexport targets a CJS fileUpdate
packages/database/package.jsonso that therequireexport points to a CommonJS build, and confirm that the file exists (or add a CJS build step). For example:"./dbDotEnv": { "types": "./dist/src/dbDotEnv.d.mts", "import": "./dist/src/dbDotEnv.mjs", - "require": "./dist/src/dbDotEnv.mjs", + "require": "./dist/src/dbDotEnv.cjs", "default": "./dist/src/dbDotEnv.mjs" },Verify that
packages/database/dist/src/dbDotEnv.cjsis produced (or add a wrapper/build config).
|
@CodeRabbit good call on typesVersions. Making types explicit in each require is anti-dry, not going that way. I did find the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9080 @maparent Thanks for the feedback! I'm glad the typesVersions suggestion resonated with you. You're absolutely right about avoiding explicit types in each require - that would indeed be anti-DRY and create maintenance overhead. The current approach with inline types in the Regarding the // This gets the type without importing the module
const someFunction = null as any as typeof import('./module').someFunction;However, you're correct that it has limitations:
For your shim concerns - I see you have the React The build pipeline changes look solid - moving type declarations to the built artifacts in ✏️ Learnings added
|
|
As stated, this seems to work for me; curious how it'll work for you. |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
After removing and all dist, running npm i (because package-lock.json it is off), both turbo check-types and turbo dev -F roam fail
|
Build is failing too 
|
maparent
force-pushed
the
ENG-815-normalize-dbDotEnv
branch
from
11e2366 to
8a91312
Compare
|
@mdroidian Ok. I tried re-running this, and @repo/database had an old typescript@4.1, imposed by vercel, which does not pick up on some settings. |
maparent
force-pushed
the
ENG-815-normalize-dbDotEnv
branch
from
efd241b to
0dd939a
Compare
|
Rebased on pnpm branch, should be good to go. |
maparent
force-pushed
the
ENG-815-normalize-dbDotEnv
branch
from
0dd939a to
a7a656b
Compare
maparent
force-pushed
the
eng-757-experiment-with-pnpm
branch
from
9239965 to
7023fbf
Compare
maparent
force-pushed
the
ENG-815-normalize-dbDotEnv
branch
2 times, most recently
from
05686ea to
210a59b
Compare
maparent
force-pushed
the
eng-757-experiment-with-pnpm
branch
from
7023fbf to
81e0a27
Compare
maparent
force-pushed
the
ENG-815-normalize-dbDotEnv
branch
from
210a59b to
78dcbc7
Compare
maparent
force-pushed
the
eng-757-experiment-with-pnpm
branch
from
81e0a27 to
8181a98
Compare
maparent
force-pushed
the
ENG-815-normalize-dbDotEnv
branch
from
78dcbc7 to
f3c2a6b
Compare
| "default": "./src/dbDotEnv.mjs" | ||
| "types": "./dist/src/dbDotEnv.d.mts", | ||
| "import": "./dist/src/dbDotEnv.mjs", | ||
| "require": "./dist/src/dbDotEnv.mjs", |
There was a problem hiding this comment.
This line annoys me; according to coderabbit, I should create a cjs wrapper and use it here.
Removing the line (as I did in the original PR) breaks the build. So may look into wrapping, even though things do work as-is.
mdroidian
force-pushed
the
eng-757-experiment-with-pnpm
branch
from
8181a98 to
a6b59ba
Compare
maparent
force-pushed
the
ENG-815-normalize-dbDotEnv
branch
2 times, most recently
from
a19ccaf to
209587d
Compare
maparent
force-pushed
the
ENG-815-normalize-dbDotEnv
branch
2 times, most recently
from
01c3c4c to
1d358a8
Compare
There are circular dependencies within `packages/database`, especially of stepdefs.ts on dbDotEnv, which is why I made it a mjs module. The alternative is to not compile stepdefs; cucumber can use it by reference. Then dbDotEnv can then be a mts module. We then have to compile it first (in a bootstrap stage) and then require it everywhere. Ensure stepdef and scripts are type-checked, now that they're not compiled.
maparent
force-pushed
the
ENG-815-normalize-dbDotEnv
branch
from
1d358a8 to
70a3f50
Compare
https://linear.app/discourse-graphs/issue/ENG-815/treat-dbdotenv-like-a-normal-ts-file-rather-than-separate-js-and-dts
There are circular dependencies within
packages/database,especially of
stepdefs.tsondbDotEnv, which is why I made it a mjs module.The alternative is to not compile
stepdefs.ts; cucumber can use it by reference.Then
dbDotEnvcan then be a.mtsmodule. We then have to require it everywhere.Ensure
stepdefand scripts are type-checked, now that they're not compiled.Summary by CodeRabbit
Impact: Stability and build consistency improvements with no changes to user workflows.