fix: Typescript schema types - move object types to Record<string, unknown>#2139
fix: Typescript schema types - move object types to Record<string, unknown>#2139KKonstantinov wants to merge 9 commits intomodelcontextprotocol:mainfrom
Conversation
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
This seems sensible to me if we're currently implicitly allowing things like functions to be assigned to objects that should be representing JSONSchemas.
To be clear though, doesn't Record<string, unknown> still allow e.g.:
{
"someKey": () => "hello world",
}i.e. still not valid JSON? Should we potentially tighten it even more (is it even possible?)
E.g. would it be worth going further, e.g. something like this?
type JSONValue = string | number | boolean | null | JSONObject | JSONArray;
type JSONObject = { [key: string]: JSONValue };
type JSONArray = JSONValue[];
Yes, it does - it only resolves the top level requirements. I agree, that going further and limiting to JSON types would be even better! EDIT: Done! |
|
@localden not super familiar with the SEP pipeline, bubbling this one up in case we need anything further to potentially get this in? E.g. do we need a core maintainer sponsor for this kind of change. |
| /** | ||
| * JSON types | ||
| */ | ||
| export type JSONValue = | ||
| | string | ||
| | number | ||
| | boolean | ||
| | null | ||
| | JSONObject | ||
| | JSONArray; | ||
| export type JSONObject = { [key: string]: JSONValue }; | ||
| export type JSONArray = JSONValue[]; |
There was a problem hiding this comment.
I think it would also be good for these to show up in the Schema Reference. We can add @category tags to enable that:
| /** | |
| * JSON types | |
| */ | |
| export type JSONValue = | |
| | string | |
| | number | |
| | boolean | |
| | null | |
| | JSONObject | |
| | JSONArray; | |
| export type JSONObject = { [key: string]: JSONValue }; | |
| export type JSONArray = JSONValue[]; | |
| /* JSON types */ | |
| /** | |
| * @category Common Types | |
| */ | |
| export type JSONValue = | |
| | string | |
| | number | |
| | boolean | |
| | null | |
| | JSONObject | |
| | JSONArray; | |
| /** | |
| * @category Common Types | |
| */ | |
| export type JSONObject = { [key: string]: JSONValue }; | |
| /** | |
| * @category Common Types | |
| */ | |
| export type JSONArray = JSONValue[]; |
4 participants
Replace
objectwithRecord<string, unknown>inschema/draft/schema.tsto accurately model JSON objects and avoid allowing non‑JSON values (functions, class instances, etc.).This tightens typing relied upon by the TypeScript SDK, which currently uses a workaround way to map its types to the
objectspecification here.Motivation and Context
objectin TypeScript means “any non‑primitive,” which includes arrays, functions, Dates, Maps, and class instances. That’s broader than a JSON object and can mask invalid payloads for a wire protocol. Further, some types which fall underobjectmight not be serializable at all (thus would break serialization and the request being sent in the first place).objectis too loose in JS/TS and weakens static guarantees;Record<string, unknown>better communicates intent and prevents accidental misuse.MetaObject = Record<string, unknown>and common JSON typing practice, improving consistency across the schema.How Has This Been Tested?
npm run generate:schemaandnpm run checkto ensure JSON schema output and docs remain consistent.Breaking Changes
objects here in reality.Types of changes
Checklist
Additional context
ClientCapabilities.experimental,ServerCapabilities.experimental), JSON Schema property maps (e.g.,Tool.inputSchema.properties,outputSchema.properties), metadata maps, and similar dictionary-shaped fields.