-
Notifications
You must be signed in to change notification settings - Fork 618
feat(fast-html): add AttributeMap class for automatic @attr definitions #7354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
c9a097b
feat(fast-html): add AttributeMap class for automatic @attr definitions
janechu fc9f1de
Change files
janechu 8ede50d
fix(fast-html): push to observedAttributes so setAttribute triggers tβ¦
janechu b8d8407
refactor(fast-html): remove Observable/fastElementRegistry globals frβ¦
janechu a91d111
test(fast-html): verify existing @attr accessors are not overwritten β¦
janechu 6762d86
refactor(fast-html): remove attribute name normalization in AttributeMap
janechu 36e45be
build(fast-html): add entry/templates/state files for attribute-map fβ¦
janechu 64b8760
refactor(fast-html): address PR review comments for AttributeMap
janechu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
7 changes: 7 additions & 0 deletions
7
change/@microsoft-fast-html-90d4b1e4-b272-4691-9b04-e6290ebc8b2f.json
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "prerelease", | ||
| "comment": "Add AttributeMap class for automatic @attr definitions on leaf template bindings", | ||
| "packageName": "@microsoft/fast-html", | ||
| "email": "7559015+janechu@users.noreply.github.com", | ||
| "dependentChangeType": "none" | ||
| } |
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
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
143 changes: 143 additions & 0 deletions
143
packages/fast-html/src/components/attribute-map.spec.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| import { expect, test } from "@playwright/test"; | ||
|
|
||
| test.describe("AttributeMap", () => { | ||
| test.beforeEach(async ({ page }) => { | ||
| await page.goto("/fixtures/attribute-map/"); | ||
| await page.waitForSelector("attribute-map-test-element"); | ||
| }); | ||
|
|
||
| test("should define @attr for a simple leaf property", async ({ page }) => { | ||
| const element = page.locator("attribute-map-test-element"); | ||
|
|
||
| const hasFooAccessor = await element.evaluate(node => { | ||
| const desc = Object.getOwnPropertyDescriptor( | ||
| Object.getPrototypeOf(node), | ||
| "foo", | ||
| ); | ||
| return typeof desc?.get === "function"; | ||
| }); | ||
|
|
||
| expect(hasFooAccessor).toBeTruthy(); | ||
| }); | ||
|
|
||
| test("should define @attr for a dash-case property", async ({ page }) => { | ||
| const element = page.locator("attribute-map-test-element"); | ||
|
|
||
| const hasFooBarAccessor = await element.evaluate(node => { | ||
| const desc = Object.getOwnPropertyDescriptor( | ||
| Object.getPrototypeOf(node), | ||
| "foo-bar", | ||
| ); | ||
| return typeof desc?.get === "function"; | ||
| }); | ||
|
|
||
| expect(hasFooBarAccessor).toBeTruthy(); | ||
| }); | ||
|
|
||
| test("should use binding key as-is for both attribute and property name", async ({ | ||
| page, | ||
| }) => { | ||
| const element = page.locator("attribute-map-test-element"); | ||
|
|
||
| // Setting the foo-bar attribute should update the foo-bar property (no conversion) | ||
| await element.evaluate(node => node.setAttribute("foo-bar", "dash-case-test")); | ||
| const propValue = await element.evaluate(node => (node as any)["foo-bar"]); | ||
|
|
||
| expect(propValue).toBe("dash-case-test"); | ||
| }); | ||
|
|
||
| test("should not define @attr for event handler methods", async ({ page }) => { | ||
| const element = page.locator("attribute-map-test-element"); | ||
|
|
||
| // @click="{setFoo()}" etc. produce "event" type bindings β excluded from schema. | ||
| // Regular methods have a value descriptor, not a getter/setter. | ||
| const results = await element.evaluate(node => { | ||
| const proto = Object.getPrototypeOf(node); | ||
| const isAccessor = (name: string) => { | ||
| const desc = Object.getOwnPropertyDescriptor(proto, name); | ||
| return typeof desc?.get === "function"; | ||
| }; | ||
| return { | ||
| setFoo: isAccessor("setFoo"), | ||
| setFooBar: isAccessor("setFooBar"), | ||
| setMultiple: isAccessor("setMultiple"), | ||
| }; | ||
| }); | ||
|
|
||
| expect(results.setFoo).toBe(false); | ||
| expect(results.setFooBar).toBe(false); | ||
| expect(results.setMultiple).toBe(false); | ||
| }); | ||
|
|
||
| test("should update template when attribute is set via setAttribute", async ({ | ||
| page, | ||
| }) => { | ||
| const element = page.locator("attribute-map-test-element"); | ||
|
|
||
| await element.evaluate(node => node.setAttribute("foo", "attr-value")); | ||
|
|
||
| await expect(page.locator(".foo-value")).toHaveText("attr-value"); | ||
| }); | ||
|
|
||
| test("should update template when dash-case attribute is set via setAttribute", async ({ | ||
| page, | ||
| }) => { | ||
| const element = page.locator("attribute-map-test-element"); | ||
|
|
||
| await element.evaluate(node => node.setAttribute("foo-bar", "bar-attr-value")); | ||
|
|
||
| await expect(page.locator(".foo-bar-value")).toHaveText("bar-attr-value"); | ||
| }); | ||
|
|
||
| test("should reflect property value back to attribute", async ({ page }) => { | ||
| const element = page.locator("attribute-map-test-element"); | ||
|
|
||
| await element.evaluate(node => { | ||
| (node as any).foo = "reflected"; | ||
| }); | ||
|
|
||
| // FAST reflects attributes asynchronously via Updates.enqueue | ||
| await page.evaluate(() => new Promise(r => requestAnimationFrame(r))); | ||
|
|
||
| const attrValue = await element.evaluate(node => node.getAttribute("foo")); | ||
| expect(attrValue).toBe("reflected"); | ||
| }); | ||
|
|
||
| test("should update definition attributeLookup for simple properties", async ({ | ||
| page, | ||
| }) => { | ||
| const element = page.locator("attribute-map-test-element"); | ||
|
|
||
| // setAttribute triggers attributeChangedCallback via attributeLookup | ||
| await element.evaluate(node => node.setAttribute("foo", "lookup-test")); | ||
| const propValue = await element.evaluate(node => (node as any).foo); | ||
|
|
||
| expect(propValue).toBe("lookup-test"); | ||
| }); | ||
|
|
||
| test("should update definition attributeLookup for dash-case properties", async ({ | ||
| page, | ||
| }) => { | ||
| const element = page.locator("attribute-map-test-element"); | ||
|
|
||
| // setAttribute with foo-bar triggers attributeChangedCallback for the foo-bar property | ||
| await element.evaluate(node => node.setAttribute("foo-bar", "lookup-bar-test")); | ||
| const propValue = await element.evaluate(node => (node as any)["foo-bar"]); | ||
|
|
||
| expect(propValue).toBe("lookup-bar-test"); | ||
| }); | ||
|
|
||
| test("should not overwrite an existing @attr accessor", async ({ page }) => { | ||
| await page.waitForSelector("attribute-map-existing-attr-test-element"); | ||
| const element = page.locator("attribute-map-existing-attr-test-element"); | ||
|
|
||
| // The @attr default value must survive AttributeMap processing | ||
| const defaultValue = await element.evaluate(node => (node as any).foo); | ||
| expect(defaultValue).toBe("original"); | ||
|
|
||
| // setAttribute must still work via the original @attr definition | ||
| await element.evaluate(node => node.setAttribute("foo", "updated")); | ||
| const updatedValue = await element.evaluate(node => (node as any).foo); | ||
| expect(updatedValue).toBe("updated"); | ||
| }); | ||
| }); |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| import type { FASTElementDefinition } from "@microsoft/fast-element"; | ||
| import { AttributeDefinition, Observable } from "@microsoft/fast-element"; | ||
| import type { Schema } from "./schema.js"; | ||
|
|
||
| /** | ||
| * AttributeMap provides functionality for detecting simple (leaf) properties in | ||
| * a generated JSON schema and defining them as @attr properties on a class prototype. | ||
| * | ||
| * A property is a candidate for @attr when its schema entry has no nested `properties`, | ||
| * no `type`, and no `anyOf` β i.e. it is a plain binding like {{foo}} or id="{{foo-bar}}". | ||
| * | ||
| * Attribute names are **not** normalized β the binding key as written in the template | ||
| * is used as both the attribute name and the property name. Because HTML attributes are | ||
| * case-insensitive, binding keys should be lowercase (optionally dash-separated). | ||
| * For example, {{foo-bar}} results in attribute `foo-bar` and property `foo-bar`. | ||
| */ | ||
| export class AttributeMap { | ||
| private schema: Schema; | ||
| private classPrototype: any; | ||
| private definition: FASTElementDefinition | undefined; | ||
|
|
||
| constructor(classPrototype: any, schema: Schema, definition?: FASTElementDefinition) { | ||
| this.classPrototype = classPrototype; | ||
| this.schema = schema; | ||
| this.definition = definition; | ||
| } | ||
|
|
||
| public defineProperties(): void { | ||
| const propertyNames = this.schema.getRootProperties(); | ||
| const existingAccessorNames = new Set( | ||
| Observable.getAccessors(this.classPrototype).map(a => a.name), | ||
| ); | ||
|
|
||
| for (const propertyName of propertyNames) { | ||
|
janechu marked this conversation as resolved.
|
||
| const propertySchema = this.schema.getSchema(propertyName); | ||
|
|
||
| // Only create @attr for leaf properties: | ||
| // - no nested properties (not a dot-syntax path) | ||
| // - no type (not an explicitly typed value like an array) | ||
| // - no anyOf (not a child element reference) | ||
| if ( | ||
| !propertySchema || | ||
| propertySchema.properties || | ||
| propertySchema.type || | ||
| propertySchema.anyOf | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
||
| // Skip if the property already has an accessor (from @attr or @observable) | ||
| if (existingAccessorNames.has(propertyName)) { | ||
| continue; | ||
| } | ||
|
|
||
| const attrDef = new AttributeDefinition( | ||
| this.classPrototype.constructor, | ||
| propertyName, | ||
| propertyName, | ||
| ); | ||
|
|
||
| Observable.defineProperty(this.classPrototype, attrDef); | ||
|
|
||
| // Mutate the existing observedAttributes array on the class. | ||
| // FAST's FASTElementDefinition sets observedAttributes via | ||
| // Reflect.defineProperty with a concrete array value (non-configurable, | ||
| // non-writable), so the descriptor cannot be replaced. However, the | ||
| // array itself is mutable, and pushing into it works because | ||
| // registry.define() β which causes the browser to snapshot | ||
| // observedAttributes β is called AFTER this method runs. | ||
| const existingObservedAttrs: string[] | undefined = ( | ||
| this.classPrototype.constructor as any | ||
| ).observedAttributes; | ||
| if ( | ||
| Array.isArray(existingObservedAttrs) && | ||
| !existingObservedAttrs.includes(propertyName) | ||
| ) { | ||
| existingObservedAttrs.push(propertyName); | ||
| } | ||
|
|
||
| if (this.definition) { | ||
| (this.definition.attributeLookup as Record<string, AttributeDefinition>)[ | ||
| propertyName | ||
| ] = attrDef; | ||
| (this.definition.propertyLookup as Record<string, AttributeDefinition>)[ | ||
| propertyName | ||
| ] = attrDef; | ||
|
|
||
| const attrs = (this.definition as any).attributes; | ||
| if ( | ||
| Array.isArray(attrs) && | ||
| !attrs.some( | ||
| (existing: AttributeDefinition) => | ||
| existing.name === attrDef.name || | ||
| existing.attribute === attrDef.attribute, | ||
| ) | ||
| ) { | ||
| attrs.push(attrDef); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,11 @@ | ||
| export { AttributeMap } from "./attribute-map.js"; | ||
| export { RenderableFASTElement } from "./element.js"; | ||
| export { ObserverMap } from "./observer-map.js"; | ||
| export { | ||
| ObserverMapOption, | ||
| TemplateElement, | ||
| AttributeMapOption, | ||
| type ElementOptions, | ||
| type ElementOptionsDictionary, | ||
| type HydrationLifecycleCallbacks, | ||
| ObserverMapOption, | ||
| TemplateElement, | ||
| } from "./template.js"; |
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.