feat: add neon for serverless postgres#191
Open
atilafassina wants to merge 12 commits into
Open
Conversation
atilafassina
force-pushed
the
connector/neon
branch
from
8e49b3e to
8cc74c5
Compare
pi0
changed the title
pi0
changed the title
neon for serverless postgres
📝 WalkthroughWalkthroughAdds Neon and Neon Instant PostgreSQL connectors, shared client and SQL handling, instant database provisioning, connector registration, tests, optional dependencies, and updated documentation. ChangesNeon connector integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant neonInstantConnector
participant neonNew
participant pgClient
Application->>neonInstantConnector: create connector
neonInstantConnector->>neonNew: provision database when no URL exists
neonNew-->>neonInstantConnector: return databaseUrl
neonInstantConnector->>pgClient: connect resolved database
Application->>pgClient: execute normalized SQL
pgClient-->>Application: return query result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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: 4
🧹 Nitpick comments (1)
src/connectors/neon.ts (1)
46-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid spreading non-
pg.ClientConfigproperties intopg.Client.
{ ...opts, connectionString }passes allConnectorOptionsproperties — includingurl,neverGenerateConnectionString, andInstantNeonParamsfields likebindingName/seed— into thepg.Clientconstructor. Whilepg.Clientmay silently ignore unknown properties, this is leaky and could cause issues if the constructor ever validates its config.♻️ Proposed refactor: destructure and pass only pg-relevant options
const client = typeof opts === "object" - ? new pg.Client({ ...opts, connectionString }) + ? new pg.Client({ ...opts, connectionString, url: undefined, neverGenerateConnectionString: undefined, bindingName: undefined, seed: undefined, referrer: undefined }) : new pg.Client(connectionString);A cleaner approach would be to extract only
pg.ClientConfig-compatible keys fromoptsbefore spreading.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/connectors/neon.ts` around lines 46 - 49, Update the client construction in the connector flow to avoid spreading the full opts object into pg.Client. Destructure or otherwise select only pg.ClientConfig-compatible options, then pass those together with connectionString; exclude connector-specific fields such as url, neverGenerateConnectionString, bindingName, and seed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/2.connectors/neon.md`:
- Line 72: Update the Neon connector documentation to remove the unsupported
environment-variable fallback claim, accurately stating that it uses the
provided connection options or automatically provisions a database in
development, while preserving the production error behavior when no connection
value is supplied.
- Around line 29-31: Correct the Usage section text by changing “Servleress” to
“Serverless,” replacing the incorrect “Instagres’” wording, and capitalizing
“Postgres.” Also update the installation command in the `:pm-install` directive
to use the correctly spelled `@neondatabase/serverless` package name.
In `@src/connectors/neon.ts`:
- Around line 29-31: Update the Neon connector flow around instantNeon to
forward the user-supplied ConnectorOptions/InstantNeonParams, including
bindingName and seed, while retaining the existing referrer value. Ensure the
provisioning call receives these options instead of only the static referrer.
- Around line 1-5: Move the get-db/sdk import out of module scope and lazily
load instantNeon and InstantNeonParams within getConnectionString(), while
preserving the existing dev-only guard behavior. Add `@neondatabase/serverless` to
runtime dependencies so the neon connector resolves in production installs.
---
Nitpick comments:
In `@src/connectors/neon.ts`:
- Around line 46-49: Update the client construction in the connector flow to
avoid spreading the full opts object into pg.Client. Destructure or otherwise
select only pg.ClientConfig-compatible options, then pass those together with
connectionString; exclude connector-specific fields such as url,
neverGenerateConnectionString, bindingName, and seed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1105f401-2978-48c8-8781-738c0c1ab42a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
docs/2.connectors/neon.mddocs/2.connectors/vercel.mdpackage.jsonsrc/connectors/neon.tstest/connectors/neon.connstring.tstest/connectors/neon.test.tstest/test-db.sql
The neon connector imported `@neondatabase/serverless` and `get-db/sdk` without either being declared in package.json, so `tsc --noEmit` failed with TS2307 on both imports. Current `get-db` returns `databaseUrl` (the pooler string) rather than `poolerUrl`, so update the connector and the mock accordingly, and rename the connstring test so it matches vitest's include glob and actually runs. Co-Authored-By: Claude Opus 4.8 <[email protected]>
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)
test/connectors/neon.connstring.test.ts (1)
40-42: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSet
NODE_ENVexplicitly in this test.
This assertion depends on the runner’s ambient environment; if CI runs withNODE_ENV=production,instantNeonwon’t be called and the test will fail. Restore the original value after the test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/connectors/neon.connstring.test.ts` around lines 40 - 42, Update the test around createDatabase and db.getInstance to explicitly set NODE_ENV to a non-production value before execution, then restore the original NODE_ENV in cleanup so the test remains isolated and does not depend on the runner environment.
🧹 Nitpick comments (1)
test/connectors/neon.connstring.test.ts (1)
40-43: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that
databaseUrlreaches the Neon client.The test only verifies that
instantNeonwas called, so it can pass even if the returned URL is ignored or an undefined connection string is passed to the mocked client. Capture the client constructor arguments and assert the generated URL is used.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/connectors/neon.connstring.test.ts` around lines 40 - 43, Update the test around createDatabase and getInstance to capture the arguments passed to the mocked getDbSdk.instantNeon call, then assert that its databaseUrl argument matches the generated connection string. Retain the existing assertion that the client is called once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 48: Move `@neondatabase/serverless` and get-db/sdk from devDependencies to
dependencies in package.json so the published dist/connectors output can resolve
the bare imports used by the Neon connector at runtime.
In `@src/_connectors.ts`:
- Around line 13-20: Update scripts/gen-connectors.ts to include Neon in the
generator inputs, then regenerate src/_connectors.ts rather than editing the
generated file directly. Ensure the regenerated connector definitions and
registrations include Neon consistently across all affected sections.
---
Outside diff comments:
In `@test/connectors/neon.connstring.test.ts`:
- Around line 40-42: Update the test around createDatabase and db.getInstance to
explicitly set NODE_ENV to a non-production value before execution, then restore
the original NODE_ENV in cleanup so the test remains isolated and does not
depend on the runner environment.
---
Nitpick comments:
In `@test/connectors/neon.connstring.test.ts`:
- Around line 40-43: Update the test around createDatabase and getInstance to
capture the arguments passed to the mocked getDbSdk.instantNeon call, then
assert that its databaseUrl argument matches the generated connection string.
Retain the existing assertion that the client is called once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44a18b44-4e7f-4ee5-84c1-2725f651bd24
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.jsonsrc/_connectors.tssrc/connectors/neon.tstest/connectors/neon.connstring.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/connectors/neon.ts
| "@cloudflare/workers-types": "^5", | ||
| "@electric-sql/pglite": "^0.5.4", | ||
| "@libsql/client": "^0.17.4", | ||
| "@neondatabase/serverless": "^1.1.0", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## package.json relevant lines\n'
nl -ba package.json | sed -n '1,220p'
printf '\n## neon connector\n'
if [ -f src/connectors/neon.ts ]; then
nl -ba src/connectors/neon.ts | sed -n '1,220p'
fi
printf '\n## publish/bundle/search\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/.git/**' \
'(files"|publishConfig|bundl|tsup|rollup|esbuild|webpack|vite|pkg\.exports|exports|main"|module"|type"|prepare|prepack|prepublishOnly|devDependencies|dependencies)' \
package.json . 2>/dev/null | sed -n '1,220p'Repository: unjs/db0
Length of output: 218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## package.json\n'
sed -n '1,220p' package.json | cat -n
printf '\n## src/connectors/neon.ts\n'
if [ -f src/connectors/neon.ts ]; then
sed -n '1,220p' src/connectors/neon.ts | cat -n
fi
printf '\n## publish/bundle config search\n'
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/.git/**' \
'(files|publishConfig|bundle|bundl|tsup|rollup|esbuild|webpack|vite|prepack|prepublishOnly|prepare|exports|main|module|type)' \
package.json . 2>/dev/null | sed -n '1,260p'
else
grep -RInE --exclude-dir=.git --exclude-dir=node_modules \
'(files|publishConfig|bundle|bundl|tsup|rollup|esbuild|webpack|vite|prepack|prepublishOnly|prepare|exports|main|module|type)' \
package.json . | sed -n '1,260p'
fiRepository: unjs/db0
Length of output: 23958
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## obuild-related files\n'
find . -maxdepth 2 \( -name '*obuild*' -o -name '.obuild*' -o -name 'build.*' -o -name 'tsup.*' -o -name 'rollup.*' -o -name 'vite.*' -o -name 'unbuild.*' \) | sed 's#^\./##' | sort
printf '\n## search for bundling/externalization hints\n'
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/.git/**' \
'(external|bundle|bundled|noExternal|inlineDependencies|inlineDeps|emit.*d\.ts|preserveModules|metafile|rollupOptions|esbuildOptions|externals?)' \
. 2>/dev/null | sed -n '1,260p'
else
grep -RInE --exclude-dir=.git --exclude-dir=node_modules \
'(external|bundle|bundled|noExternal|inlineDependencies|inlineDeps|emit.*d\.ts|preserveModules|metafile|rollupOptions|esbuildOptions|externals?)' \
. | sed -n '1,260p'
fi
printf '\n## all package.json entries for scripts/build tooling\n'
sed -n '1,140p' package.json | cat -nRepository: unjs/db0
Length of output: 4795
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## build.config.ts\n'
sed -n '1,200p' build.config.ts | cat -n
printf '\n## connector build outputs/search\n'
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/.git/**' \
'(src/connectors|connectors/\*|type: "bundle"|type: "copy"|type: "transpile"|type: "unbundle"|obuild)' \
build.config.ts scripts src .github AGENTS.md 2>/dev/null | sed -n '1,240p'
fiRepository: unjs/db0
Length of output: 1255
Move Neon runtime packages out of devDependencies.
src/connectors/neon.ts is transformed into dist/connectors/* without bundling, so its bare imports stay in the published output. @neondatabase/serverless and get-db/sdk need to be runtime dependencies or consumers importing the Neon connector will hit ERR_MODULE_NOT_FOUND.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 48, Move `@neondatabase/serverless` and get-db/sdk from
devDependencies to dependencies in package.json so the published dist/connectors
output can resolve the bare imports used by the Neon connector at runtime.
Comment on lines
+13
to
+20
| import type { ConnectorOptions as NeonOptions } from "db0/connectors/neon"; | ||
| import type { ConnectorOptions as NodeSQLiteOptions } from "db0/connectors/node-sqlite"; | ||
| import type { ConnectorOptions as PgliteOptions } from "db0/connectors/pglite"; | ||
| import type { ConnectorOptions as PlanetscaleOptions } from "db0/connectors/planetscale"; | ||
| import type { ConnectorOptions as PostgreSQLOptions } from "db0/connectors/postgresql"; | ||
| import type { ConnectorOptions as SQLite3Options } from "db0/connectors/sqlite3"; | ||
|
|
||
| export type ConnectorName = "better-sqlite3" | "bun-sqlite" | "bun" | "cloudflare-d1" | "cloudflare-hyperdrive-mysql" | "cloudflare-hyperdrive-postgresql" | "libsql-core" | "libsql-http" | "libsql-node" | "libsql" | "libsql-web" | "mysql2" | "node-sqlite" | "sqlite" | "pglite" | "planetscale" | "postgresql" | "sqlite3"; | ||
| export type ConnectorName = "better-sqlite3" | "bun-sqlite" | "bun" | "cloudflare-d1" | "cloudflare-hyperdrive-mysql" | "cloudflare-hyperdrive-postgresql" | "libsql-core" | "libsql-http" | "libsql-node" | "libsql" | "libsql-web" | "mysql2" | "neon" | "node-sqlite" | "sqlite" | "pglite" | "planetscale" | "postgresql" | "sqlite3"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Regenerate this file instead of editing it directly.
src/_connectors.ts is auto-generated by scripts/gen-connectors.ts; add Neon to the generator inputs and regenerate this file so the registration cannot be overwritten.
Also applies to: 37-37, 62-62
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/_connectors.ts` around lines 13 - 20, Update scripts/gen-connectors.ts to
include Neon in the generator inputs, then regenerate src/_connectors.ts rather
than editing the generated file directly. Ensure the regenerated connector
definitions and registrations include Neon consistently across all affected
sections.
Source: Coding guidelines
`get-db` is deprecated on npm and has been renamed to `neon-new` (the former is now just a shim re-exporting the latter). Depend on `neon-new` directly and switch from the deprecated `instantNeon`/`InstantNeonParams` aliases to `instantPostgres`/`InstantPostgresParams`. Also fixes the install snippet in the docs, which named a package that does not exist (`@neondatbse/serverless`). Co-Authored-By: Claude Opus 4.8 <[email protected]>
The `neon` connector is now SDK-only: it talks to @neondatabase/serverless and requires a connection string, so it no longer drags `neon-new` into the dependency graph of everyone who just wants to connect to an existing Neon database. Auto-provisioning moves to a new `neon-instant` connector, which resolves a connection string via `neon-new` when none is given (outside production) and otherwise behaves identically. Shared client/statement logic lives in `_internal/neon.ts`, which both connectors delegate to. `neverGenerateConnectionString` is dropped, as picking the connector is now the choice it used to express. Provisioning options (`seed`, `dotEnvFile`, `referrer`, ...) are now actually forwarded to `instantPostgres` — they were previously accepted in the type but ignored. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Review follow-ups on the neon connectors:
- Fix a concurrent-init race: `getClient()` awaited connection-string
resolution before caching the in-flight promise, so N concurrent first
queries built N clients. On `neon-instant` that meant a `Promise.all` of
queries provisioned several real databases and leaked all but one socket.
The promise is now assigned synchronously.
- Support `ClientConfig` without a connection string (`host`/`user`/...),
which the types and docs promised but the resolver rejected. Resolution
also no longer trips over `{ url: undefined, connectionString: "..." }`.
- Import `neon-new` lazily, so a dev-only CLI (and its 9 runtime deps) stays
out of production bundles of `neon-instant`.
- Reuse an already-provisioned database from `DATABASE_URL` instead of
provisioning a fresh one on every restart. `instantPostgres` appends to
`.env` and never checks it, so restarts were piling up claimable databases.
- Do not cache a failed `connect()` forever, and let `dispose()` run after
one instead of re-throwing it.
- Keep provisioning options (`seed`, `referrer`, ...) out of the pg ClientConfig.
- Teach `normalizeParams` to skip `?` in string literals, comments, and the
jsonb `?|` / `?&` / `??` operators. The same bug remains in postgresql.ts.
- Declare the drivers as optional peer dependencies, and drop an unused fixture.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/connectors/_internal/neon.ts (1)
86-90: 📐 Maintainability & Code Quality | 🔵 TrivialSplit double
awaitindispose()for readability.The nested
await (await client?.catch(() => undefined))?.end?.()is hard to parse. Splitting into two separateawaitcalls produces identical behavior with clearer intent.♻️ Proposed refactor
dispose: async () => { const client = _client; _client = undefined; - await (await client?.catch(() => undefined))?.end?.(); + const resolved = await client?.catch(() => undefined); + await resolved?.end?.(); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/connectors/_internal/neon.ts` around lines 86 - 90, Refactor the dispose callback to split the nested await expression into separate steps: first await the optional client promise with its existing rejection handling, then await the optional end method on the resolved client. Preserve the current cleanup order and behavior in dispose.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/2.connectors/neon-instant.md`:
- Line 16: Update the dotEnvKey documentation link target in the connector guide
to match the generated heading anchor for “dotEnvFile / dotEnvKey,” using the
correct single separator rather than the current duplicated separator.
In `@docs/2.connectors/neon.md`:
- Line 23: Update the Neon connector documentation text around the connection
requirements to state that an existing database identifier is required, rather
than requiring a connection string. Ensure the wording remains consistent with
support for host-based ClientConfig values, including the related guidance at
the other referenced section.
In `@src/connectors/neon-instant.ts`:
- Around line 27-38: Update the resolver callback passed to createNeonConnector
so an explicitly provided clientOpts.host is returned first, before
resolveStaticConnectionString or the dotEnvKey environment lookup. Preserve the
existing fallback behavior when host is absent, and add regression coverage for
host values with and without DATABASE_URL.
In `@test/connectors/neon.unit.test.ts`:
- Around line 204-208: Update normalizeParams to preserve standalone PostgreSQL
JSONB existence operators when scanning parameter markers, while continuing to
convert actual placeholders and preserve ?|, ?&, and ?? operators. Extend
test/connectors/neon.unit.test.ts with a regression case covering data ? 'key'
and verify the expected SQL remains valid.
---
Nitpick comments:
In `@src/connectors/_internal/neon.ts`:
- Around line 86-90: Refactor the dispose callback to split the nested await
expression into separate steps: first await the optional client promise with its
existing rejection handling, then await the optional end method on the resolved
client. Preserve the current cleanup order and behavior in dispose.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cd0f0b2-032c-4514-a710-1795d4ae3c3f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
docs/2.connectors/1.index.mddocs/2.connectors/neon-instant.mddocs/2.connectors/neon.mdpackage.jsonsrc/_connectors.tssrc/connectors/_internal/neon.tssrc/connectors/neon-instant.tssrc/connectors/neon.tstest/connectors/neon-instant.test.tstest/connectors/neon.test.tstest/connectors/neon.unit.test.ts
💤 Files with no reviewable changes (1)
- test/connectors/neon.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/_connectors.ts
| This connector behaves exactly like the [Neon connector](/connectors/neon), except that it does not require a connection string. On first use, it resolves one in this order: | ||
|
|
||
| 1. The `url` / `connectionString` option, if given. | ||
| 2. The `DATABASE_URL` environment variable (see [`dotEnvKey`](#dotenvfile-dotenvkey)), if set. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the dotEnvKey section fragment.
The generated anchor for ### dotEnvFile / dotEnvKey contains two separators.
Proposed fix
-2. The `DATABASE_URL` environment variable (see [`dotEnvKey`](`#dotenvfile-dotenvkey`)), if set.
+2. The `DATABASE_URL` environment variable (see [`dotEnvKey`](`#dotenvfile--dotenvkey`)), if set.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 2. The `DATABASE_URL` environment variable (see [`dotEnvKey`](#dotenvfile-dotenvkey)), if set. | |
| 2. The `DATABASE_URL` environment variable (see [`dotEnvKey`](`#dotenvfile--dotenvkey`)), if set. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 16-16: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/2.connectors/neon-instant.md` at line 16, Update the dotEnvKey
documentation link target in the connector guide to match the generated heading
anchor for “dotEnvFile / dotEnvKey,” using the correct single separator rather
than the current duplicated separator.
Source: Linters/SAST tools
|
|
||
| :pm-install{name="@neondatabase/serverless"} | ||
|
|
||
| This connector always connects to an existing database, so a connection string is required. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe the required database identifier, not a required connection string.
Line 23 contradicts the documented and tested support for host-based ClientConfig values.
Proposed fix
-This connector always connects to an existing database, so a connection string is required.
+This connector always connects to an existing database, so it requires either a connection string or another identifying `ClientConfig` field such as `host`.Also applies to: 44-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/2.connectors/neon.md` at line 23, Update the Neon connector
documentation text around the connection requirements to state that an existing
database identifier is required, rather than requiring a connection string.
Ensure the wording remains consistent with support for host-based ClientConfig
values, including the related guidance at the other referenced section.
Comment on lines
+27
to
+38
| return createNeonConnector("neon-instant", clientOpts, async (clientOpts) => { | ||
| const connectionString = resolveStaticConnectionString(clientOpts); | ||
| if (connectionString) { | ||
| return connectionString; | ||
| } | ||
|
|
||
| // Reuse the database provisioned by an earlier run, if it is still around. | ||
| const fromEnv = globalThis.process?.env?.[dotEnvKey]; | ||
| if (fromEnv) { | ||
| return fromEnv; | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve an explicit ClientConfig.host before consulting the environment.
createNeonConnector accepts host as a complete database identifier, but this resolver still selects DATABASE_URL or provisions a new database. Consequently, neonInstantConnector({ host: ... }) can connect to the wrong database.
Proposed fix
const connectionString = resolveStaticConnectionString(clientOpts);
if (connectionString) {
return connectionString;
}
+ if (clientOpts && "host" in clientOpts && clientOpts.host) {
+ return;
+ }
+
// Reuse the database provisioned by an earlier run, if it is still around.Add a regression test covering host both with and without DATABASE_URL.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return createNeonConnector("neon-instant", clientOpts, async (clientOpts) => { | |
| const connectionString = resolveStaticConnectionString(clientOpts); | |
| if (connectionString) { | |
| return connectionString; | |
| } | |
| // Reuse the database provisioned by an earlier run, if it is still around. | |
| const fromEnv = globalThis.process?.env?.[dotEnvKey]; | |
| if (fromEnv) { | |
| return fromEnv; | |
| } | |
| return createNeonConnector("neon-instant", clientOpts, async (clientOpts) => { | |
| const connectionString = resolveStaticConnectionString(clientOpts); | |
| if (connectionString) { | |
| return connectionString; | |
| } | |
| if (clientOpts && "host" in clientOpts && clientOpts.host) { | |
| return; | |
| } | |
| // Reuse the database provisioned by an earlier run, if it is still around. | |
| const fromEnv = globalThis.process?.env?.[dotEnvKey]; | |
| if (fromEnv) { | |
| return fromEnv; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/connectors/neon-instant.ts` around lines 27 - 38, Update the resolver
callback passed to createNeonConnector so an explicitly provided clientOpts.host
is returned first, before resolveStaticConnectionString or the dotEnvKey
environment lookup. Preserve the existing fallback behavior when host is absent,
and add regression coverage for host values with and without DATABASE_URL.
Comment on lines
+204
to
+208
| test("leaves jsonb operators alone", () => { | ||
| expect(normalizeParams("SELECT * FROM t WHERE data ?| ? AND d ?& ?")).toBe( | ||
| "SELECT * FROM t WHERE data ?| $1 AND d ?& $2", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== files ==\n'
git ls-files | rg '(^|/)(neon\.unit\.test\.ts|_tests\.ts|normalize.*\.(ts|js)|dialect.*\.(ts|js)|sql.*normalize.*\.(ts|js))$'
printf '\n== outlines ==\n'
for f in test/connectors/neon.unit.test.ts test/connectors/_tests.ts; do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --view expanded || true
fi
done
printf '\n== search normalizeParams ==\n'
rg -n "normalizeParams|\\?\\||\\?&|\\?\\?" test src . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'
printf '\n== read relevant slices ==\n'
for f in test/connectors/neon.unit.test.ts test/connectors/_tests.ts; do
if [ -f "$f" ]; then
echo "--- $f"
nl -ba "$f" | sed -n '1,280p'
fi
doneRepository: unjs/db0
Length of output: 5849
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== src/connectors/_internal/neon.ts ==\n'
if [ -f src/connectors/_internal/neon.ts ]; then
sed -n '1,220p' src/connectors/_internal/neon.ts
fi
printf '\n== test/connectors/neon.unit.test.ts ==\n'
if [ -f test/connectors/neon.unit.test.ts ]; then
sed -n '180,240p' test/connectors/neon.unit.test.ts
fiRepository: unjs/db0
Length of output: 6155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== search for literal ?? handling ==\n'
rg -n "\?\?" src test . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'
printf '\n== inspect other normalizers ==\n'
for f in src/connectors/postgresql.ts src/connectors/cloudflare-hyperdrive-postgresql.ts src/connectors/pglite.ts; do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,140p' "$f"
echo
fi
doneRepository: unjs/db0
Length of output: 8917
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== src/template.ts ==\n'
sed -n '1,140p' src/template.ts
printf '\n== search docs and tests for placeholder escaping ==\n'
rg -n "literal \\?|escape.*\\?|\\?\\?|placeholder" README* docs src test --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'
printf '\n== search for JSONB operator tests elsewhere ==\n'
rg -n "jsonb|\\?\\||\\?&|standalone \\?" src test --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'Repository: unjs/db0
Length of output: 4253
Preserve PostgreSQL JSONB ? operator
normalizeParams() rewrites every ? unless it’s ?|, ?&, or ??, so data ? 'key' becomes invalid SQL. Add a regression test and make the scanner distinguish standalone JSONB existence operators from placeholders.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/connectors/neon.unit.test.ts` around lines 204 - 208, Update
normalizeParams to preserve standalone PostgreSQL JSONB existence operators when
scanning parameter markers, while continuing to convert actual placeholders and
preserve ?|, ?&, and ?? operators. Extend test/connectors/neon.unit.test.ts with
a regression case covering data ? 'key' and verify the expected SQL remains
valid.
Source: Coding guidelines
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
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.
part of #32
This PR introduces 3 integration options.
As opposed to the existing Postgres integration (that works with
pg), this one works with Neon's Serverless Driver.The driver offer 3 connection options.
Poolis a client that connects via HTTP and WebSockets, it has a few operation differences than a regular client, most noticiable in the code is that it does not need to be disposed.Clientthe most similar to thepgconnector, this is an HTTP client.Testing
@pi0 what are your thoughts on testing, the way I see it we have 3 options.
get-dblibrary to create and seed a new DB per connector during CI runs. These DBs are ephemeral and need no credentials. (Neon Launchpadunjsaccount in Neon and have one DB for each test setup, makes it easier to debug if tests misbehave because we have access to the dashboard.Imho, the quickest and easiest is number 2. But it requires adding the
env_varsto the repository and setting up the account (which I'm more than happy to do and make sure you have access to it).Going forward for Postgres
Since both the Neon and Postgres connectors work in a very very similar fashion, I'd love to create a
postgres/serverlessconnector and have Neon be an alias to it. So, the way I'd like to suggest is:postgres/serverlesspostgres/tcpneon/httpneon/wsneon(as an alias topostgres/serverless)I can expand the docs with clear reasons when to choose which of each options.
Auto-generating connection strings
How do you feel about leveraging
import { instantPostgres } from 'get-db'to generate a connection string to people who choose one of the above connectors but did not pass a url? We do that only in dev (to avoid weirdness in CI and deploy), and extend the docs to make it clearer, something likeThoughts?
Summary by CodeRabbit
New Features
Documentation