feat(web_core): implement v0.9 basic catalog Zod schemas and ComponentApi#814
Conversation
…tApi - Defined strongly-typed ComponentApi definitions using Zod schemas for all v0.9 basic catalog components (Text, Button, ChoicePicker, etc.). - Reused common-types.ts schemas to ensure a single source of truth for the type system. - Exported the components as a BASIC_COMPONENTS array to be easily ingested by renderers. - Added a robust integration test basic_components.test.ts to statically verify the structural equivalence and description parity between the Zod definitions and the official JSON specification.
There was a problem hiding this comment.
Code Review
This pull request introduces Zod schemas for the v0.9 basic catalog components, along with a comprehensive integration test to validate them against the JSON specification. The implementation is well-structured and the addition of tests is a great step towards ensuring correctness. My review focuses on a few key areas: I've identified a correctness issue in the DateTimeInputApi schema that weakens validation, and a bug in the test suite's helper functions that prevents some checks from running as intended. I've also included suggestions to improve the maintainability and robustness of both the schemas and the tests.
| min: z.union([ | ||
| DynamicStringSchema, | ||
| z.string().date(), | ||
| z.string().time(), | ||
| z.string().datetime(), | ||
| ]).describe("The minimum allowed date/time in ISO 8601 format.").optional(), | ||
| max: z.union([ | ||
| DynamicStringSchema, | ||
| z.string().date(), | ||
| z.string().time(), | ||
| z.string().datetime(), | ||
| ]).describe("The maximum allowed date/time in ISO 8601 format.").optional(), |
There was a problem hiding this comment.
The current Zod schema for the min and max properties is incorrect. It uses z.union([DynamicStringSchema, ...]), where DynamicStringSchema includes a generic z.string(). This allows any string to be considered valid, bypassing the intended date, time, and datetime format checks for string literals.
To correctly enforce the format for string literals while still allowing data bindings and function calls, you should construct a union that doesn't include the generic z.string(). You will also need to import DataBindingSchema and FunctionCallSchema from ../../schema/common-types.js.
min: z.union([
z.string().datetime(),
z.string().date(),
z.string().time(),
DataBindingSchema,
FunctionCallSchema,
]).describe("The minimum allowed date/time in ISO 8601 format.").optional(),
max: z.union([
z.string().datetime(),
z.string().date(),
z.string().time(),
DataBindingSchema,
FunctionCallSchema,
]).describe("The maximum allowed date/time in ISO 8601 format.").optional(),There was a problem hiding this comment.
Here, let's follow the A2UI basic_catalog.json specification rather than trying to make it more specialized and strict in Zod. We can update the specification to be more strict later, and update Zod to match.
renderers/web_core/src/v0_9/basic_catalog/components/basic_components.test.ts
Show resolved
Hide resolved
| export const IconApi: ComponentApi = { | ||
| name: "Icon", | ||
| schema: z.object({ | ||
| ...CommonProps, | ||
| name: z.union([ | ||
| z.enum([ | ||
| "accountCircle", "add", "arrowBack", "arrowForward", "attachFile", | ||
| "calendarToday", "call", "camera", "check", "close", "delete", | ||
| "download", "edit", "event", "error", "fastForward", "favorite", | ||
| "favoriteOff", "folder", "help", "home", "info", "locationOn", | ||
| "lock", "lockOpen", "mail", "menu", "moreVert", "moreHoriz", | ||
| "notificationsOff", "notifications", "pause", "payment", "person", | ||
| "phone", "photo", "play", "print", "refresh", "rewind", "search", | ||
| "send", "settings", "share", "shoppingCart", "skipNext", | ||
| "skipPrevious", "star", "starHalf", "starOff", "stop", "upload", | ||
| "visibility", "visibilityOff", "volumeDown", "volumeMute", | ||
| "volumeOff", "volumeUp", "warning" | ||
| ]), | ||
| z.object({ | ||
| path: z.string(), | ||
| }).strict(), | ||
| ]).describe("The name of the icon to display."), | ||
| }).strict(), | ||
| }; |
There was a problem hiding this comment.
The list of icon names in IconApi is quite long and defined inline within the z.enum. To improve readability and maintainability, consider extracting this array of icon names into a named constant.
const ICON_NAMES = [
"accountCircle", "add", "arrowBack", "arrowForward", "attachFile",
"calendarToday", "call", "camera", "check", "close", "delete",
"download", "edit", "event", "error", "fastForward", "favorite",
"favoriteOff", "folder", "help", "home", "info", "locationOn",
"lock", "lockOpen", "mail", "menu", "moreVert", "moreHoriz",
"notificationsOff", "notifications", "pause", "payment", "person",
"phone", "photo", "play", "print", "refresh", "rewind", "search",
"send", "settings", "share", "shoppingCart", "skipNext",
"skipPrevious", "star", "starHalf", "starOff", "stop", "upload",
"visibility", "visibilityOff", "volumeDown", "volumeMute",
"volumeOff", "volumeUp", "warning"
] as const;
export const IconApi: ComponentApi = {
name: "Icon",
schema: z.object({
...CommonProps,
name: z.union([
z.enum(ICON_NAMES),
z.object({
path: z.string(),
}).strict(),
]).describe("The name of the icon to display."),
}).strict(),
};There was a problem hiding this comment.
A constant SGTM
| const SPEC_DIR_V0_9 = resolve( | ||
| __dirname, | ||
| "../../../../../../../specification/v0_9/json", | ||
| ); |
There was a problem hiding this comment.
The path to the specification JSON file is constructed using a long relative path (../../../../../../../). This is brittle and highly dependent on the build's output directory structure. If the location of the compiled test file changes, this path will break. Consider a more robust approach, such as assuming the test is run from the project root and using process.cwd() to construct an absolute path.
const SPEC_DIR_V0_9 = resolve(process.cwd(), "specification/v0_9/json");There was a problem hiding this comment.
Well, I don't like Gemini's solution either. Tests should be able to be run from any directory. I think this is fine.
There was a problem hiding this comment.
Yeah +1, let's not change this. The relative path is fine.
| const catalogCommonDef = officialSchema.$defs.CatalogComponentCommon; | ||
| if (catalogCommonDef && catalogCommonDef.properties && catalogCommonDef.properties.weight) { | ||
| if (catalogCommonDef.properties.weight.description) { | ||
| assert.strictEqual( | ||
| zodShape.weight.description, | ||
| catalogCommonDef.properties.weight.description, | ||
| `Description mismatch for property 'weight' of component '${componentName}'` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The test logic for verifying properties from CatalogComponentCommon is hardcoded to only check the weight property. If other common properties are added to $defs.CatalogComponentCommon in the future, the test will not validate them. To make the test more robust and future-proof, you should iterate over all properties defined in CatalogComponentCommon instead of checking for weight explicitly.
const catalogCommonDef = officialSchema.$defs.CatalogComponentCommon;
if (catalogCommonDef?.properties) {
for (const propName in catalogCommonDef.properties) {
const jsonProp = catalogCommonDef.properties[propName];
if (zodShape[propName] && jsonProp.description) {
assert.strictEqual(
zodShape[propName].description,
jsonProp.description,
`Description mismatch for common property '${propName}' of component '${componentName}'`
);
}
}
}There was a problem hiding this comment.
Yes, let's fix this.
| const SPEC_DIR_V0_9 = resolve( | ||
| __dirname, | ||
| "../../../../../../../specification/v0_9/json", | ||
| ); |
There was a problem hiding this comment.
Well, I don't like Gemini's solution either. Tests should be able to be run from any directory. I think this is fine.
Description
This PR defines strongly-typed
ComponentApidefinitions using Zod schemas for all v0.9 basic catalog components (Text, Button, ChoicePicker, etc.).common-types.tsschemas to ensure a single source of truth for the type system.BASIC_COMPONENTSarray to be easily ingested by renderers.basic_components.test.tsto statically verify the structural equivalence and description parity between the Zod definitions and the official JSON specification.This ensures all frameworks importing
web_corehave a validated, standardized set of components to work with out of the box.