From b9d066a6c3cfac798b4cf5f116778f2d029a380f Mon Sep 17 00:00:00 2001 From: Gordon Brander Date: Mon, 17 Jun 2024 11:23:34 -0700 Subject: [PATCH 1/2] Make vnode properties liberal in what they accept Postel's Law: be conservative in what you do, be liberal in what you accept from others. Properties were previously being validated and would complain if you included a property that was not allowed. Instead, we want to validate properties, while allowing additional properties, but ignoring those additional properties when rendering. This will make us more robust to LLM hallucinations. --- typescript/package-lock.json | 1 + .../common-ui/src/components/button.ts | 11 +-- .../common-ui/src/components/datatable.ts | 7 +- .../packages/common-ui/src/components/dict.ts | 5 +- .../packages/common-ui/src/components/div.ts | 5 +- .../packages/common-ui/src/components/h1.ts | 5 +- .../packages/common-ui/src/components/p.ts | 5 +- .../packages/common-ui/src/components/span.ts | 5 +- .../packages/common-ui/src/deep-freeze.ts | 19 ++++ .../common-ui/src/hyperscript/render.ts | 88 ++++++++++++------- .../common-ui/src/hyperscript/view.ts | 48 +++++++--- 11 files changed, 126 insertions(+), 73 deletions(-) create mode 100644 typescript/packages/common-ui/src/deep-freeze.ts diff --git a/typescript/package-lock.json b/typescript/package-lock.json index 4257ee9e0..0cc49a50f 100644 --- a/typescript/package-lock.json +++ b/typescript/package-lock.json @@ -57,6 +57,7 @@ "@commontools/data": "^0.0.1", "@commontools/io": "^0.0.1", "@commontools/module": "^0.0.1", + "@commontools/usuba-api": "^0.0.1", "@commontools/usuba-rt": "^0.0.1", "@commontools/usuba-ses": "^0.0.1" }, diff --git a/typescript/packages/common-ui/src/components/button.ts b/typescript/packages/common-ui/src/components/button.ts index 9fbebad8a..86ddaf955 100644 --- a/typescript/packages/common-ui/src/components/button.ts +++ b/typescript/packages/common-ui/src/components/button.ts @@ -1,12 +1,9 @@ import { view } from "../hyperscript/render.js"; export const button = view("button", { - type: "object", - properties: { - id: { type: "string" }, - "@click": { - type: "object", - properties: { "@type": { type: "string" }, name: { type: "string" } }, - }, + id: { type: "string" }, + "@click": { + type: "object", + properties: { "@type": { type: "string" }, name: { type: "string" } }, }, }); diff --git a/typescript/packages/common-ui/src/components/datatable.ts b/typescript/packages/common-ui/src/components/datatable.ts index f591e4b02..a37d1d68d 100644 --- a/typescript/packages/common-ui/src/components/datatable.ts +++ b/typescript/packages/common-ui/src/components/datatable.ts @@ -4,11 +4,8 @@ import { repeat } from 'lit/directives/repeat.js'; import { view } from '../hyperscript/render.js'; export const datatable = view('common-datatable', { - type: 'object', - properties: { - cols: { type: 'array' }, - rows: { type: 'array' }, - } + cols: { type: 'array' }, + rows: { type: 'array' }, }); @customElement('common-datatable') diff --git a/typescript/packages/common-ui/src/components/dict.ts b/typescript/packages/common-ui/src/components/dict.ts index 51daaf4b8..02c8a12f9 100644 --- a/typescript/packages/common-ui/src/components/dict.ts +++ b/typescript/packages/common-ui/src/components/dict.ts @@ -4,10 +4,7 @@ import { repeat } from 'lit/directives/repeat.js'; import { view } from '../hyperscript/render.js'; export const dict = view('common-dict', { - type: 'object', - properties: { - records: { type: 'object' }, - } + records: { type: 'object' }, }); @customElement('common-dict') diff --git a/typescript/packages/common-ui/src/components/div.ts b/typescript/packages/common-ui/src/components/div.ts index 7fbf84176..30d23281e 100644 --- a/typescript/packages/common-ui/src/components/div.ts +++ b/typescript/packages/common-ui/src/components/div.ts @@ -1,8 +1,5 @@ import { view } from '../hyperscript/render.js'; export const div = view('div', { - type: 'object', - properties: { - id: { type: 'string' }, - } + id: { type: 'string' }, }); \ No newline at end of file diff --git a/typescript/packages/common-ui/src/components/h1.ts b/typescript/packages/common-ui/src/components/h1.ts index a0e0e35d3..f57d4c640 100644 --- a/typescript/packages/common-ui/src/components/h1.ts +++ b/typescript/packages/common-ui/src/components/h1.ts @@ -1,8 +1,5 @@ import { view } from "../hyperscript/render.js"; export const h1 = view("h1", { - type: "object", - properties: { - id: { type: "string" }, - }, + id: { type: "string" }, }); diff --git a/typescript/packages/common-ui/src/components/p.ts b/typescript/packages/common-ui/src/components/p.ts index 5a9b18863..d5edb5dcb 100644 --- a/typescript/packages/common-ui/src/components/p.ts +++ b/typescript/packages/common-ui/src/components/p.ts @@ -1,8 +1,5 @@ import { view } from "../hyperscript/render.js"; export const p = view("p", { - type: "object", - properties: { - id: { type: "string" }, - }, + id: { type: "string" }, }); diff --git a/typescript/packages/common-ui/src/components/span.ts b/typescript/packages/common-ui/src/components/span.ts index 87429b2e6..18d75fb00 100644 --- a/typescript/packages/common-ui/src/components/span.ts +++ b/typescript/packages/common-ui/src/components/span.ts @@ -1,8 +1,5 @@ import { view } from "../hyperscript/render.js"; export const span = view("span", { - type: "object", - properties: { - id: { type: "string" }, - }, + id: { type: "string" }, }); diff --git a/typescript/packages/common-ui/src/deep-freeze.ts b/typescript/packages/common-ui/src/deep-freeze.ts new file mode 100644 index 000000000..15b8ab2de --- /dev/null +++ b/typescript/packages/common-ui/src/deep-freeze.ts @@ -0,0 +1,19 @@ +/** Deep freeze an object */ +export const deepFreeze = (obj: T): T => { + // Retrieve the property names defined on object + const propNames = Reflect.ownKeys(obj); + + // Freeze properties before freezing self + for (const name of propNames) { + // @ts-ignore + const value = obj[name]; + + if ((value && typeof value === "object") || typeof value === "function") { + deepFreeze(value); + } + } + + return Object.freeze(obj); +} + +export default deepFreeze; \ No newline at end of file diff --git a/typescript/packages/common-ui/src/hyperscript/render.ts b/typescript/packages/common-ui/src/hyperscript/render.ts index 3b8b43691..4737af576 100644 --- a/typescript/packages/common-ui/src/hyperscript/render.ts +++ b/typescript/packages/common-ui/src/hyperscript/render.ts @@ -1,58 +1,58 @@ -import { Cancel, combineCancels } from '@commontools/common-frp'; -import { Signal, effect } from '@commontools/common-frp/signal'; +import { Cancel, combineCancels } from "@commontools/common-frp"; +import { Signal, effect } from "@commontools/common-frp/signal"; import { isBinding, VNode, - AnyJSONSchema, + JSONSchemaRecord, View, - view as createView -} from './view.js'; + view as createView, +} from "./view.js"; /** Registry for tags that are allowed to be rendered */ const registry = () => { const viewByTag = new Map(); + const listViews = () => Array.from(viewByTag.values()); + const getViewByTag = (tag: string) => viewByTag.get(tag); - const register = (view: View) => { + const registerView = (view: View) => { viewByTag.set(view.tag, view); - } + }; - return {getViewByTag, register}; -} + return { getViewByTag, listViews, registerView }; +}; -export const {getViewByTag, register} = registry(); +export const { getViewByTag, listViews, registerView } = registry(); /** Define and register a view factory function */ export const view = ( tagName: string, - propsSchema: AnyJSONSchema = {} + props: JSONSchemaRecord = {}, ): View => { - const factory = createView(tagName, propsSchema); - register(factory); + const factory = createView(tagName, props); + registerView(factory); return factory; -} +}; -export type RenderContext = Record> +export type RenderContext = Record>; -export const __cancel__ = Symbol('cancel'); +export const __cancel__ = Symbol("cancel"); /** Render a VNode tree, binding reactive data sources. */ -const renderVNode = ( - vnode: VNode, - context: RenderContext -): Node => { +const renderVNode = (vnode: VNode, context: RenderContext): Node => { // Make sure we have a view for this tag. If we don't it is not whitelisted. const view = getViewByTag(vnode.tag); - if (typeof view !== 'function') { + if (typeof view !== "function") { throw new TypeError(`Unknown tag: ${vnode.tag}`); } // Validate props against the view's schema. - if (!view.validateProps(vnode.props)) { + if (!view.props.validate(vnode.props)) { throw new TypeError(`Invalid props for tag: ${vnode.tag}. - Props: ${JSON.stringify(vnode.props)}`); + Props: ${JSON.stringify(vnode.props)} + Schema: ${JSON.stringify(view.props.schema)}`); } // Create the element @@ -60,11 +60,37 @@ const renderVNode = ( // Bind each prop to a reactive value (if any) and collect cancels const cancels: Array = []; + const snapshot = { ...context }; for (const [key, value] of Object.entries(vnode.props)) { + // Don't bind properties that aren't whitelisted in the schema. + if (!Object.hasOwn(view.props.schema.properties, key)) { + continue; + } + + if (key == "@click" || key == "onclick") { + if (isBinding(value)) { + console.log("onclick bind", snapshot); + const bound = snapshot[(value as any).name]; + if (!bound) continue; + + // IMPORTANT: we cannot close over a reference to a signal reference without lit-html dropping it + // so we need to extract the send function from the signal and use that directly. + const send = (bound as any).send; + console.log("onclick bind 2", bound); + element.addEventListener("click", (ev: MouseEvent) => { + const event = { type: "click", target: ev.target, button: ev.button }; + console.log("onlick", value, send, event); + send(event); + }); + } + + continue; + } + if (isBinding(value)) { const boundValue = context[value.name]; if (boundValue != null) { - const cancel = effect([boundValue], value => { + const cancel = effect([boundValue], (value) => { setProp(element, key, value); }); cancels.push(cancel); @@ -80,7 +106,7 @@ const renderVNode = ( element[__cancel__] = cancel; for (const child of vnode.children) { - if (typeof child === 'string') { + if (typeof child === "string") { element.appendChild(document.createTextNode(child)); } else { element.appendChild(render(child, context)); @@ -88,25 +114,25 @@ const renderVNode = ( } return element; -} +}; /** Render a view tree, binding reactive data sources. */ export const render = ( vnode: VNode | string | undefined | null, - context: RenderContext = {} + context: RenderContext = {}, ): Node => { if (vnode == null) { - return document.createTextNode(''); + return document.createTextNode(""); } - if (typeof vnode === 'string') { + if (typeof vnode === "string") { return document.createTextNode(vnode); } return renderVNode(vnode, context); -} +}; export default render; const setProp = (element: HTMLElement, key: string, value: any) => { // @ts-ignore element[key] = value; -} +}; diff --git a/typescript/packages/common-ui/src/hyperscript/view.ts b/typescript/packages/common-ui/src/hyperscript/view.ts index 5b3359fe8..2ecc041bb 100644 --- a/typescript/packages/common-ui/src/hyperscript/view.ts +++ b/typescript/packages/common-ui/src/hyperscript/view.ts @@ -1,7 +1,16 @@ -import * as schema from '../schema.js'; +import * as Schema from '../schema.js'; +import deepFreeze from '../deep-freeze.js'; export type AnyJSONSchema = object; +export type JSONSchemaRecord = Record; + +export type AnyJSONObjectSchema = { + type: "object"; + properties: Record; + additionalProperties?: boolean; +}; + export type Binding = { "@type": "binding"; name: string; @@ -75,7 +84,7 @@ export const VNodeSchema = { } /** Is object a VNode? */ -export const isVNode = schema.compile(VNodeSchema) +export const isVNode = Schema.compile(VNodeSchema) /** Internal helper for creating VNodes */ const vh = ( @@ -88,6 +97,15 @@ const vh = ( children }); +/** Decorate the props JSON schema object with additional JSON schema flags */ +const decoratePropsSchema = ( + properties: JSONSchemaRecord +): AnyJSONObjectSchema => ({ + type: "object", + properties, + additionalProperties: true, +}); + export type Factory = { (): VNode @@ -97,38 +115,48 @@ export type Factory = { ): VNode }; +export type PropsDescription = { + schema: AnyJSONObjectSchema; + validate: (data: any) => boolean; +} + export type View = Factory & { tag: Tag; - validateProps: (data: any) => boolean; + props: PropsDescription; } /** * Create a tag factory that validates props against a schema. * @param tagName - HTML tag name - * @param propsSchema - JSON schema for props + * @param props - the properties section of a JSON schema */ export const view = ( tagName: string, - propsSchema: AnyJSONSchema = {} + props: JSONSchemaRecord = {} ): View => { // Normalize tag name const tag = tagName.toLowerCase(); + + const schema = decoratePropsSchema(props); + // Compile props validator for fast validation at runtime. - const validateProps = schema.compile(propsSchema); + const validate = Schema.compile(schema); /** Create an element from a view, validating props */ const create = ( props: Props = {}, ...children: Array ) => { - if (!validateProps(props)) { - throw new TypeError(`Invalid props for ${tag}`); + if (!validate(props)) { + throw new TypeError(`Invalid props for ${tag}. + Props: ${JSON.stringify(props)} + Schema: ${JSON.stringify(schema)}`); } return vh(tag, props, ...children); } create.tag = tag; - create.validateProps = validateProps; + create.props = {validate, schema}; - return Object.freeze(create); + return deepFreeze(create); }; From d2bcb55708a68d8e7ee0fdc6aa1ca128579d47b0 Mon Sep 17 00:00:00 2001 From: Gordon Brander Date: Mon, 17 Jun 2024 11:33:53 -0700 Subject: [PATCH 2/2] Give JSON validator a different object - We make the validator more lenient - Also the validator decorates the object in various ways that we don't want to expose, so better to give it its own copy --- .../common-ui/src/hyperscript/view.ts | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/typescript/packages/common-ui/src/hyperscript/view.ts b/typescript/packages/common-ui/src/hyperscript/view.ts index 2ecc041bb..cea25d07a 100644 --- a/typescript/packages/common-ui/src/hyperscript/view.ts +++ b/typescript/packages/common-ui/src/hyperscript/view.ts @@ -97,15 +97,6 @@ const vh = ( children }); -/** Decorate the props JSON schema object with additional JSON schema flags */ -const decoratePropsSchema = ( - properties: JSONSchemaRecord -): AnyJSONObjectSchema => ({ - type: "object", - properties, - additionalProperties: true, -}); - export type Factory = { (): VNode @@ -123,7 +114,7 @@ export type PropsDescription = { export type View = Factory & { tag: Tag; props: PropsDescription; -} +}; /** * Create a tag factory that validates props against a schema. @@ -132,15 +123,22 @@ export type View = Factory & { */ export const view = ( tagName: string, - props: JSONSchemaRecord = {} + properties: JSONSchemaRecord = {} ): View => { // Normalize tag name const tag = tagName.toLowerCase(); - const schema = decoratePropsSchema(props); + const schema: AnyJSONObjectSchema = { + type: "object", + properties + }; // Compile props validator for fast validation at runtime. - const validate = Schema.compile(schema); + const validate = Schema.compile({ + ...schema, + // Allow additional properties when validating props. + additionalProperties: true + }); /** Create an element from a view, validating props */ const create = (