Declare attributes as data: property tables, piloted on pc-light - #392
Closed
willeastcott wants to merge 2 commits into
Closed
Declare attributes as data: property tables, piloted on pc-light#392willeastcott wants to merge 2 commits into
willeastcott wants to merge 2 commits into
Conversation
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 <noreply@anthropic.com>
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<any>, 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 <noreply@anthropic.com>
Contributor
Author
|
Closing after reassessing the trade-off ahead of 1.0. The attribute-drift problem is real, but this implementation adds a descriptor framework and a second CEM source syntax whose return only appears after a full migration. We will keep #391 and reframe it around invariant checks first, with a smaller typed handler-map experiment against pc-material. This exploration was still valuable: it surfaced the initial-versus-invalid fallback distinction, mutable-default requirements, and inheritance cases that any future approach must handle. |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #391 — the base machinery plus the pilot element. Follow-up PRs migrate the remaining elements in small batches.
What this adds
src/properties.ts— descriptor-owned attribute schemas:descriptor.initial()instead of restating the value. Mutable defaults are factories (colorProperty(() => new Color(1, 1, 1))), so removal always assigns a fresh instance — no snapshot, no aliasing, no instance-history dependence.attribute:), target property (property:), type (factory identity), enum values (theenumPropertyargument), and published default (the declared initial, orinvalidfor a property that starts unset) are all direct reads.invalidseparates the malformed-value fallback from the initial value — pc-asset's texture options are unset (null) initially and on removal, but fall back to an engine constant ('repeat') on invalid input. One undifferentiated default cannot express that; the descriptor can.applyreplaces the assignment for presence-dependent attributes — pc-material'sroughnesswritesgloss, setsglossInvertfrom the attribute's presence, and warns on conflicts. The hook receives the parsed value plus the raw attribute value.observedAttributesand dispatch merge the constructor prototype chain at lookup (cached per class). A forgotten spread can no longer silently drop base attributes, and the runtime now matches what the plugin already assumed (own tables + the analyzer'sinheritedFromstep).pc-light(the pilot — bool, number, color, and both enum shapes) drops its 22-lineobservedAttributeslist and 19-case switch. A typing bonus: the shadow-type union is now spelled once (theshadowTypesmap) instead of twice — the field's type is inferred frominitial().utils/cem/attributes-plugin.mjsresolvesstatic properties = <const>throughdefineProperties(...)and reads the descriptor calls. Switch parsing remains for unmigrated elements and retires with the last migration.Two notes for reviewers on non-obvious choices:
definePropertiesis deliberately unconstrained — aPropertyTablebound contextually types every entry asPropertyDeclaration<any>, collapsingenumProperty's literal-union inference tostring. Shape checking happens at thestatic propertiesdeclarations instead.PropertyDeclarationbecausePropertyDefinitionis an ambient DOM global (CSS Properties API) — a shadowing hazard for any file that forgets the import.Verification (the pins from the issue)
dist/custom-elements.jsonbyte-identical to main, as arevscode.html-custom-data.jsonandweb-types.jsonutils/cem/validate.mjsuntouched and green (29 elements)attribute/propertyoverrides, and theapplyhook through scratch elementsnpm run lint,npm run type-check, prettier on changed files cleanproperties.d.tsstrips toexport {}(every declaration@internal, including each overload signature —stripInternalis per-declaration).d.tschange:light-component.d.tsloses its two now-inherited lifecycle redeclarations (identical signatures remain onComponentElement)🤖 Generated with Claude Code