Skip to content

feat(zod): support defs at root level in json schema converter#1649

Merged
dinwwwh merged 5 commits into
middleapi:mainfrom
KingSora:fix/zod-root-ref-def
Jul 14, 2026
Merged

feat(zod): support defs at root level in json schema converter#1649
dinwwwh merged 5 commits into
middleapi:mainfrom
KingSora:fix/zod-root-ref-def

Conversation

@KingSora

@KingSora KingSora commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

This is a small workaround to support root level $ref and $defs in ZodToJsonSchemaConverter until colinhacks/zod#6026 gets merged eventually.

Note: the try catch block is just here as a defensive mechanism to guard against exceptions, but I couldn't encounter any case where an exception was encountered.

@pullfrog

pullfrog Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Run failed. View the logs →

Pullfrog  | Rerun failed job ➔View workflow run | via Pullfrog | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@vercel

vercel Bot commented Jul 9, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
orpc Ready Ready Preview, Comment Jul 13, 2026 1:57pm

Comment thread packages/zod/src/converter.ts Outdated
@pullfrog

pullfrog Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

ℹ️ Concur with the existing change request — one technical note inline.

Reviewed changes — adds opt-in root-level $ref / $defs emission to ZodToJsonSchemaConverter when a Zod schema carries meta({ id }).

  • Work around dropped root meta({ id }) in ZodToJsonSchemaConverter — wraps generated schemas with root $ref / $defs when schema.meta().id is present, pending upstream zod issue #6026.
  • Add test for root-level $ref / $defs — verifies z.string().meta({ id: 'root' }) emits the expected schema.

⚠️ $defs nesting breaks local $ref resolution

When toJSONSchema already emits its own $defs (for nested named schemas or custom metadata), wrapping rest as $defs: { [id]: rest } places those definitions one level too deep. A reference like #/$defs/Foo resolves against the outer root, not against $defs[id].$defs, so the schema becomes invalid.

Technical details
# `$defs` nesting breaks local refs

## Affected sites
- `packages/zod/src/converter.ts:87``$defs: { [id]: rest }` nests any pre-existing `$defs` one level too deep.

## Required outcome
- Inner `$ref` values emitted by `toJSONSchema` must continue to resolve after wrapping the root schema.
- The outer `$defs` should contain both `[id]: rest` and any definitions already present under `rest.$defs`.

## Suggested approach
Extract `rest.$defs`, keep them out of the wrapped definition, and spread them at the outer `$defs` level:

```ts
const { $schema, $defs: restDefs, ...rest } = jsonSchema
// ...
return {
  $ref: `#/$defs/${id}`,
  $defs: {
    [id]: rest,
    ...restDefs,
  },
}

Open questions for the human

  • How should a name collision between id and an existing rest.$defs key be handled?

</details>

<!--
Pullfrog review metadata — for any agent reading this review.

- Mode: Review (initial)
- Files reviewed: 2
- Commits reviewed: 2
- Base: main (49b63f93)
- Head: fix/zod-root-ref-def (8b264fb5)
- Reviewed commits:
  - 42dafbd2 — support ref & defs at root level
  - 8b264fb5 — put back comments
- Prior pullfrog review: none
- Submitted at: 2026-07-09T10:54:57Z
-->

<!-- PULLFROG_DIVIDER_DO_NOT_REMOVE_PLZ -->
<sup><a href="https://pullfrog.com"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pullfrog.com/logos/frog-white-full-18px.png"><img src="https://pullfrog.com/logos/frog-green-full-18px.png" width="9px" height="9px" style="vertical-align: middle; " alt="Pullfrog"></picture></a>&nbsp;&nbsp;| [View workflow run](https://github.com/middleapi/orpc/actions/runs/29012700262/job/86099957419) | via [Pullfrog](https://pullfrog.com) | Using `Kimi K2` (free via [Pullfrog for OSS](https://pullfrog.com/for-oss)) | [𝕏](https://x.com/pullfrogai)</sup>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes – the follow-up commit addressed the $defs-hoisting concern from the prior review and expanded the happy-path test to cover nested named schemas.

  • packages/zod/src/converter.ts now reads the schema id from this.options.metadata ?? globalRegistry, spreads any existing $defs onto the outer $defs, and avoids overwriting an existing entry with !(id in $defs).
  • packages/zod/src/converter.test.ts exercises the nested case: an object schema with named a / b properties and a root id produces a flat $defs map with correct local $ref pointers.

