From a308c91cdc45af32327423f658d73084a21f9f3f Mon Sep 17 00:00:00 2001 From: Will Eastcott Date: Sun, 16 Aug 2026 12:01:53 +0100 Subject: [PATCH 1/2] Declare attributes as data: property tables, piloted on pc-light Part of #391. Every attribute is currently declared in five places (field default, getInitialComponentData, accessors, observedAttributes, a switch case restating the default), and nothing ties them together - default drift across 23 attributes is the library's known bug family. This adds the machinery and migrates the pilot element: - src/properties.ts: a static per-class `properties` table maps property names to parse helpers; `observedAttributes` derives from the table and a generic `applyAttribute` dispatch replaces the per-element switch. Defaults are stated once, in the field initializer: reactions never run mid-constructor, so a clone-aware snapshot of the properties at the first attributeChangedCallback captures exactly the initializer values for removal and invalid-value fallbacks. `enumOf` carries an enum's valid names once, for dispatch and for the manifest. - ComponentElement hosts the generic dispatch; unmigrated subclasses chain super exactly as before. - pc-light (bool, number, color, and both enum shapes) drops its observedAttributes list and 19-case switch for a 19-line table. - attributes-plugin.mjs reads the table where one exists (type from the parse helper identity, enum values from the enumOf argument, defaults from the field initializers); switch parsing remains for unmigrated elements and retires with the last migration. dist/custom-elements.json, vscode.html-custom-data.json and web-types.json are byte-identical to main; validate.mjs assertions are untouched and green. The only .d.ts change is pc-light losing its two now-inherited lifecycle redeclarations (identical signatures on ComponentElement). New element-tier tests pin the table semantics. Co-Authored-By: Claude Fable 5 --- custom-elements-manifest.config.mjs | 2 +- src/components/component.ts | 19 ++- src/components/light-component.ts | 115 ++++-------------- src/properties.ts | 171 +++++++++++++++++++++++++++ test/elements/light.test.ts | 176 ++++++++++++++++++++++++++++ utils/cem/attributes-plugin.mjs | 167 +++++++++++++++++++++----- 6 files changed, 525 insertions(+), 125 deletions(-) create mode 100644 src/properties.ts create mode 100644 test/elements/light.test.ts diff --git a/custom-elements-manifest.config.mjs b/custom-elements-manifest.config.mjs index 470b791a..f0351e37 100644 --- a/custom-elements-manifest.config.mjs +++ b/custom-elements-manifest.config.mjs @@ -10,7 +10,7 @@ export default { // None of these files declares an element, so excluding them keeps the manifest to the public // element surface. The base classes in async-element.ts and components/component.ts are // deliberately included - the elements that extend them inherit their attributes and events. - exclude: ['src/colors.ts', 'src/loading-bar.ts', 'src/parse.ts'], + exclude: ['src/colors.ts', 'src/loading-bar.ts', 'src/parse.ts', 'src/properties.ts'], // dist is wiped by `prebuild` and shipped via the `files` and `exports` fields, so the // generated files can never go stale and never need committing diff --git a/src/components/component.ts b/src/components/component.ts index 3ed0104b..1e7e3c7e 100644 --- a/src/components/component.ts +++ b/src/components/component.ts @@ -4,6 +4,8 @@ import type { AppElement } from '../app'; import { AsyncElement } from '../async-element'; import type { EntityBaseElement } from '../entity-base'; import { parseBool } from '../parse'; +import type { PropertyTable } from '../properties'; +import { applyAttribute, attributeNames } from '../properties'; /** * Represents a component in the PlayCanvas engine. @@ -241,16 +243,21 @@ class ComponentElement extends AsyncElement { return this._enabled; } + /** + * The attribute schema shared by every component element. A subclass declares its own + * attributes by spreading this table into its own (see `src/properties.ts`). + * @internal + */ + static properties: PropertyTable = { + enabled: { parse: parseBool } + }; + static get observedAttributes() { - return ['enabled']; + return attributeNames(this.properties); } attributeChangedCallback(name: string, _oldValue: string | null, newValue: string | null) { - switch (name) { - case 'enabled': - this.enabled = parseBool(newValue, true); - break; - } + applyAttribute(this, name, newValue); } } diff --git a/src/components/light-component.ts b/src/components/light-component.ts index bf4ce824..5ca7ca56 100644 --- a/src/components/light-component.ts +++ b/src/components/light-component.ts @@ -12,7 +12,9 @@ import { SHADOW_VSM_32F } from 'playcanvas'; -import { parseBool, parseColor, parseEnum, parseNumber } from '../parse'; +import { parseBool, parseColor, parseNumber } from '../parse'; +import type { PropertyTable } from '../properties'; +import { enumOf } from '../properties'; import { ComponentElement } from './component'; @@ -507,94 +509,29 @@ class LightComponentElement extends ComponentElement { return this._shadowBlockerSamples; } - static get observedAttributes() { - return [ - ...super.observedAttributes, - 'color', - 'cast-shadows', - 'intensity', - 'inner-cone-angle', - 'normal-offset-bias', - 'outer-cone-angle', - 'penumbra-falloff', - 'penumbra-size', - 'range', - 'shadow-bias', - 'shadow-blocker-samples', - 'shadow-distance', - 'shadow-intensity', - 'shadow-resolution', - 'shadow-samples', - 'shadow-type', - 'type', - 'vsm-bias', - 'vsm-blur-size' - ]; - } - - attributeChangedCallback(name: string, _oldValue: string | null, newValue: string | null) { - super.attributeChangedCallback(name, _oldValue, newValue); - - switch (name) { - case 'color': - this.color = parseColor(newValue, Color.WHITE, name); - break; - case 'cast-shadows': - this.castShadows = parseBool(newValue, false); - break; - case 'inner-cone-angle': - this.innerConeAngle = parseNumber(newValue, 40, name); - break; - case 'intensity': - this.intensity = parseNumber(newValue, 1, name); - break; - case 'normal-offset-bias': - this.normalOffsetBias = parseNumber(newValue, 0.05, name); - break; - case 'outer-cone-angle': - this.outerConeAngle = parseNumber(newValue, 45, name); - break; - case 'penumbra-falloff': - this.penumbraFalloff = parseNumber(newValue, 1, name); - break; - case 'penumbra-size': - this.penumbraSize = parseNumber(newValue, 1, name); - break; - case 'range': - this.range = parseNumber(newValue, 10, name); - break; - case 'shadow-bias': - this.shadowBias = parseNumber(newValue, 0.2, name); - break; - case 'shadow-distance': - this.shadowDistance = parseNumber(newValue, 16, name); - break; - case 'shadow-blocker-samples': - this.shadowBlockerSamples = parseNumber(newValue, 16, name); - break; - case 'shadow-resolution': - this.shadowResolution = parseNumber(newValue, 1024, name); - break; - case 'shadow-intensity': - this.shadowIntensity = parseNumber(newValue, 1, name); - break; - case 'shadow-samples': - this.shadowSamples = parseNumber(newValue, 16, name); - break; - case 'shadow-type': - this.shadowType = parseEnum(newValue, shadowTypes, 'pcf3-32f', name); - break; - case 'type': - this.type = parseEnum(newValue, ['directional', 'omni', 'spot'], 'directional', name); - break; - case 'vsm-bias': - this.vsmBias = parseNumber(newValue, 0.01, name); - break; - case 'vsm-blur-size': - this.vsmBlurSize = parseNumber(newValue, 11, name); - break; - } - } + /** @internal */ + static properties: PropertyTable = { + ...ComponentElement.properties, + castShadows: { parse: parseBool }, + color: { parse: parseColor }, + innerConeAngle: { parse: parseNumber }, + intensity: { parse: parseNumber }, + normalOffsetBias: { parse: parseNumber }, + outerConeAngle: { parse: parseNumber }, + penumbraFalloff: { parse: parseNumber }, + penumbraSize: { parse: parseNumber }, + range: { parse: parseNumber }, + shadowBias: { parse: parseNumber }, + shadowBlockerSamples: { parse: parseNumber }, + shadowDistance: { parse: parseNumber }, + shadowIntensity: { parse: parseNumber }, + shadowResolution: { parse: parseNumber }, + shadowSamples: { parse: parseNumber }, + shadowType: { parse: enumOf(shadowTypes) }, + type: { parse: enumOf(['directional', 'omni', 'spot']) }, + vsmBias: { parse: parseNumber }, + vsmBlurSize: { parse: parseNumber } + }; } customElements.define('pc-light', LightComponentElement); diff --git a/src/properties.ts b/src/properties.ts new file mode 100644 index 00000000..06440228 --- /dev/null +++ b/src/properties.ts @@ -0,0 +1,171 @@ +/** + * The property-table machinery behind `attributeChangedCallback`. An element class declares a + * static `properties` table mapping each property to how its attribute parses; the base classes + * derive `observedAttributes` from the table and route every attribute change through + * {@link applyAttribute}. One table entry replaces what used to be restated per attribute: the + * entry in `observedAttributes` and the `case` in the dispatch switch. + * + * Defaults are deliberately absent from the table. Custom element reactions never run + * mid-constructor, so by the first `attributeChangedCallback` every field initializer has run; + * snapshotting the element's properties then captures exactly the initializer values, and an + * absent, removed or invalid attribute falls back to that snapshot. A default therefore lives in + * one place only — the backing field's initializer — and the manifest tooling reads it from + * there rather than from a restated literal. + */ + +import { Color, Quat, Vec2, Vec3, Vec4 } from 'playcanvas'; + +import { parseEnum } from './parse'; + +/** + * Converts an attribute value into the value assigned to the element property. The parse helpers + * in `parse.ts` all have this shape; `defaultValue` is `any` so their narrower per-type + * signatures remain assignable. + * + * @param value - The attribute value (`null` when the attribute is absent or removed). + * @param defaultValue - The value to fall back to, from the element's defaults snapshot. + * @param attribute - The attribute name, used in warning messages. + * @returns The value to assign to the property. + * @internal + */ +export type AttributeParser = (value: string | null, defaultValue: any, attribute: string) => unknown; + +/** + * One property's entry in a {@link PropertyTable}. + * @internal + */ +export type PropertyDefinition = { + /** + * The attribute name, when it is not simply the kebab-cased property name (an alias, or an + * initialism the mechanical conversion would mangle). + */ + attribute?: string; + + /** The parse helper converting the attribute value for this property. */ + parse: AttributeParser; +}; + +/** + * An element class's attribute schema, keyed by property name. A subclass extends its base + * class's schema by spreading it (`{ ...ComponentElement.properties, intensity: ... }`), so the + * table on any class always describes that element's full attribute surface. + * @internal + */ +export type PropertyTable = Record; + +/** + * The attribute name observed for a property: an explicit `attribute` override, or the + * kebab-cased property name. + * + * @param property - The property name. + * @param definition - The property's table entry. + * @returns The attribute name. + */ +const attributeOf = (property: string, definition: PropertyDefinition): string => { + return definition.attribute ?? property.replace(/[A-Z]/g, (char) => `-${char.toLowerCase()}`); +}; + +/** + * Reverse indexes (attribute name → property and definition), built once per table. Tables are + * class statics, so this caches per class, not per element. + */ +const indexes = new WeakMap>(); + +const indexOf = (table: PropertyTable) => { + let index = indexes.get(table); + if (!index) { + index = new Map(); + for (const [property, definition] of Object.entries(table)) { + index.set(attributeOf(property, definition), { property, definition }); + } + indexes.set(table, index); + } + return index; +}; + +/** + * The attribute names a table observes, for `static get observedAttributes()`. + * + * @param table - The property table. + * @returns The attribute names. + * @internal + */ +export const attributeNames = (table: PropertyTable): string[] => { + return [...indexOf(table).keys()]; +}; + +/** + * Clones a snapshot value, so the recorded default can never alias a live property value that a + * later parse result (or an in-place mutation) would write through. Math types are the mutable + * ones the parse helpers produce; arrays cover `parseTags`. + * + * @param value - The property value to record. + * @returns The value to store in the snapshot. + */ +const cloneValue = (value: unknown): unknown => { + if ( + value instanceof Color || + value instanceof Quat || + value instanceof Vec2 || + value instanceof Vec3 || + value instanceof Vec4 + ) { + return value.clone(); + } + return Array.isArray(value) ? [...value] : value; +}; + +/** Per-element snapshots of every table property's pre-attribute value. */ +const defaults = new WeakMap>(); + +const snapshotDefaults = (element: HTMLElement, table: PropertyTable): Record => { + const snapshot: Record = {}; + for (const property of Object.keys(table)) { + snapshot[property] = cloneValue((element as unknown as Record)[property]); + } + return snapshot; +}; + +/** + * Generic `attributeChangedCallback` dispatch: resolves the changed attribute against the + * element class's property table and assigns the parsed value through the property's accessor. + * An attribute the table does not know is ignored, so an element handling extra, non-property + * attributes overrides `attributeChangedCallback` and chains `super` exactly as before. + * + * @param element - The element whose attribute changed. + * @param name - The attribute name. + * @param value - The new attribute value (`null` when the attribute was removed). + * @internal + */ +export const applyAttribute = (element: HTMLElement, name: string, value: string | null): void => { + const table = (element.constructor as unknown as { properties: PropertyTable }).properties; + const match = indexOf(table).get(name); + if (!match) { + return; + } + + let snapshot = defaults.get(element); + if (!snapshot) { + snapshot = snapshotDefaults(element, table); + defaults.set(element, snapshot); + } + + (element as unknown as Record)[match.property] = match.definition.parse( + value, + snapshot[match.property], + name + ); +}; + +/** + * Binds `parseEnum` to its set of valid names. The set is carried once, here: dispatch resolves + * values against it, and the manifest tooling reads the published enum values from this call's + * argument. + * + * @param valid - The valid names: an array, or a map whose keys are the valid names. + * @returns The bound parser. + * @internal + */ +export const enumOf = (valid: readonly T[] | ReadonlyMap): AttributeParser => { + return (value, defaultValue, attribute) => parseEnum(value, valid, defaultValue, attribute); +}; diff --git a/test/elements/light.test.ts b/test/elements/light.test.ts new file mode 100644 index 00000000..ead7dc11 --- /dev/null +++ b/test/elements/light.test.ts @@ -0,0 +1,176 @@ +import { Color } from 'playcanvas'; +import { describe, expect, it } from 'vitest'; + +import { LightComponentElement } from '../../src/components/light-component'; +import { useGuard } from '../helpers/guard'; + +/** + * is the pilot of the static property table (src/properties.ts), so beyond the + * element's own attribute surface these tests pin the table machinery itself: observedAttributes + * derived from the table, dispatch through the shared attributeChangedCallback, and defaults + * restored from the first-callback snapshot of the field initializers - for removal, and for the + * fallback (and warning text) of an invalid value. The element caches every property to a private + * field and only writes through once its engine component exists, so all of it is observable with + * no in play. + */ +describe('', () => { + const { warnings } = useGuard(); + + const create = () => document.createElement('pc-light') as LightComponentElement; + + it('observes every attribute of the property table', () => { + expect([...LightComponentElement.observedAttributes].sort()).toEqual([ + 'cast-shadows', + 'color', + 'enabled', + 'inner-cone-angle', + 'intensity', + 'normal-offset-bias', + 'outer-cone-angle', + 'penumbra-falloff', + 'penumbra-size', + 'range', + 'shadow-bias', + 'shadow-blocker-samples', + 'shadow-distance', + 'shadow-intensity', + 'shadow-resolution', + 'shadow-samples', + 'shadow-type', + 'type', + 'vsm-bias', + 'vsm-blur-size' + ]); + }); + + it('parses a number attribute and restores the default on removal', () => { + const element = create(); + + expect(element.intensity).toBe(1); + + element.setAttribute('intensity', '2.5'); + expect(element.intensity).toBe(2.5); + + element.removeAttribute('intensity'); + expect(element.intensity).toBe(1); + }); + + it('falls back to the field default on an invalid number, naming it in the warning', () => { + const element = create(); + + element.setAttribute('shadow-bias', '0.5'); + expect(element.shadowBias).toBe(0.5); + + element.setAttribute('shadow-bias', 'steep'); + expect(element.shadowBias, 'an invalid value falls back to the default, not the previous value').toBe(0.2); + warnings.expect("Invalid value 'steep' for attribute 'shadow-bias'. Expected a finite number. Using '0.2'."); + }); + + it('parses a boolean attribute with the standard rules', () => { + const element = create(); + + expect(element.castShadows).toBe(false); + + // A bare boolean attribute (e.g. ) arrives as the empty string + element.setAttribute('cast-shadows', ''); + expect(element.castShadows).toBe(true); + + element.setAttribute('cast-shadows', 'false'); + expect(element.castShadows).toBe(false); + + element.setAttribute('cast-shadows', 'true'); + expect(element.castShadows).toBe(true); + + element.removeAttribute('cast-shadows'); + expect(element.castShadows).toBe(false); + }); + + it('parses a color attribute and restores the default on removal', () => { + const element = create(); + + expect(element.color).toEqual(new Color(1, 1, 1)); + + element.setAttribute('color', 'red'); + expect(element.color).toEqual(new Color(1, 0, 0)); + + element.setAttribute('color', 'not-a-color'); + expect(element.color).toEqual(new Color(1, 1, 1)); + warnings.expect(/Invalid value 'not-a-color' for attribute 'color'/); + + element.removeAttribute('color'); + expect(element.color).toEqual(new Color(1, 1, 1)); + }); + + it('never hands out the recorded default itself', () => { + const element = create(); + + element.setAttribute('color', 'red'); + element.removeAttribute('color'); + + // Mutate the restored value in place; the snapshot must not be written through + element.color.r = 0.25; + + element.setAttribute('color', 'blue'); + element.removeAttribute('color'); + expect(element.color).toEqual(new Color(1, 1, 1)); + }); + + it('resolves an enum attribute against a constant map', () => { + const element = create(); + + expect(element.shadowType).toBe('pcf3-32f'); + + element.setAttribute('shadow-type', 'vsm-16f'); + expect(element.shadowType).toBe('vsm-16f'); + + // The fallback is the field default, not the previous value + element.setAttribute('shadow-type', 'soft'); + expect(element.shadowType).toBe('pcf3-32f'); + warnings.expect( + "Invalid value 'soft' for attribute 'shadow-type'. Valid values: pcf1-16f, pcf1-32f, " + + "pcf3-16f, pcf3-32f, pcf5-16f, pcf5-32f, vsm-16f, vsm-32f, pcss-32f. Using 'pcf3-32f'." + ); + + element.setAttribute('shadow-type', 'pcss-32f'); + element.removeAttribute('shadow-type'); + expect(element.shadowType).toBe('pcf3-32f'); + }); + + it('resolves an enum attribute against an inline array', () => { + const element = create(); + + expect(element.type).toBe('directional'); + + element.setAttribute('type', 'spot'); + expect(element.type).toBe('spot'); + + element.setAttribute('type', 'point'); + expect(element.type).toBe('directional'); + warnings.expect( + "Invalid value 'point' for attribute 'type'. Valid values: directional, omni, spot. Using 'directional'." + ); + + element.removeAttribute('type'); + expect(element.type).toBe('directional'); + }); + + it('handles the enabled attribute inherited from ComponentElement', () => { + const element = create(); + + expect(element.enabled).toBe(true); + + element.setAttribute('enabled', 'false'); + expect(element.enabled).toBe(false); + + element.removeAttribute('enabled'); + expect(element.enabled).toBe(true); + }); + + it('leaves the accessors as the programmatic surface', () => { + const element = create(); + + element.intensity = 7; + expect(element.intensity).toBe(7); + expect(element.hasAttribute('intensity'), 'properties do not reflect back to attributes').toBe(false); + }); +}); diff --git a/utils/cem/attributes-plugin.mjs b/utils/cem/attributes-plugin.mjs index ed92b991..d26ac28f 100644 --- a/utils/cem/attributes-plugin.mjs +++ b/utils/cem/attributes-plugin.mjs @@ -1,21 +1,27 @@ /** - * Custom Elements Manifest plugin that derives attribute metadata from each element's - * `attributeChangedCallback`. + * Custom Elements Manifest plugin that derives attribute metadata from each element's attribute + * declarations, from either of two sources: * - * Every element in this library declares its attributes in `static get observedAttributes()` and - * handles them in an `attributeChangedCallback` whose body is a uniform mapping of the form: + * - A static `properties` table (see `src/properties.ts`), where an entry of the form + * `shadowType: { parse: enumOf(shadowTypes) }` carries the property (`fieldName`), the + * attribute name (kebab-cased, unless overridden by `attribute`), the attribute's type + * (implied by the parse helper) and its enum values (the `enumOf` argument). The default is + * read from the backing field's initializer (`private _shadowType = 'pcf3-32f'`), which the + * table pattern makes the single statement of each default. * - * ```js - * case 'clear-color': - * this.clearColor = parseColor(newValue, new Color(0.75, 0.75, 0.75, 1), name); - * break; - * ``` + * - Transitionally, an `attributeChangedCallback` whose body is a uniform mapping of the form: * - * That single expression carries everything the manifest needs: the attribute name, the property - * it writes to (`fieldName`), the attribute's type (implied by the `parse*` helper) and its - * default value (the helper's second argument). Deriving all of it here keeps the manifest in - * lockstep with the code, rather than requiring ~200 hand-written `@attribute` tags that would - * silently drift. + * ```js + * case 'clear-color': + * this.clearColor = parseColor(newValue, new Color(0.75, 0.75, 0.75, 1), name); + * break; + * ``` + * + * which carries the same metadata, with the default restated as the helper's second argument. + * This path retires once the last element migrates to a `properties` table. + * + * Deriving all of it here keeps the manifest in lockstep with the code, rather than requiring + * ~200 hand-written `@attribute` tags that would silently drift. */ /** @@ -69,6 +75,8 @@ const EMPTY_CONSTRUCTORS = { const kebabToCamel = name => name.replace(/-([a-z])/g, (_, char) => char.toUpperCase()); +const camelToKebab = name => name.replace(/[A-Z]/g, char => `-${char.toLowerCase()}`); + /** * Extracts `name === 'some-attribute'` comparisons, so that elements handling a single attribute * with an `if` rather than a `switch` (see `src/asset.ts`) are covered too. @@ -344,6 +352,92 @@ const describeValue = (ts, sourceFile, value, context) => { }; }; +/** + * Collects the entries of a class's static `properties` table (see `src/properties.ts`). Spread + * entries (`...ComponentElement.properties`) restate a base class's table for the runtime merge; + * they are skipped here because the analyzer's inheritance step already copies the base class's + * attributes, marked with `inheritedFrom`. + * + * @param {import('typescript')} ts - The TypeScript module supplied by the analyzer. + * @param {import('typescript').ClassDeclaration} node - The class declaration. + * @returns {{ fieldName: string, attribute: string, parse?: import('typescript').Expression }[]} The entries. + */ +const collectTableEntries = (ts, node) => { + const table = node.members.find(member => ts.isPropertyDeclaration(member) && + member.name.getText() === 'properties' && + member.modifiers?.some(modifier => modifier.kind === ts.SyntaxKind.StaticKeyword)); + if (!table?.initializer || !ts.isObjectLiteralExpression(table.initializer)) { + return []; + } + + const entries = []; + for (const property of table.initializer.properties) { + if (!ts.isPropertyAssignment(property) || !ts.isObjectLiteralExpression(property.initializer)) { + continue; + } + const fieldName = ts.isStringLiteralLike(property.name) ? property.name.text : property.name.getText(); + + let attribute = camelToKebab(fieldName); + let parse; + for (const option of property.initializer.properties) { + if (!ts.isPropertyAssignment(option)) { + continue; + } + const name = option.name.getText(); + if (name === 'attribute' && ts.isStringLiteralLike(option.initializer)) { + attribute = option.initializer.text; + } else if (name === 'parse') { + parse = option.initializer; + } + } + entries.push({ fieldName, attribute, parse }); + } + return entries; +}; + +/** + * Derives an attribute's type from a table entry's `parse` expression: a parse helper by + * identity, or an `enumOf(...)` call whose argument carries the valid names. + * + * @param {import('typescript')} ts - The TypeScript module supplied by the analyzer. + * @param {import('typescript').SourceFile} sourceFile - The file declaring the element. + * @param {import('typescript').Expression} [parse] - The entry's `parse` expression. + * @param {string} context - A label used in warnings. + * @returns {{ type: string, format?: string }} The derived metadata. + */ +const describeParse = (ts, sourceFile, parse, context) => { + if (parse && ts.isCallExpression(parse) && parse.expression.getText() === 'enumOf') { + const values = resolveEnumValues(ts, sourceFile, parse.arguments[0]); + if (values.length === 0) { + console.warn(`[cem] could not resolve enum values for ${context}; falling back to string`); + return { type: 'string' }; + } + return { type: values.map(name => `'${name}'`).join(' | ') }; + } + + const helper = parse && ts.isIdentifier(parse) ? PARSE_HELPERS[parse.text] : undefined; + if (!helper || helper.type === 'enum') { + return { type: 'string' }; + } + return { type: helper.type, format: helper.format }; +}; + +/** + * Renders the default of a table-declared property from its backing field's initializer — the + * `_shadowType` of `shadowType`. A property with no backing field (or no initializer) has no + * default to publish. + * + * @param {import('typescript')} ts - The TypeScript module supplied by the analyzer. + * @param {import('typescript').ClassDeclaration} node - The class declaration. + * @param {string} fieldName - The property name. + * @returns {string | undefined} The rendered default. + */ +const renderFieldDefault = (ts, node, fieldName) => { + const field = node.members.find(member => ts.isPropertyDeclaration(member) && + member.name.getText() === `_${fieldName}`); + return renderDefault(ts, field?.initializer); +}; + /** * Reduces a member's JSDoc to a single tooltip-friendly sentence, rewriting the accessor voice * ("Sets the field of view of the camera.") into the declarative voice an attribute description @@ -384,26 +478,14 @@ export const attributesFromCallbackPlugin = () => ({ return; } - const callback = node.members.find(member => ts.isMethodDeclaration(member) && - member.name.getText() === 'attributeChangedCallback'); - if (!callback?.body) { - return; - } - const sourceFile = node.getSourceFile(); - for (const branch of collectBranches(ts, callback.body)) { - const assignment = findAssignment(ts, branch.statements); - const fieldName = assignment?.fieldName ?? kebabToCamel(branch.name); - const { type, default: defaultValue, format } = describeValue( - ts, sourceFile, assignment?.value, `${classDoc.name}'s '${branch.name}'` - ); - + const record = (name, fieldName, { type, default: defaultValue, format }) => { classDoc.attributes ??= []; - let attribute = classDoc.attributes.find(existing => existing.name === branch.name); + let attribute = classDoc.attributes.find(existing => existing.name === name); if (!attribute) { - attribute = { name: branch.name }; + attribute = { name }; classDoc.attributes.push(attribute); } @@ -416,6 +498,33 @@ export const attributesFromCallbackPlugin = () => ({ // Consumed (and removed) in moduleLinkPhase, once member docs are available attribute._pwcFormat = format; } + }; + + // The static `properties` table + for (const entry of collectTableEntries(ts, node)) { + const { type, format } = describeParse( + ts, sourceFile, entry.parse, `${classDoc.name}'s '${entry.attribute}'` + ); + record(entry.attribute, entry.fieldName, { + type, + default: renderFieldDefault(ts, node, entry.fieldName), + format + }); + } + + // Transitional: the attributeChangedCallback switch + const callback = node.members.find(member => ts.isMethodDeclaration(member) && + member.name.getText() === 'attributeChangedCallback'); + if (!callback?.body) { + return; + } + + for (const branch of collectBranches(ts, callback.body)) { + const assignment = findAssignment(ts, branch.statements); + const fieldName = assignment?.fieldName ?? kebabToCamel(branch.name); + record(branch.name, fieldName, describeValue( + ts, sourceFile, assignment?.value, `${classDoc.name}'s '${branch.name}'` + )); } }, From e343e75a882d710fc0011bdad39b923d9873e5b9 Mon Sep 17 00:00:00 2001 From: Will Eastcott Date: Sun, 16 Aug 2026 17:25:32 +0100 Subject: [PATCH 2/2] Revise the tables around descriptor-owned defaults and chain-walk merge Review on the pilot identified the lazy instance snapshot as a defect, not a trade: a property written programmatically before the element's first attribute reaction shifted the fallback baseline, making removal order-dependent and able to restore values that contradict the published defaults. Synthetic construction was rejected as a fix - it would run consumer subclass constructors invisibly and still leave runtime and manifest recovering defaults through two mechanisms. The descriptor is now the single authoritative declaration, read by both runtime dispatch and the manifest plugin: - Factories (booleanProperty, numberProperty, stringProperty, colorProperty, enumProperty) own the defaults. Mutable initial values are factories, so removal always assigns a fresh instance; backing fields reference descriptor.initial() instead of restating the value. The snapshot machinery is deleted outright. - `invalid` declares a malformed-value fallback distinct from the initial value (pc-asset's texture options: unset initially, engine constant on invalid input). `attribute`, `property` and `apply` cover aliases and presence-dependent side effects (pc-material's roughness) declaratively. - Each class declares only its own table; observedAttributes and dispatch merge the constructor chain at lookup, so a base table cannot be dropped by a forgotten spread. - defineProperties is deliberately unconstrained: a PropertyTable bound contextually typed every entry as PropertyDeclaration, collapsing enumProperty's literal-union inference to string. Shape checking happens at the static declarations instead. A side effect of the descriptor types: pc-light's shadow-type union is now spelled once (the Map) instead of twice. - New regression test: a property-before-first-attribute write no longer shifts what removal or an invalid value restores. New machinery suite pins the chain merge, the initial/invalid split, the overrides, and the apply hook through scratch elements. dist/custom-elements.json, vscode.html-custom-data.json and web-types.json remain byte-identical to main; validate.mjs untouched and green; the d.ts diff is unchanged from the previous commit. Co-Authored-By: Claude Fable 5 --- src/components/component.ts | 19 +- src/components/light-component.ts | 96 ++++---- src/properties.ts | 356 +++++++++++++++++++++--------- test/elements/light.test.ts | 41 +++- test/elements/properties.test.ts | 191 ++++++++++++++++ utils/cem/attributes-plugin.mjs | 275 ++++++++++++++--------- 6 files changed, 694 insertions(+), 284 deletions(-) create mode 100644 test/elements/properties.test.ts diff --git a/src/components/component.ts b/src/components/component.ts index 1e7e3c7e..ad413fb6 100644 --- a/src/components/component.ts +++ b/src/components/component.ts @@ -3,9 +3,12 @@ import type { Component } from 'playcanvas'; import type { AppElement } from '../app'; import { AsyncElement } from '../async-element'; import type { EntityBaseElement } from '../entity-base'; -import { parseBool } from '../parse'; import type { PropertyTable } from '../properties'; -import { applyAttribute, attributeNames } from '../properties'; +import { applyAttribute, attributeNames, booleanProperty, defineProperties } from '../properties'; + +const componentProperties = defineProperties({ + enabled: booleanProperty(true) +}); /** * Represents a component in the PlayCanvas engine. @@ -15,7 +18,7 @@ import { applyAttribute, attributeNames } from '../properties'; class ComponentElement extends AsyncElement { private _componentName: string; - private _enabled = true; + private _enabled = componentProperties.enabled.initial(); private _component: Component | null = null; @@ -244,16 +247,14 @@ class ComponentElement extends AsyncElement { } /** - * The attribute schema shared by every component element. A subclass declares its own - * attributes by spreading this table into its own (see `src/properties.ts`). + * The attribute schema shared by every component element. A subclass declares only its own + * table; the chain of tables is merged at lookup (see `src/properties.ts`). * @internal */ - static properties: PropertyTable = { - enabled: { parse: parseBool } - }; + static properties: PropertyTable = componentProperties; static get observedAttributes() { - return attributeNames(this.properties); + return attributeNames(this); } attributeChangedCallback(name: string, _oldValue: string | null, newValue: string | null) { diff --git a/src/components/light-component.ts b/src/components/light-component.ts index 5ca7ca56..21ba19ad 100644 --- a/src/components/light-component.ts +++ b/src/components/light-component.ts @@ -12,9 +12,7 @@ import { SHADOW_VSM_32F } from 'playcanvas'; -import { parseBool, parseColor, parseNumber } from '../parse'; -import type { PropertyTable } from '../properties'; -import { enumOf } from '../properties'; +import { booleanProperty, colorProperty, defineProperties, enumProperty, numberProperty } from '../properties'; import { ComponentElement } from './component'; @@ -33,6 +31,28 @@ const shadowTypes = new Map< ['pcss-32f', SHADOW_PCSS_32F] ]); +const lightProperties = defineProperties({ + castShadows: booleanProperty(false), + color: colorProperty(() => new Color(1, 1, 1)), + innerConeAngle: numberProperty(40), + intensity: numberProperty(1), + normalOffsetBias: numberProperty(0.05), + outerConeAngle: numberProperty(45), + penumbraFalloff: numberProperty(1), + penumbraSize: numberProperty(1), + range: numberProperty(10), + shadowBias: numberProperty(0.2), + shadowBlockerSamples: numberProperty(16), + shadowDistance: numberProperty(16), + shadowIntensity: numberProperty(1), + shadowResolution: numberProperty(1024), + shadowSamples: numberProperty(16), + shadowType: enumProperty(shadowTypes, 'pcf3-32f'), + type: enumProperty(['directional', 'omni', 'spot'], 'directional'), + vsmBias: numberProperty(0.01), + vsmBlurSize: numberProperty(11) +}); + /** * The LightComponentElement interface provides properties and methods for manipulating * {@link https://developer.playcanvas.com/user-manual/web-components/tags/pc-light/ | ``} elements. @@ -42,52 +62,43 @@ const shadowTypes = new Map< * @category Components */ class LightComponentElement extends ComponentElement { - private _castShadows = false; + private _castShadows = lightProperties.castShadows.initial(); - private _color = new Color(1, 1, 1); + private _color = lightProperties.color.initial(); - private _innerConeAngle = 40; + private _innerConeAngle = lightProperties.innerConeAngle.initial(); - private _intensity = 1; + private _intensity = lightProperties.intensity.initial(); - private _normalOffsetBias = 0.05; + private _normalOffsetBias = lightProperties.normalOffsetBias.initial(); - private _outerConeAngle = 45; + private _outerConeAngle = lightProperties.outerConeAngle.initial(); - private _range = 10; + private _range = lightProperties.range.initial(); - private _shadowBias = 0.2; + private _shadowBias = lightProperties.shadowBias.initial(); - private _shadowDistance = 16; + private _shadowDistance = lightProperties.shadowDistance.initial(); - private _shadowIntensity = 1; + private _shadowIntensity = lightProperties.shadowIntensity.initial(); - private _shadowResolution = 1024; + private _shadowResolution = lightProperties.shadowResolution.initial(); - private _shadowType: - | 'pcf1-16f' - | 'pcf1-32f' - | 'pcf3-16f' - | 'pcf3-32f' - | 'pcf5-16f' - | 'pcf5-32f' - | 'vsm-16f' - | 'vsm-32f' - | 'pcss-32f' = 'pcf3-32f'; + private _shadowType = lightProperties.shadowType.initial(); - private _type: 'directional' | 'omni' | 'spot' = 'directional'; + private _type = lightProperties.type.initial(); - private _vsmBias = 0.01; + private _vsmBias = lightProperties.vsmBias.initial(); - private _vsmBlurSize = 11; + private _vsmBlurSize = lightProperties.vsmBlurSize.initial(); - private _penumbraSize = 1; + private _penumbraSize = lightProperties.penumbraSize.initial(); - private _penumbraFalloff = 1; + private _penumbraFalloff = lightProperties.penumbraFalloff.initial(); - private _shadowSamples = 16; + private _shadowSamples = lightProperties.shadowSamples.initial(); - private _shadowBlockerSamples = 16; + private _shadowBlockerSamples = lightProperties.shadowBlockerSamples.initial(); /** @ignore */ constructor() { @@ -510,28 +521,7 @@ class LightComponentElement extends ComponentElement { } /** @internal */ - static properties: PropertyTable = { - ...ComponentElement.properties, - castShadows: { parse: parseBool }, - color: { parse: parseColor }, - innerConeAngle: { parse: parseNumber }, - intensity: { parse: parseNumber }, - normalOffsetBias: { parse: parseNumber }, - outerConeAngle: { parse: parseNumber }, - penumbraFalloff: { parse: parseNumber }, - penumbraSize: { parse: parseNumber }, - range: { parse: parseNumber }, - shadowBias: { parse: parseNumber }, - shadowBlockerSamples: { parse: parseNumber }, - shadowDistance: { parse: parseNumber }, - shadowIntensity: { parse: parseNumber }, - shadowResolution: { parse: parseNumber }, - shadowSamples: { parse: parseNumber }, - shadowType: { parse: enumOf(shadowTypes) }, - type: { parse: enumOf(['directional', 'omni', 'spot']) }, - vsmBias: { parse: parseNumber }, - vsmBlurSize: { parse: parseNumber } - }; + static properties = lightProperties; } customElements.define('pc-light', LightComponentElement); diff --git a/src/properties.ts b/src/properties.ts index 06440228..293ea776 100644 --- a/src/properties.ts +++ b/src/properties.ts @@ -1,136 +1,296 @@ /** - * The property-table machinery behind `attributeChangedCallback`. An element class declares a - * static `properties` table mapping each property to how its attribute parses; the base classes - * derive `observedAttributes` from the table and route every attribute change through - * {@link applyAttribute}. One table entry replaces what used to be restated per attribute: the - * entry in `observedAttributes` and the `case` in the dispatch switch. + * The property-descriptor machinery behind `attributeChangedCallback`. An element class declares + * a static `properties` table of descriptors built by {@link defineProperties}; the base classes + * derive `observedAttributes` from the merged tables of the constructor chain and route every + * attribute change through {@link applyAttribute}. One descriptor is the single, authoritative + * declaration of everything that used to be restated per attribute — the observed name, the + * value type, and the defaults — and both runtime dispatch and the manifest tooling read it. * - * Defaults are deliberately absent from the table. Custom element reactions never run - * mid-constructor, so by the first `attributeChangedCallback` every field initializer has run; - * snapshotting the element's properties then captures exactly the initializer values, and an - * absent, removed or invalid attribute falls back to that snapshot. A default therefore lives in - * one place only — the backing field's initializer — and the manifest tooling reads it from - * there rather than from a restated literal. + * The semantics every descriptor shares: + * + * - An absent or removed attribute assigns `initial()` — a fresh value each time, so a mutable + * default can never be aliased and mutated through. + * - A malformed value warns (via the `parse` helpers) and assigns the `invalid` fallback, which + * is the initial value unless declared otherwise. `pc-asset`'s texture options are the split + * case: unset means "leave the engine's per-format default in force" (`initial: null`) while a + * malformed value falls back to the engine constant the warning names. + * - The parse helpers therefore never see `null` here; removal is resolved before parsing. + * + * Escape hatches, for the attributes that are genuinely not plain mappings: `attribute` names an + * attribute the kebab-cased property name cannot express, `property` retargets the assignment + * (and the manifest's `fieldName`) for aliases, and `apply` replaces the assignment entirely for + * attributes with presence-dependent side effects. Elements with non-property attributes keep an + * `attributeChangedCallback` override chaining `super`, exactly as before. */ -import { Color, Quat, Vec2, Vec3, Vec4 } from 'playcanvas'; +import type { Color } from 'playcanvas'; -import { parseEnum } from './parse'; +import { parseBool, parseColor, parseEnum, parseNumber } from './parse'; /** - * Converts an attribute value into the value assigned to the element property. The parse helpers - * in `parse.ts` all have this shape; `defaultValue` is `any` so their narrower per-type - * signatures remain assignable. + * Replaces the plain property assignment of a parsed attribute value, for attributes whose + * effects go beyond one property — writing several, or reacting to the attribute's presence. * - * @param value - The attribute value (`null` when the attribute is absent or removed). - * @param defaultValue - The value to fall back to, from the element's defaults snapshot. - * @param attribute - The attribute name, used in warning messages. - * @returns The value to assign to the property. + * @param element - The element whose attribute changed. Typed loosely so a hook can be declared + * against its concrete element class. + * @param value - The parsed value: `initial()` for a removed attribute, the parse result + * otherwise. + * @param raw - The raw attribute value (`null` when the attribute was removed), for + * presence-dependent behavior. * @internal */ -export type AttributeParser = (value: string | null, defaultValue: any, attribute: string) => unknown; +export type PropertyApply = (element: any, value: T, raw: string | null) => void; /** - * One property's entry in a {@link PropertyTable}. + * The options every descriptor factory accepts. * @internal */ -export type PropertyDefinition = { +export type PropertyOptions = { /** * The attribute name, when it is not simply the kebab-cased property name (an alias, or an * initialism the mechanical conversion would mangle). */ attribute?: string; - /** The parse helper converting the attribute value for this property. */ - parse: AttributeParser; + /** + * The property the parsed value is assigned to (and the manifest's `fieldName`), when it is + * not the table key — i.e. the attribute is an alias for another property. + */ + property?: string; + + /** + * The fallback for a malformed attribute value, when it differs from the initial value. A + * factory, for a mutable value. + */ + invalid?: T | (() => T); + + /** The assignment replacement — see {@link PropertyApply}. */ + apply?: PropertyApply; }; /** - * An element class's attribute schema, keyed by property name. A subclass extends its base - * class's schema by spreading it (`{ ...ComponentElement.properties, intensity: ... }`), so the - * table on any class always describes that element's full attribute surface. + * One property's descriptor: how its attribute parses, and the defaults. Built by the factory + * functions below, never by hand — the manifest tooling derives each attribute's type from the + * factory's identity and its published default from the factory's arguments. * @internal */ -export type PropertyTable = Record; +export type PropertyDeclaration = { + attribute?: string; + property?: string; + apply?: PropertyApply; + + /** Parses a present attribute value; `fallback` is only consulted for a malformed one. */ + parse: (value: string, fallback: T, attribute: string) => T; + + /** Creates the initial value — referenced by the backing field's initializer. */ + initial: () => T; + + /** Creates the malformed-value fallback. Defaults to {@link initial}. */ + invalid: () => T; +}; /** - * The attribute name observed for a property: an explicit `attribute` override, or the - * kebab-cased property name. - * - * @param property - The property name. - * @param definition - The property's table entry. - * @returns The attribute name. + * An element class's own attribute schema, keyed by property name. Declared per class and merged + * across the constructor chain at lookup time — a subclass never restates (or spreads) its base + * class's table. + * @internal */ -const attributeOf = (property: string, definition: PropertyDefinition): string => { - return definition.attribute ?? property.replace(/[A-Z]/g, (char) => `-${char.toLowerCase()}`); +export type PropertyTable = Record>; + +/** Normalizes an optional value-or-factory to a factory. */ +const toFactory = (value: T | (() => T)): (() => T) => { + return typeof value === 'function' ? (value as () => T) : () => value; +}; + +const definition = ( + parse: PropertyDeclaration['parse'], + initial: () => T, + options?: PropertyOptions +): PropertyDeclaration => { + return { + parse, + initial, + invalid: options?.invalid === undefined ? initial : toFactory(options.invalid), + attribute: options?.attribute, + property: options?.property, + apply: options?.apply + }; }; /** - * Reverse indexes (attribute name → property and definition), built once per table. Tables are - * class statics, so this caches per class, not per element. + * Declares a boolean property, with the standard boolean attribute rules (`'false'` is false; + * any other present value, including a bare attribute's empty string, is true). + * + * @param initial - The initial value, or `null` for a property that starts unset. + * @param options - The descriptor options. + * @returns The descriptor. + * @internal */ -const indexes = new WeakMap>(); - -const indexOf = (table: PropertyTable) => { - let index = indexes.get(table); - if (!index) { - index = new Map(); - for (const [property, definition] of Object.entries(table)) { - index.set(attributeOf(property, definition), { property, definition }); - } - indexes.set(table, index); - } - return index; +export function booleanProperty(initial: boolean, options?: PropertyOptions): PropertyDeclaration; +/** @internal */ +export function booleanProperty( + initial: null, + options?: PropertyOptions +): PropertyDeclaration; +/** @internal */ +export function booleanProperty(initial: boolean | null, options?: PropertyOptions): PropertyDeclaration { + return definition( + (value, fallback) => parseBool(value, fallback as boolean), + () => initial, + options + ); +} + +/** + * Declares a number property. + * + * @param initial - The initial value, or `null` for a property that starts unset. + * @param options - The descriptor options. + * @returns The descriptor. + * @internal + */ +export function numberProperty(initial: number, options?: PropertyOptions): PropertyDeclaration; +/** @internal */ +export function numberProperty( + initial: null, + options?: PropertyOptions +): PropertyDeclaration; +/** @internal */ +export function numberProperty(initial: number | null, options?: PropertyOptions): PropertyDeclaration { + return definition(parseNumber, () => initial, options); +} + +/** + * Declares a string property. Every string is a valid value, so this never warns and `invalid` + * is meaningless. + * + * @param initial - The initial value. + * @param options - The descriptor options. + * @returns The descriptor. + * @internal + */ +export const stringProperty = (initial: string, options?: PropertyOptions): PropertyDeclaration => { + return definition( + (value) => value, + () => initial, + options + ); }; /** - * The attribute names a table observes, for `static get observedAttributes()`. + * Declares a {@link Color} property. The initial value is a factory, so each element (and each + * removal) gets a fresh instance. * - * @param table - The property table. - * @returns The attribute names. + * @param initial - Creates the initial value. + * @param options - The descriptor options. + * @returns The descriptor. * @internal */ -export const attributeNames = (table: PropertyTable): string[] => { - return [...indexOf(table).keys()]; +export const colorProperty = (initial: () => Color, options?: PropertyOptions): PropertyDeclaration => { + return definition(parseColor, initial, options); }; /** - * Clones a snapshot value, so the recorded default can never alias a live property value that a - * later parse result (or an in-place mutation) would write through. Math types are the mutable - * ones the parse helpers produce; arrays cover `parseTags`. + * Declares an enum property. The valid-name set is carried once, here: dispatch resolves values + * against it, and the manifest tooling reads the published enum values from this call's argument. * - * @param value - The property value to record. - * @returns The value to store in the snapshot. + * @param valid - The valid names: an array, or a map whose keys are the valid names. + * @param initial - The initial value, or `null` for a property that starts unset. + * @param options - The descriptor options. + * @returns The descriptor. + * @internal */ -const cloneValue = (value: unknown): unknown => { - if ( - value instanceof Color || - value instanceof Quat || - value instanceof Vec2 || - value instanceof Vec3 || - value instanceof Vec4 - ) { - return value.clone(); - } - return Array.isArray(value) ? [...value] : value; +export function enumProperty( + valid: readonly T[] | ReadonlyMap, + initial: T, + options?: PropertyOptions +): PropertyDeclaration; +/** @internal */ +export function enumProperty( + valid: readonly T[] | ReadonlyMap, + initial: null, + options?: PropertyOptions +): PropertyDeclaration; +/** @internal */ +export function enumProperty( + valid: readonly T[] | ReadonlyMap, + initial: T | null, + options?: PropertyOptions +): PropertyDeclaration { + return definition( + (value, fallback, attribute) => parseEnum(value, valid, fallback as T, attribute), + () => initial, + options + ); +} + +/** + * Declares a class's own property table. An identity function: it gives the manifest tooling a + * recognizable marker to read the table from. Deliberately unconstrained — a `PropertyTable` + * bound would contextually type every entry as `PropertyDeclaration`, collapsing the + * literal-union inference of `enumProperty` to `string`. The table's shape is checked where the + * class declares it instead: `static properties` is typed `PropertyTable` on each root class, + * and a subclass's static must be assignable to its base's. + * + * @param properties - The descriptors, keyed by property name. + * @returns The table, unchanged. + * @internal + */ +export const defineProperties = (properties: Table): Table => { + return properties; }; -/** Per-element snapshots of every table property's pre-attribute value. */ -const defaults = new WeakMap>(); +const camelToKebab = (name: string): string => { + return name.replace(/[A-Z]/g, (char) => `-${char.toLowerCase()}`); +}; + +/** + * Merged lookup tables (attribute name → target property and descriptor), built once per class + * by walking the constructor chain base-first, so a derived class's entry shadows its base's. + */ +const tables = new WeakMap }>>(); + +const tableFor = (constructor: object) => { + let table = tables.get(constructor); + if (table) { + return table; + } + + const chain: PropertyTable[] = []; + for (let current: object | null = constructor; current; current = Object.getPrototypeOf(current)) { + if (Object.hasOwn(current, 'properties')) { + chain.push((current as { properties: PropertyTable }).properties); + } + } -const snapshotDefaults = (element: HTMLElement, table: PropertyTable): Record => { - const snapshot: Record = {}; - for (const property of Object.keys(table)) { - snapshot[property] = cloneValue((element as unknown as Record)[property]); + table = new Map(); + for (const properties of chain.reverse()) { + for (const [key, entry] of Object.entries(properties)) { + table.set(entry.attribute ?? camelToKebab(key), { property: entry.property ?? key, entry }); + } } - return snapshot; + tables.set(constructor, table); + return table; +}; + +/** + * The attribute names observed by a class, merged across its constructor chain, for + * `static get observedAttributes()`. + * + * @param constructor - The element class (`this`, in the static getter). + * @returns The attribute names, base class's first. + * @internal + */ +export const attributeNames = (constructor: object): string[] => { + return [...tableFor(constructor).keys()]; }; /** * Generic `attributeChangedCallback` dispatch: resolves the changed attribute against the - * element class's property table and assigns the parsed value through the property's accessor. - * An attribute the table does not know is ignored, so an element handling extra, non-property - * attributes overrides `attributeChangedCallback` and chains `super` exactly as before. + * element class's merged property table and assigns the parsed value through the target + * property's accessor (or hands it to the descriptor's `apply` hook). An attribute the tables + * do not know is ignored, so an element handling extra, non-property attributes overrides + * `attributeChangedCallback` and chains `super` exactly as before. * * @param element - The element whose attribute changed. * @param name - The attribute name. @@ -138,34 +298,18 @@ const snapshotDefaults = (element: HTMLElement, table: PropertyTable): Record { - const table = (element.constructor as unknown as { properties: PropertyTable }).properties; - const match = indexOf(table).get(name); + const match = tableFor(element.constructor).get(name); if (!match) { return; } - let snapshot = defaults.get(element); - if (!snapshot) { - snapshot = snapshotDefaults(element, table); - defaults.set(element, snapshot); - } + const { property, entry } = match; + const parsed = value === null ? entry.initial() : entry.parse(value, entry.invalid(), name); - (element as unknown as Record)[match.property] = match.definition.parse( - value, - snapshot[match.property], - name - ); -}; + if (entry.apply) { + entry.apply(element, parsed, value); + return; + } -/** - * Binds `parseEnum` to its set of valid names. The set is carried once, here: dispatch resolves - * values against it, and the manifest tooling reads the published enum values from this call's - * argument. - * - * @param valid - The valid names: an array, or a map whose keys are the valid names. - * @returns The bound parser. - * @internal - */ -export const enumOf = (valid: readonly T[] | ReadonlyMap): AttributeParser => { - return (value, defaultValue, attribute) => parseEnum(value, valid, defaultValue, attribute); + (element as unknown as Record)[property] = parsed; }; diff --git a/test/elements/light.test.ts b/test/elements/light.test.ts index ead7dc11..90bb3d5f 100644 --- a/test/elements/light.test.ts +++ b/test/elements/light.test.ts @@ -5,13 +5,13 @@ import { LightComponentElement } from '../../src/components/light-component'; import { useGuard } from '../helpers/guard'; /** - * is the pilot of the static property table (src/properties.ts), so beyond the - * element's own attribute surface these tests pin the table machinery itself: observedAttributes - * derived from the table, dispatch through the shared attributeChangedCallback, and defaults - * restored from the first-callback snapshot of the field initializers - for removal, and for the - * fallback (and warning text) of an invalid value. The element caches every property to a private - * field and only writes through once its engine component exists, so all of it is observable with - * no in play. + * is the pilot of the static property-descriptor table (src/properties.ts), so beyond + * the element's own attribute surface these tests pin the machinery itself: observedAttributes + * merged across the constructor chain, dispatch through the shared attributeChangedCallback, and + * the descriptor's declared initial value - restored on removal, and named in the warning as the + * fallback of an invalid value, regardless of any earlier programmatic writes. The element caches + * every property to a private field and only writes through once its engine component exists, so + * all of it is observable with no in play. */ describe('', () => { const { warnings } = useGuard(); @@ -55,7 +55,7 @@ describe('', () => { expect(element.intensity).toBe(1); }); - it('falls back to the field default on an invalid number, naming it in the warning', () => { + it('falls back to the declared default on an invalid number, naming it in the warning', () => { const element = create(); element.setAttribute('shadow-bias', '0.5'); @@ -66,6 +66,25 @@ describe('', () => { warnings.expect("Invalid value 'steep' for attribute 'shadow-bias'. Expected a finite number. Using '0.2'."); }); + it('restores the declared default, not an earlier programmatic write', () => { + const element = create(); + + // A property written before the element's first attribute reaction must not shift what + // removal (or an invalid value) restores + element.intensity = 7; + + element.setAttribute('intensity', '2'); + expect(element.intensity).toBe(2); + + element.removeAttribute('intensity'); + expect(element.intensity).toBe(1); + + element.intensity = 7; + element.setAttribute('intensity', 'garbage'); + expect(element.intensity).toBe(1); + warnings.expect("Invalid value 'garbage' for attribute 'intensity'. Expected a finite number. Using '1'."); + }); + it('parses a boolean attribute with the standard rules', () => { const element = create(); @@ -101,13 +120,13 @@ describe('', () => { expect(element.color).toEqual(new Color(1, 1, 1)); }); - it('never hands out the recorded default itself', () => { + it('creates a fresh default on every removal', () => { const element = create(); element.setAttribute('color', 'red'); element.removeAttribute('color'); - // Mutate the restored value in place; the snapshot must not be written through + // Mutate the restored value in place; the declared default must not be written through element.color.r = 0.25; element.setAttribute('color', 'blue'); @@ -123,7 +142,7 @@ describe('', () => { element.setAttribute('shadow-type', 'vsm-16f'); expect(element.shadowType).toBe('vsm-16f'); - // The fallback is the field default, not the previous value + // The fallback is the declared default, not the previous value element.setAttribute('shadow-type', 'soft'); expect(element.shadowType).toBe('pcf3-32f'); warnings.expect( diff --git a/test/elements/properties.test.ts b/test/elements/properties.test.ts new file mode 100644 index 00000000..da35dbf5 --- /dev/null +++ b/test/elements/properties.test.ts @@ -0,0 +1,191 @@ +import { describe, expect, it } from 'vitest'; + +import type { PropertyTable } from '../../src/properties'; +import { + applyAttribute, + attributeNames, + defineProperties, + enumProperty, + numberProperty, + stringProperty +} from '../../src/properties'; +import { useGuard } from '../helpers/guard'; + +/** + * The property-descriptor machinery, exercised through scratch elements rather than the + * library's own. These pin the behaviors an element migration relies on but which no migrated + * element demonstrates in isolation yet: tables merged across the constructor chain, the + * attribute and property overrides, the split between the unset initial value and the + * invalid-value fallback (pc-asset's texture options), and the apply hook for attributes with + * presence-dependent side effects (pc-material's roughness). 's suite covers the same + * dispatch through a real element. + */ + +const baseProperties = defineProperties({ + alpha: numberProperty(1) +}); + +class PropertiesBaseElement extends HTMLElement { + /** @internal */ + static properties: PropertyTable = baseProperties; + + static get observedAttributes() { + return attributeNames(this); + } + + private _alpha = baseProperties.alpha.initial(); + + set alpha(value: number) { + this._alpha = value; + } + + get alpha() { + return this._alpha; + } + + attributeChangedCallback(name: string, _oldValue: string | null, newValue: string | null) { + applyAttribute(this, name, newValue); + } +} + +const derivedProperties = defineProperties({ + // pc-asset's texture-option shape: unset initially, but a malformed value falls back to a + // declared constant rather than to unset + mode: enumProperty(['fast', 'slow'], null, { invalid: 'fast' }), + + // an alias: the attribute name and the assigned property both differ from the key + strength: numberProperty(0.5, { attribute: 'power', property: 'level' }), + + // pc-material roughness's shape: the parsed value plus the attribute's presence, applied by + // hand instead of assigned + boost: numberProperty(0.25, { + property: 'output', + apply: (element: PropertiesDerivedElement, value, raw) => { + element.output = value; + element.boosted = raw !== null; + } + }), + + label: stringProperty('none') +}); + +class PropertiesDerivedElement extends PropertiesBaseElement { + /** @internal */ + static properties: PropertyTable = derivedProperties; + + private _mode = derivedProperties.mode.initial(); + + private _level = derivedProperties.strength.initial(); + + private _output = derivedProperties.boost.initial(); + + private _label = derivedProperties.label.initial(); + + boosted = false; + + set mode(value: 'fast' | 'slow' | null) { + this._mode = value; + } + + get mode() { + return this._mode; + } + + set level(value: number) { + this._level = value; + } + + get level() { + return this._level; + } + + set output(value: number) { + this._output = value; + } + + get output() { + return this._output; + } + + set label(value: string) { + this._label = value; + } + + get label() { + return this._label; + } +} + +customElements.define('test-properties-base', PropertiesBaseElement); +customElements.define('test-properties-derived', PropertiesDerivedElement); + +describe('property descriptors', () => { + const { warnings } = useGuard(); + + const create = () => document.createElement('test-properties-derived') as PropertiesDerivedElement; + + it('merges observedAttributes across the constructor chain, base class first', () => { + expect(PropertiesBaseElement.observedAttributes).toEqual(['alpha']); + expect(PropertiesDerivedElement.observedAttributes).toEqual(['alpha', 'mode', 'power', 'boost', 'label']); + }); + + it('dispatches an attribute declared by the base class on a derived element', () => { + const element = create(); + + element.setAttribute('alpha', '0.5'); + expect(element.alpha).toBe(0.5); + + element.removeAttribute('alpha'); + expect(element.alpha).toBe(1); + }); + + it('distinguishes the unset initial value from the invalid-value fallback', () => { + const element = create(); + + expect(element.mode, 'starts unset').toBeNull(); + + element.setAttribute('mode', 'slow'); + expect(element.mode).toBe('slow'); + + element.setAttribute('mode', 'medium'); + expect(element.mode, 'a malformed value falls back to the declared constant, not to unset').toBe('fast'); + warnings.expect("Invalid value 'medium' for attribute 'mode'. Valid values: fast, slow. Using 'fast'."); + + element.removeAttribute('mode'); + expect(element.mode, 'removal restores unset').toBeNull(); + }); + + it('honors the attribute and property overrides', () => { + const element = create(); + + expect(element.level).toBe(0.5); + + element.setAttribute('power', '2'); + expect(element.level).toBe(2); + + element.removeAttribute('power'); + expect(element.level).toBe(0.5); + }); + + it('hands an apply hook the parsed value and the raw presence', () => { + const element = create(); + + element.setAttribute('boost', '4'); + expect(element.output).toBe(4); + expect(element.boosted).toBe(true); + + element.removeAttribute('boost'); + expect(element.output, 'the hook receives the declared initial value on removal').toBe(0.25); + expect(element.boosted).toBe(false); + }); + + it('accepts any string for a string property, without warning', () => { + const element = create(); + + element.setAttribute('label', 'anything at all'); + expect(element.label).toBe('anything at all'); + + element.removeAttribute('label'); + expect(element.label).toBe('none'); + }); +}); diff --git a/utils/cem/attributes-plugin.mjs b/utils/cem/attributes-plugin.mjs index d26ac28f..18aeb77e 100644 --- a/utils/cem/attributes-plugin.mjs +++ b/utils/cem/attributes-plugin.mjs @@ -2,12 +2,14 @@ * Custom Elements Manifest plugin that derives attribute metadata from each element's attribute * declarations, from either of two sources: * - * - A static `properties` table (see `src/properties.ts`), where an entry of the form - * `shadowType: { parse: enumOf(shadowTypes) }` carries the property (`fieldName`), the - * attribute name (kebab-cased, unless overridden by `attribute`), the attribute's type - * (implied by the parse helper) and its enum values (the `enumOf` argument). The default is - * read from the backing field's initializer (`private _shadowType = 'pcf3-32f'`), which the - * table pattern makes the single statement of each default. + * - A static `properties` table of descriptors (see `src/properties.ts`), where an entry of the + * form `shadowType: enumProperty(shadowTypes, 'pcf3-32f')` carries the property (`fieldName`, + * unless retargeted by `property`), the attribute name (kebab-cased, unless overridden by + * `attribute`), the attribute's type (implied by the descriptor factory), its enum values (the + * factory's first argument) and its default (the declared initial value — or the `invalid` + * fallback, for a property whose initial value is `null`, matching the constant the + * invalid-value warning names). The descriptor is the single authoritative declaration: + * runtime dispatch reads the same one. * * - Transitionally, an `attributeChangedCallback` whose body is a uniform mapping of the form: * @@ -73,10 +75,45 @@ const EMPTY_CONSTRUCTORS = { Vec4: '0 0 0 0' }; +/** + * The property-descriptor factories from `src/properties.ts`, mapped to the manifest type each + * one implies. String attributes never publish a default — every one of them names an external + * resource (or a label) with no meaningful value to suggest. + */ +const PROPERTY_FACTORIES = { + booleanProperty: { type: 'boolean' }, + numberProperty: { type: 'number' }, + enumProperty: { type: 'enum' }, + stringProperty: { type: 'string', omitDefault: true }, + colorProperty: { type: 'string', format: 'color' } +}; + const kebabToCamel = name => name.replace(/-([a-z])/g, (_, char) => char.toUpperCase()); const camelToKebab = name => name.replace(/[A-Z]/g, char => `-${char.toLowerCase()}`); +/** + * Resolves an identifier to the initializer of its module-scope `const` declaration. + * + * @param {import('typescript')} ts - The TypeScript module supplied by the analyzer. + * @param {import('typescript').SourceFile} sourceFile - The file to search. + * @param {string} name - The identifier text. + * @returns {import('typescript').Expression | undefined} The initializer, or `undefined`. + */ +const resolveModuleConst = (ts, sourceFile, name) => { + for (const statement of sourceFile.statements) { + if (!ts.isVariableStatement(statement)) { + continue; + } + for (const declaration of statement.declarationList.declarations) { + if (declaration.name.getText() === name && declaration.initializer) { + return declaration.initializer; + } + } + } + return undefined; +}; + /** * Extracts `name === 'some-attribute'` comparisons, so that elements handling a single attribute * with an `if` rather than a `switch` (see `src/asset.ts`) are covered too. @@ -200,6 +237,10 @@ const resolveEnumValues = (ts, sourceFile, expression) => { .filter(element => ts.isStringLiteralLike(element)) .map(element => element.text); + if (!expression) { + return []; + } + if (ts.isArrayLiteralExpression(expression)) { return fromArrayLiteral(expression); } @@ -209,43 +250,35 @@ const resolveEnumValues = (ts, sourceFile, expression) => { } // Resolve a module-scope `const orientations = new Map<'horizontal' | 'vertical', number>([...])` - const name = expression.text; - for (const statement of sourceFile.statements) { - if (!ts.isVariableStatement(statement)) { - continue; - } - for (const declaration of statement.declarationList.declarations) { - if (declaration.name.getText() !== name || !declaration.initializer) { - continue; - } - const { initializer } = declaration; - - if (ts.isArrayLiteralExpression(initializer)) { - return fromArrayLiteral(initializer); - } + const initializer = resolveModuleConst(ts, sourceFile, expression.text); + if (!initializer) { + return []; + } - if (ts.isNewExpression(initializer)) { - // Prefer the entry keys, falling back to the `Map` type argument - const entries = initializer.arguments?.[0]; - if (entries && ts.isArrayLiteralExpression(entries)) { - const keys = entries.elements - .filter(entry => ts.isArrayLiteralExpression(entry)) - .map(entry => entry.elements[0]) - .filter(key => key && ts.isStringLiteralLike(key)) - .map(key => key.text); - if (keys.length > 0) { - return keys; - } - } + if (ts.isArrayLiteralExpression(initializer)) { + return fromArrayLiteral(initializer); + } - const union = initializer.typeArguments?.[0]; - if (union && ts.isUnionTypeNode(union)) { - return union.types - .filter(type => ts.isLiteralTypeNode(type) && ts.isStringLiteralLike(type.literal)) - .map(type => type.literal.text); - } + if (ts.isNewExpression(initializer)) { + // Prefer the entry keys, falling back to the `Map` type argument + const entries = initializer.arguments?.[0]; + if (entries && ts.isArrayLiteralExpression(entries)) { + const keys = entries.elements + .filter(entry => ts.isArrayLiteralExpression(entry)) + .map(entry => entry.elements[0]) + .filter(key => key && ts.isStringLiteralLike(key)) + .map(key => key.text); + if (keys.length > 0) { + return keys; } } + + const union = initializer.typeArguments?.[0]; + if (union && ts.isUnionTypeNode(union)) { + return union.types + .filter(type => ts.isLiteralTypeNode(type) && ts.isStringLiteralLike(type.literal)) + .map(type => type.literal.text); + } } return []; @@ -353,89 +386,126 @@ const describeValue = (ts, sourceFile, value, context) => { }; /** - * Collects the entries of a class's static `properties` table (see `src/properties.ts`). Spread - * entries (`...ComponentElement.properties`) restate a base class's table for the runtime merge; - * they are skipped here because the analyzer's inheritance step already copies the base class's - * attributes, marked with `inheritedFrom`. + * Collects the entries of a class's static `properties` descriptor table (see + * `src/properties.ts`). The table is a module-scope `const` (the backing field initializers + * reference it), so an identifier initializer is resolved to its declaration and the + * `defineProperties(...)` wrapper unwrapped. Each entry is a descriptor-factory call plus its + * resolved options. * * @param {import('typescript')} ts - The TypeScript module supplied by the analyzer. + * @param {import('typescript').SourceFile} sourceFile - The file declaring the element. * @param {import('typescript').ClassDeclaration} node - The class declaration. - * @returns {{ fieldName: string, attribute: string, parse?: import('typescript').Expression }[]} The entries. + * @returns {{ fieldName: string, attribute: string, factory: string, + * args: readonly import('typescript').Expression[], + * invalid?: import('typescript').Expression }[]} The entries. */ -const collectTableEntries = (ts, node) => { - const table = node.members.find(member => ts.isPropertyDeclaration(member) && - member.name.getText() === 'properties' && - member.modifiers?.some(modifier => modifier.kind === ts.SyntaxKind.StaticKeyword)); - if (!table?.initializer || !ts.isObjectLiteralExpression(table.initializer)) { +const collectTableEntries = (ts, sourceFile, node) => { + const member = node.members.find(candidate => ts.isPropertyDeclaration(candidate) && + candidate.name.getText() === 'properties' && + candidate.modifiers?.some(modifier => modifier.kind === ts.SyntaxKind.StaticKeyword)); + if (!member?.initializer) { + return []; + } + + let table = member.initializer; + if (ts.isIdentifier(table)) { + table = resolveModuleConst(ts, sourceFile, table.text); + } + if (table && ts.isCallExpression(table) && table.expression.getText() === 'defineProperties') { + table = table.arguments[0]; + } + if (!table || !ts.isObjectLiteralExpression(table)) { return []; } const entries = []; - for (const property of table.initializer.properties) { - if (!ts.isPropertyAssignment(property) || !ts.isObjectLiteralExpression(property.initializer)) { + for (const property of table.properties) { + if (!ts.isPropertyAssignment(property) || !ts.isCallExpression(property.initializer)) { continue; } - const fieldName = ts.isStringLiteralLike(property.name) ? property.name.text : property.name.getText(); - - let attribute = camelToKebab(fieldName); - let parse; - for (const option of property.initializer.properties) { - if (!ts.isPropertyAssignment(option)) { - continue; - } - const name = option.name.getText(); - if (name === 'attribute' && ts.isStringLiteralLike(option.initializer)) { - attribute = option.initializer.text; - } else if (name === 'parse') { - parse = option.initializer; + const key = ts.isStringLiteralLike(property.name) ? property.name.text : property.name.getText(); + const { arguments: args } = property.initializer; + + let attribute; + let fieldName = key; + let invalid; + + // The trailing options argument, when present + const options = args[args.length - 1]; + if (options && ts.isObjectLiteralExpression(options)) { + for (const option of options.properties) { + if (!ts.isPropertyAssignment(option)) { + continue; + } + const name = option.name.getText(); + if (name === 'attribute' && ts.isStringLiteralLike(option.initializer)) { + attribute = option.initializer.text; + } else if (name === 'property' && ts.isStringLiteralLike(option.initializer)) { + fieldName = option.initializer.text; + } else if (name === 'invalid') { + invalid = option.initializer; + } } } - entries.push({ fieldName, attribute, parse }); + + entries.push({ + fieldName, + attribute: attribute ?? camelToKebab(key), + factory: property.initializer.expression.getText(), + args, + invalid + }); } return entries; }; /** - * Derives an attribute's type from a table entry's `parse` expression: a parse helper by - * identity, or an `enumOf(...)` call whose argument carries the valid names. + * Derives an attribute's type and default from its descriptor-factory call. The published + * default is the declared initial value — or the `invalid` fallback, for a property whose + * initial value is `null` (renderDefault omits `null`), matching the constant the invalid-value + * warning names. * * @param {import('typescript')} ts - The TypeScript module supplied by the analyzer. * @param {import('typescript').SourceFile} sourceFile - The file declaring the element. - * @param {import('typescript').Expression} [parse] - The entry's `parse` expression. + * @param {ReturnType[number]} entry - The table entry. * @param {string} context - A label used in warnings. - * @returns {{ type: string, format?: string }} The derived metadata. + * @returns {{ type: string, default?: string, format?: string }} The derived metadata. */ -const describeParse = (ts, sourceFile, parse, context) => { - if (parse && ts.isCallExpression(parse) && parse.expression.getText() === 'enumOf') { - const values = resolveEnumValues(ts, sourceFile, parse.arguments[0]); +const describeEntry = (ts, sourceFile, entry, context) => { + const factory = PROPERTY_FACTORIES[entry.factory]; + if (!factory) { + console.warn(`[cem] unknown property descriptor for ${context}; falling back to string`); + return { type: 'string' }; + } + + // A mutable initial value is declared as a factory — `() => new Color(1, 1, 1)` — so the + // rendered default is the factory's body + const unwrap = expression => (expression && ts.isArrowFunction(expression) && !ts.isBlock(expression.body) ? + expression.body : + expression); + + // enumProperty(valid, initial, options?) carries the valid names first; the value factories + // are (initial, options?) + const initial = factory.type === 'enum' ? entry.args[1] : entry.args[0]; + const defaultValue = renderDefault(ts, unwrap(entry.invalid) ?? unwrap(initial)); + + if (factory.type === 'enum') { + const values = resolveEnumValues(ts, sourceFile, entry.args[0]); if (values.length === 0) { console.warn(`[cem] could not resolve enum values for ${context}; falling back to string`); - return { type: 'string' }; + return { type: 'string', default: defaultValue }; } - return { type: values.map(name => `'${name}'`).join(' | ') }; - } - - const helper = parse && ts.isIdentifier(parse) ? PARSE_HELPERS[parse.text] : undefined; - if (!helper || helper.type === 'enum') { - return { type: 'string' }; + return { + type: values.map(name => `'${name}'`).join(' | '), + default: defaultValue + }; } - return { type: helper.type, format: helper.format }; -}; -/** - * Renders the default of a table-declared property from its backing field's initializer — the - * `_shadowType` of `shadowType`. A property with no backing field (or no initializer) has no - * default to publish. - * - * @param {import('typescript')} ts - The TypeScript module supplied by the analyzer. - * @param {import('typescript').ClassDeclaration} node - The class declaration. - * @param {string} fieldName - The property name. - * @returns {string | undefined} The rendered default. - */ -const renderFieldDefault = (ts, node, fieldName) => { - const field = node.members.find(member => ts.isPropertyDeclaration(member) && - member.name.getText() === `_${fieldName}`); - return renderDefault(ts, field?.initializer); + return { + type: factory.type, + default: factory.omitDefault ? undefined : defaultValue, + format: factory.format + }; }; /** @@ -500,16 +570,11 @@ export const attributesFromCallbackPlugin = () => ({ } }; - // The static `properties` table - for (const entry of collectTableEntries(ts, node)) { - const { type, format } = describeParse( - ts, sourceFile, entry.parse, `${classDoc.name}'s '${entry.attribute}'` - ); - record(entry.attribute, entry.fieldName, { - type, - default: renderFieldDefault(ts, node, entry.fieldName), - format - }); + // The static `properties` descriptor table + for (const entry of collectTableEntries(ts, sourceFile, node)) { + record(entry.attribute, entry.fieldName, describeEntry( + ts, sourceFile, entry, `${classDoc.name}'s '${entry.attribute}'` + )); } // Transitional: the attributeChangedCallback switch