refactor(orm): consolidate kysely dialects under /dialects exports

[ymc9]

Summary by CodeRabbit

• New Features

  • Added direct dialect support in the main ORM for SQLite, PostgreSQL, and SQL.js.

• Chores

  • Consolidated SQL.js dialect into the ORM package (standalone SQL.js package artifacts removed).
  • Updated package exports and build entries to expose new dialects and optional sql.js dependency.

[coderabbitai]

Walkthrough

This PR removes the standalone packages/dialects/sql.js package (sources, tests, configs, package.json) and adds re-export entry points for sqlite, postgres, and sql.js into the main ORM package with updated package exports, tsup entries, and workspace catalog entries.

Changes

Cohort / File(s)	Summary
Removed sql.js package packages/dialects/sql.js/*	Deleted entire package directory: README.md, package.json, src/index.ts (barrel exports), configs (eslint.config.js, tsconfig.json, tsup.config.ts, vitest.config.ts), and tests (test/getting-started/*).
Added ORM dialect re-exports packages/orm/src/dialects/sql.js/index.ts, packages/orm/src/dialects/postgres.ts, packages/orm/src/dialects/sqlite.ts	New modules added that re-export dialect classes and config types for sqlite, postgres, and sql.js.
Updated ORM package manifest packages/orm/package.json	Added public export paths for ./dialects/sqlite, ./dialects/postgres, ./dialects/sql.js; adjusted ./helpers export; added @types/sql.js to devDependencies and sql.js to peerDependencies (optional).
Updated ORM build config packages/orm/tsup.config.ts	Added tsup entries: dialects/sqlite, dialects/postgres, dialects/sql.js pointing to respective source files.
Workspace catalog update pnpm-workspace.yaml	Added sql.js (version ^1.13.0) to the catalog section.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Verify package.json export paths map exactly to dist outputs and match tsup entry names.
• Confirm re-exported types/classes in packages/orm/src/dialects/* cover the original public surface required by consumers.
• Check there are no orphaned references to the removed packages/dialects/sql.js package elsewhere in the repo.

Possibly related PRs

• merge dev to main (v3.0.0-beta.15) #354 — moves/removes the standalone packages/dialects/sql.js package while adding orm-level dialect exports (directly related consolidation).
• merge dev to main (v3.0.0-alpha.17) #137 — touches the same sql.js dialect package surface and tests; likely the inverse or closely related change.
• merge dev to main (v3.0.0-alpha.16) #132 — modifies how Kysely dialects are exposed/consumed; related to re-export and API surface changes in ORM.

Poem

🐰 I hopped from package to ORM with glee,

sql.js tucked snug where all can see,
Exports aligned in a tidy row,
Tests and configs waved as I go,
A little rabbit dance — tidy and free 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: consolidating Kysely dialects under /dialects exports in the orm package, which aligns with adding new dialect exports and refactoring the package structure.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch chore/kysely-dialect-reexport

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ad1a500 and db54f49.

⛔ Files ignored due to path filters (1)

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (1)

• packages/orm/package.json (3 hunks)

🔇 Additional comments (3)

packages/orm/package.json (3)

39-68: ✓ Dialects exports are well-structured and consistent.

The new dialect export paths follow the established pattern with matching import/require/types entries for each dialect. The paths align with the new modular export approach for sqlite, postgres, and sql.js dialects.

---

101-102: ✓ sql.js peer dependency correctly configured as optional.

The sql.js entry in peerDependencies is properly marked as optional in peerDependenciesMeta, matching the pattern for other optional peer dependencies (better-sqlite3, pg).

Also applies to: 111-113

---

123-124: Verify @types/sql.js placement in devDependencies vs peerDependencies.

Since sql.js is configured as an optional peerDependency, its type definitions should follow the same pattern. Currently @types/sql.js is in devDependencies rather than peerDependencies (marked optional). Verify whether this is intentional (e.g., types only needed during development) or if it should be moved to peerDependencies to match the optional dependency structure.

Can you confirm the intended placement? If type definitions should be available to consumers using the sql.js dialect, move @types/sql.js to peerDependencies and mark it as optional:

     "peerDependencies": {
         "better-sqlite3": "catalog:",
         "pg": "catalog:",
         "zod": "catalog:",
-        "sql.js": "catalog:"
+        "sql.js": "catalog:",
+        "@types/sql.js": "catalog:"
     },

Then update peerDependenciesMeta to mark it optional, and remove it from devDependencies.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[Copilot]

Pull Request Overview

This PR refactors the kysely dialects architecture by consolidating them under the @zenstackhq/orm package's /dialects exports, removing the standalone @zenstackhq/kysely-sql-js package.

Key Changes:

• Moved sql.js dialect implementation from a separate package (packages/dialects/sql.js) into the main ORM package under packages/orm/src/dialects/sql.js/
• Added new dialect export paths for sqlite, postgres, and sql.js under @zenstackhq/orm/dialects/*
• Added sql.js as an optional peer dependency in the ORM package

Reviewed Changes

Copilot reviewed 17 out of 22 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pnpm-workspace.yaml	Added sql.js version to the catalog for dependency management
pnpm-lock.yaml	Updated lockfile with sql.js dependencies and resolved package references
packages/orm/tsup.config.ts	Added build entries for the three dialect exports (sqlite, postgres, sql.js)
packages/orm/src/dialects/sqlite.ts	Re-exports SqliteDialect from kysely for convenience
packages/orm/src/dialects/postgres.ts	Re-exports PostgresDialect from kysely for convenience
packages/orm/src/dialects/sql.js/index.ts	Main export file for sql.js dialect with attribution comment
packages/orm/src/dialects/sql.js/types.ts	TypeScript type definitions for SqlJsDialectConfig
packages/orm/src/dialects/sql.js/dialect.ts	SqlJsDialect class implementation with SQLite adapters
packages/orm/src/dialects/sql.js/driver.ts	SqlJsDriver implementation for connection and transaction management
packages/orm/src/dialects/sql.js/connection.ts	SqlJsConnection implementation for query execution
packages/orm/package.json	Added dialect export paths and sql.js as optional peer dependency
packages/dialects/sql.js/*	Removed standalone package files (package.json, configs, tests, README)

Files not reviewed (1)

• pnpm-lock.yaml: Language not supported

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dc53b11 and ad1a500.

⛔ Files ignored due to path filters (1)

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (17)

• packages/dialects/sql.js/README.md (0 hunks)
• packages/dialects/sql.js/eslint.config.js (0 hunks)
• packages/dialects/sql.js/package.json (0 hunks)
• packages/dialects/sql.js/src/index.ts (0 hunks)
• packages/dialects/sql.js/test/getting-started/database.ts (0 hunks)
• packages/dialects/sql.js/test/getting-started/person-repository.test.ts (0 hunks)
• packages/dialects/sql.js/test/getting-started/person-repository.ts (0 hunks)
• packages/dialects/sql.js/test/getting-started/types.ts (0 hunks)
• packages/dialects/sql.js/tsconfig.json (0 hunks)
• packages/dialects/sql.js/tsup.config.ts (0 hunks)
• packages/dialects/sql.js/vitest.config.ts (0 hunks)
• packages/orm/package.json (2 hunks)
• packages/orm/src/dialects/postgres.ts (1 hunks)
• packages/orm/src/dialects/sql.js/index.ts (1 hunks)
• packages/orm/src/dialects/sqlite.ts (1 hunks)
• packages/orm/tsup.config.ts (1 hunks)
• pnpm-workspace.yaml (1 hunks)

💤 Files with no reviewable changes (11)

• packages/dialects/sql.js/test/getting-started/database.ts
• packages/dialects/sql.js/README.md
• packages/dialects/sql.js/eslint.config.js
• packages/dialects/sql.js/src/index.ts
• packages/dialects/sql.js/tsup.config.ts
• packages/dialects/sql.js/tsconfig.json
• packages/dialects/sql.js/test/getting-started/person-repository.ts
• packages/dialects/sql.js/package.json
• packages/dialects/sql.js/test/getting-started/types.ts
• packages/dialects/sql.js/test/getting-started/person-repository.test.ts
• packages/dialects/sql.js/vitest.config.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

• GitHub Check: Agent
• GitHub Check: claude-review
• GitHub Check: build-test (20.x, sqlite)
• GitHub Check: build-test (20.x, postgresql)

🔇 Additional comments (9)

packages/orm/src/dialects/sql.js/index.ts (2)

1-2: Good practice: Attributing the original source.

The comment properly credits the original author and repository.

---

3-4: No issues found—referenced modules exist.

The verification confirms that both dialect.ts and types.ts files exist in the packages/orm/src/dialects/sql.js/ directory. The exports in lines 3-4 correctly reference these files.

packages/orm/tsup.config.ts (2)

8-9: LGTM: SQLite and Postgres dialect entries configured correctly.

The build entries for SQLite and Postgres dialects follow a clean pattern and align with the package.json exports.

---

10-10: Output file naming is intentional and correct.

The double .js extension (dist/dialects/sql.js.js) is intentional and matches the package.json exports. This only applies to the dialects/sql.js entry because the source directory is named sql.js (the library name), whereas other entries (dialects/sqlite, dialects/postgres) follow standard naming. The package.json exports are already configured to reference these double-extension paths, confirming the convention is intentional and consistent.

packages/orm/package.json (2)

39-58: LGTM: SQLite and Postgres dialect exports configured correctly.

The export paths for SQLite and Postgres dialects follow the standard dual-module pattern (ESM/CJS) and align with the tsup build configuration.

---

59-67: Confirm the sql.js output file naming is intentional.

The export paths reference files with double extensions (e.g., sql.js.js, sql.js.cjs). While this matches the tsup configuration on line 10 of tsup.config.ts, the naming is unconventional. Verify this is the intended output structure.

pnpm-workspace.yaml (1)

20-20: The sql.js version ^1.13.0 is the latest stable release (v1.13.0 released March 15, 2025), and the semver range specification is appropriate. No compatibility concerns identified.

packages/orm/src/dialects/postgres.ts (1)

1-1: No issues found.

Kysely v0.28.8 exports both PostgresDialect and PostgresDialectConfig as named exports from the main package, so the re-export in postgres.ts is correct and consistent with the library's documented API.

packages/orm/src/dialects/sqlite.ts (1)

1-1: Kysely exports verified.

Both SqliteDialect and SqliteDialectConfig are exported from 'kysely' in v0.28. The re-export pattern is correct and follows the documented API.

[claude]

Claude finished @ymc9's task —— View job

---

Pull Request Review

This PR consolidates the standalone @zenstackhq/kysely-sql-js package into the main ORM package under /dialects exports. Here's my comprehensive review:

Todo List

• Read and analyze changed files
• Review consolidation approach and implementation
• Check for code quality issues and best practices
• Assess potential bugs, performance, and security concerns
• Evaluate test coverage impacts
• Provide comprehensive feedback

---

✅ Overall Assessment: Good Consolidation with Minor Issues

The refactoring achieves its goal of simplifying the package structure by removing the standalone sql.js dialect package and consolidating it into the main ORM package. The approach is architecturally sound and follows good practices.

🔧 Code Quality & Best Practices

Excellent:

• Clean re-export pattern for sqlite and postgres dialects (packages/orm/src/dialects/sqlite.ts:1, packages/orm/src/dialects/postgres.ts:1)
• Proper attribution comment for ported code (packages/orm/src/dialects/sql.js/index.ts:1)
• Consistent build configuration in tsup.config.ts:8-10
• Well-structured package exports in package.json:39-67

Good:

• Maintained original implementation structure in the sql.js dialect
• Proper TypeScript interfaces and exports

⚠️ Issues Found

1. Dependency Configuration Problems

High Priority:

• @types/sql.js is in dependencies (package.json:124) but should be in peerDependencies marked as optional, matching the pattern of sql.js itself. This forces all ORM consumers to install sql.js types even if they don't use the sql.js dialect.
• zod is listed as a required peerDependency (package.json:101) but should be optional since it's only used internally and not exposed in the public API.

Recommended fixes:

"peerDependencies": {
    "better-sqlite3": "catalog:",
    "pg": "catalog:",
    "zod": "catalog:",
    "sql.js": "catalog:",
    "@types/sql.js": "^1.4.9"  // Move from dependencies
},
"peerDependenciesMeta": {
    "better-sqlite3": { "optional": true },
    "pg": { "optional": true },
    "sql.js": { "optional": true },
    "@types/sql.js": { "optional": true },  // Add this
    "zod": { "optional": true }  // Add this
}

2. Unconventional File Naming

Medium Priority:

• The sql.js exports generate files with double extensions (dist/dialects/sql.js.js, dist/dialects/sql.js.cjs) due to the directory name matching the library name. While this works, it's unconventional. Consider renaming the directory to sql-js for cleaner output files.

🚨 Security Concerns

• None identified. The dialect consolidation doesn't introduce security issues.
• The sql.js dialect maintains its "testing purposes only" warning appropriately.

🚀 Performance Considerations

Positive:

• Consolidation reduces package overhead and dependency complexity
• Re-exports from kysely are lightweight and don't duplicate functionality
• No performance regression expected

Note:

• The sql.js dialect is appropriately marked for testing only, which is correct since sql.js runs in-memory

📊 Test Coverage Impact

Significant Concern:

• The original person-repository.test.ts test file was removed during consolidation but there's no equivalent test coverage in the main ORM package
• No existing tests found in packages/orm/test/ directory
• This reduces overall test coverage for the sql.js dialect functionality

Recommendation:

• Consider adding integration tests for all dialect exports to ensure they work correctly
• The removed tests provided valuable validation of basic CRUD operations

🏗️ Architecture & Implementation

Strengths:

• Follows the existing pattern of other dialect exports
• Maintains backward compatibility through proper export paths
• Clean separation of concerns with dedicated files for types, dialect, driver, and connection
• Proper use of Kysely's SQLite adapters for sql.js (which is SQLite-compatible)

Minor Suggestions:

• The build configuration is correct but the entry point naming could be more conventional
• Consider adding JSDoc comments to the main export files for better developer experience

📋 Summary & Recommendations

Must Fix:

1. Move @types/sql.js to optional peerDependencies
2. Mark zod as optional in peerDependenciesMeta

Should Consider:

1. Add basic integration tests for dialect exports
2. Rename sql.js directory to sql-js for conventional file naming
3. Add JSDoc documentation to main export files

Nice to Have:

1. Consider adding a migration guide for users of the old @zenstackhq/kysely-sql-js package

The consolidation successfully reduces package complexity while maintaining functionality. With the dependency configuration fixes, this will be a solid improvement to the codebase architecture.