fix(zod): json type compatibility between inferred zod types and @zenstackhq/orm types#2641
Conversation
📝 WalkthroughWalkthroughThis PR fixes JSON type incompatibility between zenstack/orm and zenstack/zod by introducing a concrete ChangesJSON Type Compatibility Fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
c000930 to
bc3f177
Compare
Replace the loose union of Zod types for JSON fields with a proper recursive JsonValue type that matches the ORM's JsonValue definition, and export it. Adds cross-package type compatibility tests in the ORM package to prevent regressions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
bc3f177 to
1426e12
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/orm/package.json (1)
21-21: 💤 Low value
"pack": "pnpm pack"is redundant.
pnpm packcan be called directly; a wrapper script adds no value here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/package.json` at line 21, Remove the redundant npm script entry "pack": "pnpm pack" from package.json; locate the "scripts" object where the "pack" key is defined and delete that key/value pair so callers can run pnpm pack directly without an unnecessary wrapper.packages/zod/src/types.ts (1)
138-138: RemoveJsonValueduplication: ORM should import from@zenstackhq/zodinstead of maintaining a separate definition.ORM's
JsonValuedefinition inpackages/orm/src/common-types.tsis structurally identical to the new zod definition. However, ORM currently maintains its own separate definition rather than importing from the zod package. This creates code duplication and drift risk. Update ORM to importJsonValuefrom@zenstackhq/zodand re-export it if needed by other packages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/zod/src/types.ts` at line 138, Replace the duplicated local JsonValue type with an import from the zod package: remove the local export/type alias named JsonValue in the ORM's common-types (the existing JsonValue definition) and add an import from "@zenstackhq/zod" importing JsonValue, then re-export it if other modules rely on it (e.g., export { JsonValue } or export type { JsonValue } so callers remain unchanged); ensure any references to the old local symbol continue to use the imported JsonValue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/zod/src/types.ts`:
- Around line 138-142: The current type/runtime mismatch stems from JsonValue
excluding null while makeJsonSchema() accepts top-level null; fix it by removing
top-level z.null() from makeJsonSchema() and introduce a new helper
makeJsonItemSchema() that returns a z.ZodType allowing JsonValue | null; update
makeJsonSchema() to use z.array(z.lazy(() => this.makeJsonItemSchema())) and
z.object({}).catchall(z.lazy(() => this.makeJsonItemSchema())) so recursive
items can be null while the top-level schema (used for required Json fields)
remains non-null and matches the JsonValue / JsonZodType typing.
In `@scripts/test-generate.ts`:
- Line 25: The command string in scripts/test-generate.ts interpolates
${schemaPath} unquoted so paths with spaces will be split by the shell; update
the template to quote the schema path (e.g., change the fragment to --schema
"${schemaPath}") in the same template string that builds `${RUNTIME} ${cliPath}
generate ... ${options.join(' ')} ...` so execSync receives the schema path as a
single argument; ensure you escape any embedded quotes in schemaPath if
necessary or prefer using execFileSync to avoid shell parsing.
---
Nitpick comments:
In `@packages/orm/package.json`:
- Line 21: Remove the redundant npm script entry "pack": "pnpm pack" from
package.json; locate the "scripts" object where the "pack" key is defined and
delete that key/value pair so callers can run pnpm pack directly without an
unnecessary wrapper.
In `@packages/zod/src/types.ts`:
- Line 138: Replace the duplicated local JsonValue type with an import from the
zod package: remove the local export/type alias named JsonValue in the ORM's
common-types (the existing JsonValue definition) and add an import from
"@zenstackhq/zod" importing JsonValue, then re-export it if other modules rely
on it (e.g., export { JsonValue } or export type { JsonValue } so callers remain
unchanged); ensure any references to the old local symbol continue to use the
imported JsonValue.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5ea96f4-5966-4e18-b009-3de1d1b71859
📒 Files selected for processing (10)
packages/orm/package.jsonpackages/orm/test/schema/models.tspackages/orm/test/schema/schema.tspackages/orm/test/schema/schema.zmodelpackages/orm/test/zod-compat.test.tspackages/zod/src/index.tspackages/zod/src/types.tspackages/zod/test/factory.test.tspackages/zod/test/schema/schema.tsscripts/test-generate.ts
Fixes #2639
A small fix that copy
JsonValuefrom @zenstackhq/orm to @zenstackhq/zod.Add a test to ensure zod inferred type is compatible with $ModelReturn type from orm (except optionality).
Summary by CodeRabbit
New Features
JsonValuetype export for improved JSON type handling.Tests