Pullfrog  | View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@pkg-pr-new

pkg-pr-new Bot commented Jul 9, 2026

Copy link
Copy Markdown
More templates

@orpc/arktype

npm i https://pkg.pr.new/@orpc/arktype@1649

@orpc/bun

npm i https://pkg.pr.new/@orpc/bun@1649

@orpc/client

npm i https://pkg.pr.new/@orpc/client@1649

@orpc/cloudflare

npm i https://pkg.pr.new/@orpc/cloudflare@1649

@orpc/contract

npm i https://pkg.pr.new/@orpc/contract@1649

@orpc/experimental-effect

npm i https://pkg.pr.new/@orpc/experimental-effect@1649

@orpc/evlog

npm i https://pkg.pr.new/@orpc/evlog@1649

@orpc/json-schema

npm i https://pkg.pr.new/@orpc/json-schema@1649

@orpc/nest

npm i https://pkg.pr.new/@orpc/nest@1649

@orpc/next

npm i https://pkg.pr.new/@orpc/next@1649

@orpc/openapi

npm i https://pkg.pr.new/@orpc/openapi@1649

@orpc/opentelemetry

npm i https://pkg.pr.new/@orpc/opentelemetry@1649

@orpc/pino

npm i https://pkg.pr.new/@orpc/pino@1649

@orpc/publisher

npm i https://pkg.pr.new/@orpc/publisher@1649

@orpc/ratelimit

npm i https://pkg.pr.new/@orpc/ratelimit@1649

@orpc/server

npm i https://pkg.pr.new/@orpc/server@1649

@orpc/shared

npm i https://pkg.pr.new/@orpc/shared@1649

@orpc/tanstack-query

npm i https://pkg.pr.new/@orpc/tanstack-query@1649

@orpc/valibot

npm i https://pkg.pr.new/@orpc/valibot@1649

@orpc/zod

npm i https://pkg.pr.new/@orpc/zod@1649

commit: 69807ef

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@dinwwwh dinwwwh changed the title feat: ZodToJsonSchemaConverter support ref & defs at root level feat(zod): supprt defs at root level in json schema converter Jul 13, 2026
@dinwwwh

dinwwwh commented Jul 13, 2026

Copy link
Copy Markdown
Member

@KingSora Just update logic and tests, if y have time please review it again!

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — minor suggestion inline.

Reviewed changes — the latest improve commit tightened id-collision behavior, removed the defensive try/catch, and expanded test coverage for the root $ref/$defs workaround.

  • packages/zod/src/converter.ts no longer swallows exceptions in the root workaround; instead, when the schema id collides with an existing $defs entry, the root definition is renamed to <id>__<index>.
  • packages/zod/src/converter.test.ts now module-mocks zod/v4/core to stub toJSONSchema and splits the single root test into three cases covering the global registry, a custom metadata registry, and id-collision handling.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

Comment thread packages/zod/src/converter.ts
@KingSora

KingSora commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

@dinwwwh Cool :) Thats looks nice, I only have one note: I've been also thinking about implementing the re-naming strategy for colliding ids but I didn't do it because zods toJSONSchema function also doesn't do it. It throws an error if it encounters an id collision.

Edit: You can look it up here: https://github.com/colinhacks/zod/blob/912f0f51b0ced654d0069741e7160834dca742ee/packages/zod/src/v4/core/to-json-schema.ts#L227-L240

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found for this delta.

Reviewed changes — the latest commit adds JSON Pointer escaping for root definition ids, so ids containing ~ or / produce valid $ref pointers while keeping the raw id as the $defs key.

  • packages/zod/src/converter.ts now encodes the root definition name with encodeJsonPointerSegment before interpolating it into the $ref pointer.
  • packages/zod/src/converter.test.ts exercises the escaping path with an id of 'root~/', expecting #/$defs/root~0~1.

Pullfrog  | Fix it ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@dinwwwh dinwwwh changed the title feat(zod): supprt defs at root level in json schema converter feat(zod): support defs at root level in json schema converter Jul 14, 2026
@dinwwwh dinwwwh merged commit f7e65e9 into middleapi:main Jul 14, 2026
9 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