Skip to content

Conversation

@ymc9
Copy link
Member

@ymc9 ymc9 commented Nov 10, 2025

Summary by CodeRabbit

  • Chores
    • Updated internal dependency configuration to optimize package management.

Copilot AI review requested due to automatic review settings November 10, 2025 04:01
@coderabbitai
Copy link

coderabbitai bot commented Nov 10, 2025

Walkthrough

The ORM package's dependency configuration was updated by migrating kysely from peerDependencies to dependencies. This change promotes kysely from an optional peer dependency to a direct, required dependency of the package.

Changes

Cohort / File(s) Summary
Dependency Management
packages/orm/package.json
Moved kysely from peerDependencies to dependencies with catalog-versioning scheme

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Verify that promoting kysely to a direct dependency aligns with the package's design and doesn't introduce conflicting version constraints elsewhere
  • Confirm this change is intentional and reflects the package's actual runtime requirements

Poem

🐰 A hop, a skip, through package.json's light,
Kysely moves closer, no longer out of sight!
From peer to direct, a bond now made true,
Dependencies strengthened—our orm feels brand new! ✨

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 PR title accurately summarizes the main change: moving 'kysely' from peerDependencies to regular dependencies in the package.json file.
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 refactor/kysely-dep

📜 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 2f17553 and 95463c1.

📒 Files selected for processing (1)
  • packages/orm/package.json (1 hunks)
⏰ 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: build-test (20.x, sqlite)
  • GitHub Check: build-test (20.x, postgresql)
  • GitHub Check: claude-review
🔇 Additional comments (1)
packages/orm/package.json (1)

70-70: Verify this breaking change is documented for consumers.

Moving kysely from peerDependencies to dependencies is a breaking change—consumers can no longer manage their own kysely version or opt out of installing it. They'll now always receive the pinned catalog version bundled with the ORM package.

Ensure this change is accompanied by:

  1. A major version bump for @zenstackhq/orm
  2. Changelog or migration documentation describing the change and rationale
  3. Any guidance for consumers who were previously managing kysely themselves

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

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

@claude
Copy link

claude bot commented Nov 10, 2025

Claude finished @ymc9's task —— View job


PR Review: Change kysely dependency from peer to regular

Todo List:

  • Examine the package.json change
  • Analyze dependency change implications
  • Review for code quality, bugs, performance, security, test coverage
  • Provide comprehensive feedback

Analysis:

This PR changes kysely from a peer dependency to a regular dependency in packages/orm/package.json. Here's my comprehensive review:

Code Quality ✅

  • Clean, focused change: Single line move from peerDependencies to dependencies
  • Consistent version management: Uses catalog: referencing the workspace catalog (~0.28.8)
  • Proper alphabetical ordering: kysely is correctly placed between json-stable-stringify and nanoid

Architecture & Design Impact 🏗️

This change aligns well with ZenStack V3's architecture:

  • V3 Design Goal: ZenStack V3 uses Kysely as its core query builder engine, not just an optional dependency
  • Dependency Justification: Since the ORM package directly builds on Kysely (as mentioned in README: "implemented the entire ORM engine using Kysely"), it makes sense to bundle it directly rather than require users to manage it separately
  • User Experience: Simplifies dependency management for consumers - they don't need to separately install kysely

Potential Issues & Considerations ⚠️

  1. Bundle Size Impact:

    • Moving from peer to regular dependency will increase the ORM package size
    • However, kysely (~0.28.8) is a relatively lean query builder, so impact should be minimal
    • Users who were already installing kysely separately won't see size increases in their final bundles
  2. Version Control Concerns:

    • Risk: Users can no longer control kysely version independently
    • Mitigation: The catalog system ensures consistent versioning across the monorepo
    • Assessment: Low risk since ZenStack is tightly coupled to specific kysely APIs
  3. Dependency Duplication:

    • If users have other packages requiring different kysely versions, npm/pnpm should handle deduplication
    • The catalog: approach helps maintain version consistency across workspace packages

Performance ✅

  • Positive: No runtime performance impact
  • Bundle optimization: Modern bundlers should tree-shake unused kysely features
  • Installation: Slightly longer install times due to additional dependency, but negligible

Security ✅

  • No security concerns: Moving an existing dependency from peer to regular doesn't introduce new attack vectors
  • Supply chain: kysely is a well-maintained, popular package with good security practices

Test Coverage Considerations 📋

  • Current change: No test modifications needed for this dependency change
  • Recommendation: Existing tests should continue to work since the kysely API remains the same
  • Future consideration: Consider adding integration tests that validate the bundled kysely version works correctly with ZenStack's ORM features

Backwards Compatibility ⚠️

  • Breaking change potential: Users who previously installed kysely separately might experience version conflicts
  • Migration path: Users should remove kysely from their own dependencies if they were managing it manually
  • Documentation update needed: This change should be documented in migration guides

Recommendation 👍

APPROVE - This is a sensible architectural improvement that:

  1. Simplifies the developer experience
  2. Aligns with V3's design where kysely is a core dependency, not optional
  3. Maintains version consistency across the monorepo
  4. Has minimal negative impacts

Suggested follow-up actions:

  1. Update documentation to reflect that kysely is now bundled
  2. Consider adding a migration note for users who manually installed kysely
  3. Test the change with downstream packages/samples to ensure no conflicts

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR moves the kysely package from peer dependencies to direct dependencies in the @zenstackhq/orm package. This change ensures that kysely is automatically installed with the ORM package rather than requiring users to install it separately.

  • Moved kysely from peerDependencies to dependencies with catalog version reference

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

@ymc9 ymc9 merged commit 860697c into dev Nov 10, 2025
11 checks passed
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.

2 participants