Skip to content

fix(workflows): replace libsql client + native fetch#2332

Open
finxol wants to merge 3 commits into
mainfrom
fix/workflows-deno
Open

fix(workflows): replace libsql client + native fetch#2332
finxol wants to merge 3 commits into
mainfrom
fix/workflows-deno

Conversation

@finxol

@finxol finxol commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Review in cubic

@vercel

vercel Bot commented Jul 1, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
openstatus-dashboard Ready Ready Preview, Comment Jul 1, 2026 10:59am
openstatus-status-page Ready Ready Preview, Comment Jul 1, 2026 10:59am
openstatus-web Ready Ready Preview, Comment Jul 1, 2026 10:59am

Request Review

@vercel vercel Bot temporarily deployed to Preview – openstatus-status-page July 1, 2026 10:26 Inactive
@vercel vercel Bot temporarily deployed to Preview – openstatus-web July 1, 2026 10:26 Inactive
@vercel vercel Bot temporarily deployed to Preview – openstatus-dashboard July 1, 2026 10:26 Inactive
@finxol

finxol commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

@claude review this

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown

Claude finished @finxol's task in 4m 32s —— View job


Review complete

  • Gather context (PR diff, changed files)
  • Review apps/workflows/src/lib/db.ts changes (libsql client replacement)
  • Review apps/workflows/src/lib/native-fetch.ts (new file)
  • Review apps/workflows/src/serve.ts changes
  • Review Dockerfile / dofigen config changes
  • Post final review

Summary

Good direction overall — switching from @libsql/client to the lower-level libsql (better-sqlite3-compatible) driver to avoid the Object.defineProperty row-rebuilding overhead, and shimming window so google-gax picks native fetch instead of node-fetch, are both sound, well-reasoned perf fixes for the Deno runtime. The comments explaining why (CPU cost on V8, @t3-oss/env's "Deno" in window check) are exactly the kind of non-obvious context worth keeping.

🐛 Likely type error: syncUrl missing from ReplicaOptions

apps/workflows/src/lib/db.ts:15-24:

type ReplicaOptions = Database.Options & {
  authToken?: string;
  syncPeriod?: number;
};

const options: ReplicaOptions = {
  syncUrl: env().DATABASE_URL,
  authToken: env().DATABASE_AUTH_TOKEN,
  syncPeriod: 60,
};

The comment above states libsql's typings are better-sqlite3-derived and omit the embedded-replica options — that's why authToken and syncPeriod were added to the intersection. But syncUrl was left out of ReplicaOptions, even though it's used in the object literal on line 21. Since Database.Options (better-sqlite3-derived) has no syncUrl field either, this object literal assigned directly to const options: ReplicaOptions should trip TypeScript's excess-property check ("Object literal may only specify known properties, and 'syncUrl' does not exist in type 'ReplicaOptions'").

This isn't just a lint nit — both compile paths for this app type-check:

  • apps/workflows/package.json:4: "check": "deno check --sloppy-imports src/serve.ts", run repo-wide via pnpm checkturbo run check (.github/workflows/check.yml).
  • apps/workflows/Dockerfile:78: deno bundle (no --no-check flag — only the later deno compile step at line 79 skips checking).

So as written this looks like it would fail both CI's check job and the Docker image build. Fix: add syncUrl?: string; to the ReplicaOptions intersection alongside authToken and syncPeriod.

Fix this →

Minor note

@libsql/client is still listed as a direct dependency in apps/workflows/package.json:11 and still referenced in the Dockerfile/dofigen --external flags, but db.ts no longer imports it anywhere in apps/workflows/src (only mentioned in a comment now). Worth double-checking whether it's still needed transitively (e.g. by drizzle-orm's optional peer deps) — if not, it and its dofigen sidecar entry could be dropped in a follow-up, though I wouldn't block this PR on it.

Verification note

I wasn't able to run pnpm install / deno check / a Docker build in this sandbox (commands beyond read-only git operations required interactive approval that wasn't available), so the syncUrl finding is based on static analysis of the type declarations rather than a reproduced compiler error — worth confirming with a local deno check before merging.

@vercel vercel Bot temporarily deployed to Preview – openstatus-dashboard July 1, 2026 10:45 Inactive
@vercel vercel Bot temporarily deployed to Preview – openstatus-status-page July 1, 2026 10:45 Inactive
@vercel vercel Bot temporarily deployed to Preview – openstatus-web July 1, 2026 10:45 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant