feat: include type in enum
#1351
Conversation
|
@paoloricciuti is attempting to deploy a commit to the Valibot Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
bot
added
size:M
There was a problem hiding this comment.
Pull Request Overview
This PR adds type field support to enum and picklist JSON schema conversions to meet Model Context Protocol (MCP) conformance requirements. The implementation infers the type ('string', 'number', or ['string', 'number']) based on the actual values in the enum/picklist options.
- Added type inference logic for
enumandpicklistschema converters - Included comprehensive test coverage for string-only, number-only, and mixed enums/picklists
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/to-json-schema/src/converters/convertSchema/convertSchema.ts | Implements type field generation for enum and picklist schemas based on option types |
| packages/to-json-schema/src/converters/convertSchema/convertSchema.test.ts | Adds test cases validating type field generation for various enum and picklist configurations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const invalid = valibotSchema.options.some( | ||
| (option) => typeof option !== 'number' && typeof option !== 'string' | ||
| ); | ||
| if (invalid) { | ||
| errors = addError( | ||
| errors, | ||
| 'An option of the "picklist" schema is not JSON compatible.' | ||
| ); | ||
| } | ||
| // @ts-expect-error | ||
| jsonSchema.enum = valibotSchema.options; | ||
| // the type could technically always be an array with only `string` or `number` | ||
| // we are specifically doing this check to match the MCP expectations that an enum | ||
| // can only contain string values AND the type is exactly "string" instead of ["string"] | ||
| if (valibotSchema.options.every((option) => typeof option === 'string')) { | ||
| jsonSchema.type = 'string'; | ||
| } else if ( | ||
| valibotSchema.options.every((option) => typeof option === 'number') | ||
| ) { | ||
| jsonSchema.type = 'number'; | ||
| } else if (!invalid) { | ||
| jsonSchema.type = ['string', 'number']; | ||
| } |
There was a problem hiding this comment.
The code performs up to three separate iterations over valibotSchema.options: one some() check at line 447 and two every() checks at lines 461 and 464. This could be optimized to a single pass through the array, especially since we're checking the same typeof conditions. Consider combining the validation and type determination into one loop.
| if (valibotSchema.options.every((option) => typeof option === 'string')) { | ||
| jsonSchema.type = 'string'; | ||
| } else if ( | ||
| valibotSchema.options.every((option) => typeof option === 'number') | ||
| ) { | ||
| jsonSchema.type = 'number'; | ||
| } else { | ||
| jsonSchema.type = ['string', 'number']; | ||
| } |
There was a problem hiding this comment.
The code performs two separate every() iterations over valibotSchema.options to determine the type. In the worst case (mixed string/number enum), it checks all elements twice. Consider optimizing to a single pass through the array.
| // the type could technically always be an array with only `string` or `number` | ||
| // we are specifically doing this check to match the MCP expectations that an enum | ||
| // can only contain string values AND the type is exactly "string" instead of ["string"] | ||
| if (valibotSchema.options.every((option) => typeof option === 'string')) { | ||
| jsonSchema.type = 'string'; | ||
| } else if ( | ||
| valibotSchema.options.every((option) => typeof option === 'number') | ||
| ) { | ||
| jsonSchema.type = 'number'; | ||
| } else { | ||
| jsonSchema.type = ['string', 'number']; | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| case 'picklist': { | ||
| if ( | ||
| valibotSchema.options.some( | ||
| (option) => typeof option !== 'number' && typeof option !== 'string' | ||
| ) | ||
| ) { | ||
| const invalid = valibotSchema.options.some( | ||
| (option) => typeof option !== 'number' && typeof option !== 'string' | ||
| ); | ||
| if (invalid) { | ||
| errors = addError( | ||
| errors, | ||
| 'An option of the "picklist" schema is not JSON compatible.' | ||
| ); | ||
| } | ||
| // @ts-expect-error | ||
| jsonSchema.enum = valibotSchema.options; | ||
| // the type could technically always be an array with only `string` or `number` | ||
| // we are specifically doing this check to match the MCP expectations that an enum | ||
| // can only contain string values AND the type is exactly "string" instead of ["string"] | ||
| if (valibotSchema.options.every((option) => typeof option === 'string')) { | ||
| jsonSchema.type = 'string'; | ||
| } else if ( | ||
| valibotSchema.options.every((option) => typeof option === 'number') | ||
| ) { | ||
| jsonSchema.type = 'number'; | ||
| } else if (!invalid) { | ||
| jsonSchema.type = ['string', 'number']; | ||
| } |
There was a problem hiding this comment.
The logic for determining the type is duplicated between the enum and picklist cases. Consider extracting this into a helper function to reduce code duplication and improve maintainability.
Example:
function determineEnumType(options: unknown[], invalid = false): string | string[] | undefined {
if (invalid) return undefined;
if (options.every((option) => typeof option === 'string')) {
return 'string';
} else if (options.every((option) => typeof option === 'number')) {
return 'number';
} else {
return ['string', 'number'];
}
}Then use it in both cases:
const enumType = determineEnumType(valibotSchema.options);
if (enumType) jsonSchema.type = enumType;1 participant
So, all of this sprouted by this PR in the conformance tests for model context protocol: modelcontextprotocol/conformance#14
The official SDK does not support Valibot, but the library I've built (https://github.com/paoloricciuti/tmcp) does, and I was getting a failing test when implementing the
everything-serverfor the conformance test using Valibot as the schema.After investigating the reason why, turns out that
@valibot/to-json-schemadoesn't include thetypeparameter for enums JSON schema, which is required to bestringfor elicitation requests. I kinda fixed in at the adapter layer in TMCP with a somewhat atrocious hack, but I think it would be interesting to actually fix this in@valibot/to-json-schemaand that's what I did here...since we bothpicklistandenumcan only accept string or numbers to be valid we can actually check what the options are and include the righttypeinto the result JSON-Schema.As I wrote in the comment, technically we could always return a type as an array and pushing stuff into it, but that would still fail the MCP validation which I think it would be a shame.
WDYT? Should we include this?