Skip to content

Commit 1dd91d2

Browse files
janechuCopilot
andcommitted
refactor(fast-html): address PR review comments for AttributeMap
- Remove async from test.describe callbacks in both spec files (Playwright requires synchronous describe callbacks) - Convert existingAccessors to a Set for O(1) lookup instead of O(N) - Update definition.attributes array alongside attributeLookup and propertyLookup to keep the definition's canonical attribute list in sync - Add .define() call to attributeMap README usage snippet - Add comment explaining why observedAttributes mutation via push is safe (FAST uses a concrete array, and registry.define() runs after this method) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent c31bc76 commit 1dd91d2

4 files changed

Lines changed: 26 additions & 10 deletions

File tree

packages/fast-html/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ TemplateElement.options({
230230
"my-element": {
231231
attributeMap: "all",
232232
},
233-
});
233+
}).define({ name: "f-template" });
234234
```
235235

236236
With the template:

packages/fast-html/src/components/attribute-map.spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { expect, test } from "@playwright/test";
22

3-
test.describe("AttributeMap", async () => {
3+
test.describe("AttributeMap", () => {
44
test.beforeEach(async ({ page }) => {
55
await page.goto("/fixtures/attribute-map/");
66
await page.waitForSelector("attribute-map-test-element");

packages/fast-html/src/components/attribute-map.ts

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,9 @@ export class AttributeMap {
2727

2828
public defineProperties(): void {
2929
const propertyNames = this.schema.getRootProperties();
30-
const existingAccessors = Observable.getAccessors(this.classPrototype);
30+
const existingAccessorNames = new Set(
31+
Observable.getAccessors(this.classPrototype).map(a => a.name),
32+
);
3133

3234
for (const propertyName of propertyNames) {
3335
const propertySchema = this.schema.getSchema(propertyName);
@@ -46,7 +48,7 @@ export class AttributeMap {
4648
}
4749

4850
// Skip if the property already has an accessor (from @attr or @observable)
49-
if (existingAccessors.some(accessor => accessor.name === propertyName)) {
51+
if (existingAccessorNames.has(propertyName)) {
5052
continue;
5153
}
5254

@@ -58,11 +60,13 @@ export class AttributeMap {
5860

5961
Observable.defineProperty(this.classPrototype, attrDef);
6062

61-
// Push to the existing observedAttributes array on the class.
62-
// For all f-template-registered elements, registry.define() (which causes
63-
// the browser to cache observedAttributes) is called AFTER this method runs.
64-
// Mutating the existing array reference ensures the browser observes these
65-
// attributes, enabling setAttribute() → attributeChangedCallback() → template update.
63+
// Mutate the existing observedAttributes array on the class.
64+
// FAST's FASTElementDefinition sets observedAttributes via
65+
// Reflect.defineProperty with a concrete array value (non-configurable,
66+
// non-writable), so the descriptor cannot be replaced. However, the
67+
// array itself is mutable, and pushing into it works because
68+
// registry.define() — which causes the browser to snapshot
69+
// observedAttributes — is called AFTER this method runs.
6670
const existingObservedAttrs: string[] | undefined = (
6771
this.classPrototype.constructor as any
6872
).observedAttributes;
@@ -80,6 +84,18 @@ export class AttributeMap {
8084
(this.definition.propertyLookup as Record<string, AttributeDefinition>)[
8185
propertyName
8286
] = attrDef;
87+
88+
const attrs = (this.definition as any).attributes;
89+
if (
90+
Array.isArray(attrs) &&
91+
!attrs.some(
92+
(existing: AttributeDefinition) =>
93+
existing.name === attrDef.name ||
94+
existing.attribute === attrDef.attribute,
95+
)
96+
) {
97+
attrs.push(attrDef);
98+
}
8399
}
84100
}
85101
}

packages/fast-html/test/fixtures/attribute-map/attribute-map.spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { expect, test } from "@playwright/test";
22

3-
test.describe("AttributeMap", async () => {
3+
test.describe("AttributeMap", () => {
44
test.beforeEach(async ({ page }) => {
55
await page.goto("/fixtures/attribute-map/");
66
await page.waitForSelector("attribute-map-test-element");

0 commit comments

Comments
 (0)