feat(arktype): support ToJsonSchema.Options in ArkTypeToJsonSchemaConverter#502
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
""" WalkthroughThe changes introduce configurability to the ArkType-to-JSON-schema conversion process by adding an options parameter to the converter class. Tests are updated to verify fallback handling, especially for Date types. Additionally, the package's peer dependencies are updated to require the Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Converter
participant Type
Test->>Converter: new Converter({ fallback })
Test->>Converter: convert(type)
Converter->>Type: toJsonSchema(options)
Type-->>Converter: JSON Schema
Converter-->>Test: JSON Schema
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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/arktype/package.json (1)
37-37: Consider using a more specific version range for the new peer dependency.Adding
"@ark/schema": "*"as a peer dependency correctly supports the new fallback functionality for JSON schema conversion. However, using a wildcard version (*) might lead to compatibility issues if the API changes in future versions.Consider specifying a minimum version or version range (e.g., "^1.0.0") to ensure compatibility with future releases.
packages/arktype/src/converter.test.ts (1)
27-33: Consider adding a test that explicitly demonstrates the bug fix.The new test case correctly verifies that Date types are now handled properly. To further strengthen the test suite, consider adding a test case that explicitly demonstrates how the code handles a type that would previously have thrown an error (without fallback handlers). This would document the specific issue being fixed by this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/arktype/package.json(1 hunks)packages/arktype/src/converter.test.ts(2 hunks)packages/arktype/src/converter.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/arktype/src/converter.test.ts (1)
packages/contract/src/schema.ts (1)
type(25-39)
packages/arktype/src/converter.ts (2)
packages/openapi/src/schema-converter.ts (2)
ConditionalSchemaConverter(13-15)SchemaConvertOptions(5-7)packages/contract/src/schema.ts (1)
AnySchema(7-7)
🔇 Additional comments (4)
packages/arktype/src/converter.test.ts (1)
6-11: LGTM! Good approach to testing the fallback configuration.The test setup correctly demonstrates how to use the new fallback configuration, with handlers for both default cases and specific Date types.
packages/arktype/src/converter.ts (3)
1-1: LGTM! Properly imports type from the new peer dependency.The import of
ToJsonSchemafrom '@ark/schema' aligns with the new peer dependency added to package.json.
7-11: LGTM! Clean implementation of the options parameter.The class property and constructor implementation correctly stores the optional configuration for later use. Making the parameter optional maintains backward compatibility.
18-18: LGTM! Properly passes options to toJsonSchema.The implementation correctly passes the stored options to the
toJsonSchemamethod, enabling the fallback handlers to work as intended.
|
It would be better if we had built-in support for dates as a fallback, and also supported a fallback option. |
greg-schrammel
force-pushed
the
options-ArkTypeToJsonSchemaConverter
branch
from
91bf6d9 to
0df5f82
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/arktype/src/converter.ts (1)
1-36: 💡 Verification agent🧩 Analysis chain
Verify tests are updated to cover the new functionality.
Make sure there are corresponding tests that verify:
- The converter correctly uses the default date fallback
- The converter correctly uses custom fallbacks provided via options
- The fallback precedence works as expected (user-provided fallbacks take precedence over defaults)
According to the AI summary, tests have been updated to verify fallback handling for Date types, which is good. Just confirming this recommendation.
🏁 Script executed:
#!/bin/bash # Check for test coverage of the new fallback functionality # Look for test files related to the converter echo "Looking for converter test files..." fd "converter.test" packages/arktype # Check if there are tests for the date fallback echo "Checking for date fallback tests..." rg -A 5 -B 5 "date.*fallback|fallback.*date" packages/arktypeLength of output: 301
🏁 Script executed:
#!/bin/bash # Verify if the converter tests reference the fallback logic echo "Searching for 'fallback' in converter tests..." rg -n "fallback" packages/arktype/src/converter.test.ts || echo "No matches found" # Also check for explicit date fallback assertions echo "Searching for 'date' in converter tests..." rg -n "date" packages/arktype/src/converter.test.ts || echo "No matches found"Length of output: 390
🏁 Script executed:
#!/bin/bash # Display the contents of the converter test file to verify what scenarios are covered echo "=== converter.test.ts ===" sed -n '1,200p' packages/arktype/src/converter.test.tsLength of output: 1412
Add tests for custom fallback options and precedence
The existing tests cover the default date fallback, but we need to ensure custom fallbacks work and override defaults:• In packages/arktype/src/converter.test.ts, add a test that constructs the converter with a custom
fallback.dateand asserts that
converter.convert(type('Date'), …) returns your custom schema.
• Add a test demonstrating that user‐provided fallbacks take precedence over the defaults (e.g. provide both default and custom, then confirm the custom one is used).
• (Optional) Add a test for custom fallback on a non‐date type to prove the mechanism is generic.
🧹 Nitpick comments (2)
packages/arktype/src/converter.ts (2)
17-25: Well-structured constructor with proper options merging.The constructor implementation correctly:
- Accepts an optional options parameter
- Merges user-provided options with defaults
- Ensures user-provided fallbacks take precedence over default fallbacks
Consider adding JSDoc comments to document the purpose and usage of this parameter for better developer experience.
+/** + * Creates a converter that transforms ArkType schemas to JSON schemas. + * @param _options - Configuration options for the JSON schema conversion, + * including custom fallback handlers for specific types. + */ constructor(_options?: ToJsonSchema.Options) { this.options = { ..._options, fallback: { ...defaultFallbacks, ..._options?.fallback, }, } }
6-36: Consider adding error handling for potential conversion failures.While the fallback mechanism should handle many cases, there might still be situations where toJsonSchema throws errors. Consider adding try/catch error handling in the convert method to provide more graceful failure cases or better error messages.
convert(schema: AnySchema | undefined, _options: SchemaConvertOptions): [required: boolean, jsonSchema: Exclude<JSONSchema, boolean>] { - const jsonSchema = (schema as Type).toJsonSchema(this.options) + try { + const jsonSchema = (schema as Type).toJsonSchema(this.options) + return [true, jsonSchema] + } catch (error) { + console.warn(`Failed to convert ArkType schema to JSON schema: ${error instanceof Error ? error.message : String(error)}`) + // Provide a minimal valid schema as fallback + return [true, { type: 'object' }] + } - return [true, jsonSchema] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
packages/arktype/src/converter.test.ts(1 hunks)packages/arktype/src/converter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/arktype/src/converter.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/arktype/src/converter.ts (2)
packages/openapi/src/schema-converter.ts (2)
ConditionalSchemaConverter(13-15)SchemaConvertOptions(5-7)packages/contract/src/schema.ts (1)
AnySchema(7-7)
🔇 Additional comments (4)
packages/arktype/src/converter.ts (4)
1-1: Appropriate import for new functionality.Adding the ToJsonSchema type import is necessary to support the new options parameter and type definitions.
6-12: Well-implemented default fallback for Date types.The defaultFallbacks implementation provides a sensible conversion from Date to a JSON schema with type "string" and format "date-time", which follows standard JSON Schema conventions. This addresses the issue mentioned in the PR objectives where the converter previously threw errors when processing Date types.
15-15: Good addition of options property.Adding an options property to store the configuration is appropriate and follows good OOP principles by maintaining state within the class instance.
32-32: Correctly passing options to toJsonSchema.The convert method now properly uses the stored options when calling toJsonSchema, enabling the configured fallback handlers to be used during conversion.
added a default simple date fallback, do you think we should also add a description for what the type can infer? something like this function getDateDescription({ base, before, after }) {
if (after && base.description?.startsWith("Before")) {
const _before = base.description.replace("Before ", "");
return `Between ${_before} and ${after}`;
}
if (before) return `Before ${before}`;
if (after) return `After ${(after as Date).toLocaleString()}`;
return "Anytime";
}
const defaultFallbacks: ToJsonSchema.FallbackOption = {
date: ctx => ({
...ctx.base,
type: 'string',
format: 'date-time',
description: getDateDescription(ctx),
}),
}the above snipped would automatically add a description (but won't overwrite custom descriptions) 
|
No, it is not standard, so we should avoid built-in it PR is good now, I will change a bit to make it follow best-particle and merge it in next version |
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
More templates
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/nest
@orpc/openapi
@orpc/react
@orpc/openapi-client
@orpc/react-query
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/arktype/src/converter.ts (2)
7-13: Good addition of default date fallback.The default fallback handler for dates addresses the primary issue in the PR by converting Date types to a JSON schema with type "string" and format "date-time". This follows JSON Schema best practices for date representation.
However, could you add a brief comment explaining the purpose of this default fallback for future developers?
+/** + * Default fallback handlers for common types that ArkType can't directly convert to JSON Schema. + * Dates are converted to string with date-time format by default. + */ const defaultToJsonSchemaFallback: ToJsonSchema.FallbackOption = { date: ctx => ({ ...ctx.base, type: 'string', format: JSONSchemaFormat.DateTime, }), }
33-33: Updated conversion to use configured options.The converter now correctly passes the stored options to the
toJsonSchemamethod, enabling the fallback handlers to be used during conversion.One potential improvement would be to add a type guard before the cast to ensure the schema is actually an ArkType instance.
- const jsonSchema = (schema as Type).toJsonSchema(this.#options) + if (!this.condition(schema)) { + throw new Error('Schema is not an ArkType schema') + } + const jsonSchema = (schema as Type).toJsonSchema(this.#options)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/arktype/package.json(1 hunks)packages/arktype/src/converter.test.ts(2 hunks)packages/arktype/src/converter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/arktype/package.json
- packages/arktype/src/converter.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/arktype/src/converter.ts (3)
packages/openapi/src/schema.ts (2)
JSONSchemaFormat(5-5)JSONSchema(6-6)packages/openapi/src/schema-converter.ts (2)
ConditionalSchemaConverter(13-15)SchemaConvertOptions(5-7)packages/contract/src/schema.ts (1)
AnySchema(7-7)
🔇 Additional comments (3)
packages/arktype/src/converter.ts (3)
1-1: New imports enhance the converter capabilities.The addition of
ToJsonSchemafrom@ark/schemaandJSONSchemaFormatfrom@orpc/openapialign well with your goal of adding configuration options and standardized format handling for the converter.Also applies to: 5-5
16-16: Private options field provides state for conversion configuration.This private field properly encapsulates the conversion options within the converter instance.
18-26: Well-implemented constructor with options merging.The constructor correctly merges user-provided options with defaults, ensuring that custom fallbacks take precedence over default ones while still providing the date fallback when not explicitly overridden.
The deep merge for the fallback property is particularly well-done, preserving both default and custom handlers.
dinwwwh
changed the title
2 participants
Issue:
ArkTypeToJsonSchemaConverterthrows on some types like Datesthis PR adds a
optionsarg to configure fallback json schemashttps://arktype.io/docs/configuration#tojsonschema
Summary by CodeRabbit