refactor: extract a runtime schema package #350
Conversation
WalkthroughThe changes extract schema utilities into a standalone Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Claude finished @ymc9's task —— View job PR Review: Extract Runtime Schema PackageThis PR extracts schema-related functionality from ✅ Positive AspectsGood Architecture Decision
Code Quality
🚨 Critical Issues1. Incomplete Import Migration (HIGH PRIORITY) Affected Packages:
Examples of broken imports: // packages/server/src/types.ts:2
import type { SchemaDef } from '@zenstackhq/orm/schema';
// Should be:
import type { SchemaDef } from '@zenstackhq/schema';2. Missing Package Dependencies
|
There was a problem hiding this comment.
Pull Request Overview
This PR extracts schema-related types and utilities from the @zenstackhq/sdk package into a new standalone package @zenstackhq/schema. This enables packages to depend on schema types without requiring the heavier SDK package, improving modularity and reducing dependency weight.
- Created new
@zenstackhq/schemapackage with schema types, expression types, and expression utilities - Updated import paths from
@zenstackhq/sdk/schemato@zenstackhq/schemaacross the codebase - Moved
AuthTypefrom a separate file to the contract types file for better organization
Reviewed Changes
Copilot reviewed 20 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/schema/package.json | New package configuration for the standalone schema package |
| packages/schema/src/schema.ts | Schema type definitions moved from SDK package |
| packages/schema/src/expression.ts | Expression type definitions extracted to new package |
| packages/schema/src/expression-utils.ts | Expression utility functions with updated import path |
| packages/schema/src/index.ts | Main export file for the schema package |
| packages/schema/tsup.config.ts | Build configuration for the schema package |
| packages/schema/tsconfig.json | TypeScript configuration for the schema package |
| packages/schema/eslint.config.js | ESLint configuration for the schema package |
| packages/sdk/package.json | Removed /schema export now handled by separate package |
| packages/sdk/tsup.config.ts | Removed schema entry point from build configuration |
| packages/orm/src/schema.ts | Re-exports schema types from new package for backward compatibility |
| packages/orm/src/schema/index.ts | Deleted - functionality moved to src/schema.ts |
| packages/orm/src/schema/auth.ts | Deleted - AuthType moved to contract.ts |
| packages/orm/src/client/contract.ts | Added AuthType definition previously in separate file |
| packages/orm/src/client/client-impl.ts | Updated import to get AuthType from contract.ts |
| packages/orm/src/client/crud/validator/utils.ts | Updated import to use new schema package |
| packages/orm/package.json | Added schema package dependency, removed language and sdk from devDependencies |
| packages/orm/tsup.config.ts | Updated schema entry point path |
| packages/testtools/package.json | Added schema package dependency |
| packages/testtools/src/schema.ts | Updated import to use new schema package |
| tests/e2e/package.json | Added schema package dependency |
| tests/e2e/orm/schemas/basic/helper.ts | Updated import to use new schema package |
| pnpm-lock.yaml | Updated lockfile reflecting new package and dependency changes |
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/schema/tsconfig.json (1)
4-4: Consider keepingnoUnusedLocalsenabled.Disabling
noUnusedLocalscan allow unused variables to accumulate, reducing code quality. Unless there's a specific reason (e.g., temporary during development), consider keeping this check enabled to maintain clean code.packages/schema/package.json (1)
18-29: Consider addingmainandtypesfields for broader compatibility.While the
exportsfield is the modern approach, adding top-levelmainandtypesfields can improve compatibility with older tooling that doesn't fully support theexportsfield.Apply this diff:
"files": [ "dist" ], + "main": "./dist/index.cjs", + "types": "./dist/index.d.ts", "exports": {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
packages/orm/package.json(1 hunks)packages/orm/src/client/client-impl.ts(1 hunks)packages/orm/src/client/contract.ts(1 hunks)packages/orm/src/client/crud/validator/utils.ts(1 hunks)packages/orm/src/schema.ts(1 hunks)packages/orm/src/schema/auth.ts(0 hunks)packages/orm/src/schema/index.ts(0 hunks)packages/orm/tsup.config.ts(1 hunks)packages/schema/eslint.config.js(1 hunks)packages/schema/package.json(1 hunks)packages/schema/src/expression-utils.ts(1 hunks)packages/schema/src/index.ts(1 hunks)packages/schema/tsconfig.json(1 hunks)packages/schema/tsup.config.ts(1 hunks)packages/sdk/package.json(0 hunks)packages/sdk/tsup.config.ts(0 hunks)packages/testtools/package.json(1 hunks)packages/testtools/src/schema.ts(1 hunks)tests/e2e/orm/schemas/basic/helper.ts(1 hunks)tests/e2e/package.json(1 hunks)
💤 Files with no reviewable changes (4)
- packages/sdk/package.json
- packages/orm/src/schema/index.ts
- packages/orm/src/schema/auth.ts
- packages/sdk/tsup.config.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T16:09:31.218Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/executor/zenstack-query-executor.ts:63-72
Timestamp: 2025-10-21T16:09:31.218Z
Learning: In ZenStack, TypeDefs can be inherited by models. When a TypeDef contains fields with `map` attributes, those mapped field names need to be processed by the QueryNameMapper since they become part of the inheriting model's schema. Therefore, when checking if a schema has mapped names (e.g., in `schemaHasMappedNames`), both `schema.models` and `schema.typeDefs` must be inspected for `@map` and `map` attributes.
Applied to files:
packages/testtools/src/schema.ts
🧬 Code graph analysis (1)
packages/orm/src/client/contract.ts (2)
packages/schema/src/schema.ts (2)
SchemaDef(10-18)GetModels(109-109)packages/orm/src/client/crud-types.ts (1)
ModelResult(135-179)
⏰ 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). (3)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: claude-review
🔇 Additional comments (15)
packages/orm/tsup.config.ts (1)
6-6: LGTM!The entry point update from
src/schema/index.tstosrc/schema.tscorrectly aligns with the new ORM schema surface that re-exports from@zenstackhq/schema.packages/orm/src/client/crud/validator/utils.ts (1)
2-10: LGTM!The import path update to
@zenstackhq/schemacorrectly reflects the new schema package structure. Type imports remain unchanged functionally.packages/schema/tsup.config.ts (1)
1-13: LGTM!The tsup configuration follows established patterns in the project with appropriate settings for building a library package (dual CJS/ESM output, type declarations, sourcemaps).
packages/orm/src/client/contract.ts (1)
806-814: LGTM!The local
AuthTypedefinition correctly centralizes the type where it's used in the client contract. The conditional type logic properly handles schema-defined auth types vs. fallback to generic records.packages/orm/src/client/client-impl.ts (1)
15-21: LGTM!The import reorganization consolidates
AuthTypewith other contract types in a single import block, aligning with the type now being defined incontract.ts. No functional changes.packages/schema/eslint.config.js (1)
1-4: LGTM!The ESLint configuration correctly re-exports the shared base config, following the established pattern for configuration sharing across packages.
tests/e2e/package.json (1)
18-18: LGTM! Schema package dependency added.The new
@zenstackhq/schemaworkspace dependency is correctly added to support the refactored schema package.packages/orm/package.json (1)
66-66: LGTM! Schema package dependency added.The ORM package correctly references the newly extracted
@zenstackhq/schemaworkspace package.packages/testtools/src/schema.ts (1)
3-3: LGTM! Import updated to new schema package.The import path is correctly updated to reference the extracted
@zenstackhq/schemapackage.tests/e2e/orm/schemas/basic/helper.ts (1)
1-1: LGTM! Import updated to new schema package.The import path correctly references the extracted
@zenstackhq/schemapackage.packages/orm/src/schema.ts (1)
1-2: LGTM! Clean re-export module.This new file provides a convenient entry point that combines the extracted schema package exports with ORM-specific types like
OperandExpressionfrom Kysely. The separation makes sense asOperandExpressionis Kysely-specific and belongs in the ORM layer.packages/testtools/package.json (1)
36-36: LGTM! Schema package dependency added.The testtools package correctly references the newly extracted
@zenstackhq/schemaworkspace package.packages/schema/src/expression-utils.ts (2)
14-14: LGTM! More explicit import path.The import path change from
'.'to'./expression'makes the dependency more explicit and improves code maintainability.
16-18: LGTM! Documentation added.The JSDoc comment clearly describes the purpose of the ExpressionUtils object.
packages/schema/src/index.ts (1)
2-2: LGTM! Expression utilities added to public API.The expression-utils module is now part of the schema package's public API surface. The removal of
OperandExpressionre-export (now in the ORM package) correctly reflects that it's Kysely-specific and belongs at the ORM layer rather than the core schema package.
Summary by CodeRabbit
Refactor
Chores