diff --git a/change/@microsoft-fast-element-ab7cde20-cff1-47a9-8e95-5895dd961321.json b/change/@microsoft-fast-element-ab7cde20-cff1-47a9-8e95-5895dd961321.json new file mode 100644 index 00000000000..86b747c61b9 --- /dev/null +++ b/change/@microsoft-fast-element-ab7cde20-cff1-47a9-8e95-5895dd961321.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Ignore stale observer map array notifications after replacement.", + "packageName": "@microsoft/fast-element", + "email": "pradeepramolaa@gmail.com", + "dependentChangeType": "none" +} diff --git a/packages/fast-element/src/declarative/observer-map-utilities.ts b/packages/fast-element/src/declarative/observer-map-utilities.ts index caa4c1515b5..4602ad65047 100644 --- a/packages/fast-element/src/declarative/observer-map-utilities.ts +++ b/packages/fast-element/src/declarative/observer-map-utilities.ts @@ -12,15 +12,26 @@ interface ObservedTargetsAndProperties { rootProperty: string; } +interface ArrayObserverRegistration { + target: any; + rootProperty: string; + schema: JSONSchema | JSONSchemaDefinition; + rootSchema: JSONSchema; + reachableArray: any; + subscriber: { + handleChange(subject: any, args: any[]): void; + }; +} + /** * A map of proxied objects. */ const objectTargetsMap = new WeakMap(); /** - * A map of arrays being observed. + * A map of arrays to their owner-specific observer registrations. */ -const observedArraysMap = new WeakSet(); +const observedArraysMap = new WeakMap(); type DataType = "array" | "object" | "primitive"; @@ -35,6 +46,10 @@ function getDataType(data: any): DataType { return "primitive"; } +function isObjectLike(value: any): value is object { + return typeof value === "object" && value !== null; +} + /** * Get properties from an anyOf array * @param anyOf - The anyOf array in a JSON schema @@ -104,7 +119,6 @@ function isSchemaExcluded(schema: any): boolean { return schema?.$observe === false && !hasObservedSchemaDescendant(schema); } - function defineObservableProperty( targetObject: any, key: string, @@ -156,6 +170,181 @@ function findArrayItemDef(schema: JSONSchema | JSONSchemaDefinition): string | n return findDef(schema) ?? (items === undefined ? null : findDef(items)); } +function isReachableFromRoot(root: any, value: any): boolean { + if (root === value) { + return true; + } + + if (!isObjectLike(root) || !isObjectLike(value)) { + return false; + } + + const seen = new WeakSet(); + const stack = [root]; + + while (stack.length > 0) { + const current = stack.pop(); + + if (current === value) { + return true; + } + + if (!isObjectLike(current) || seen.has(current)) { + continue; + } + + seen.add(current); + + for (const key of Object.keys(current)) { + if (key === "$fastController") { + continue; + } + + stack.push(current[key]); + } + } + + return false; +} + +function isArrayRegistrationReachable( + registration: ArrayObserverRegistration, + subject: any, +): boolean { + const root = registration.target?.[registration.rootProperty]; + + return ( + isReachableFromRoot(root, subject) || + isReachableFromRoot(root, registration.reachableArray) + ); +} + +function isSameArrayObserverContext( + registration: ArrayObserverRegistration, + target: any, + rootProperty: string, + schema: JSONSchema | JSONSchemaDefinition, + rootSchema: JSONSchema, +): boolean { + return ( + registration.target === target && + registration.rootProperty === rootProperty && + registration.schema === schema && + registration.rootSchema === rootSchema + ); +} + +function handleArrayChange( + registration: ArrayObserverRegistration, + subject: any, + args: any[], +) { + if (!isArrayRegistrationReachable(registration, subject)) { + removeArrayRegistration(subject, registration); + return; + } + + const schemaProperties = getSchemaProperties(registration.schema); + + args.forEach((arg: any) => { + if (arg.addedCount > 0) { + if (schemaProperties) { + for (let i = arg.addedCount - 1; i >= 0; i--) { + const item = subject[arg.index + i]; + const originalItem = Object.assign({}, item); + + assignProxyToItemsInArray( + item, + originalItem, + registration.schema, + registration.rootSchema, + registration.target, + registration.rootProperty, + ); + + Object.assign(item, originalItem); + } + } + + Observable.notify(registration.target, registration.rootProperty); + } + }); +} + +function removeArrayRegistration( + data: any[], + registration: ArrayObserverRegistration, +): void { + const registrations = observedArraysMap.get(data); + + if (!registrations) { + return; + } + + const registrationIndex = registrations.indexOf(registration); + + if (registrationIndex === -1) { + return; + } + + registrations.splice(registrationIndex, 1); + + if (registrations.length === 0) { + observedArraysMap.delete(data); + } + + Promise.resolve().then(() => { + Observable.getNotifier(data).unsubscribe(registration.subscriber); + }); +} + +function observeArray( + data: any[], + reachableArray: any, + schema: JSONSchema | JSONSchemaDefinition, + rootSchema: JSONSchema, + target: any, + rootProperty: string, +): void { + let registrations = observedArraysMap.get(data); + + if (!registrations) { + registrations = []; + observedArraysMap.set(data, registrations); + } + + const existingRegistration = registrations.find(registration => + isSameArrayObserverContext( + registration, + target, + rootProperty, + schema, + rootSchema, + ), + ); + + if (existingRegistration) { + existingRegistration.reachableArray = reachableArray; + return; + } + + const registration: ArrayObserverRegistration = { + target, + rootProperty, + schema, + rootSchema, + reachableArray, + subscriber: { + handleChange(subject, args) { + handleArrayChange(registration, subject, args); + }, + }, + }; + + registrations.push(registration); + Observable.getNotifier(data).subscribe(registration.subscriber); +} + /** * Assigns Observable properties to items in an array and sets up change notifications * @param proxiedData - The array data to make observable @@ -193,40 +382,8 @@ function assignObservablesToArray( }) : proxiedData; - if (!observedArraysMap.has(data)) { - observedArraysMap.add(data); - - Observable.getNotifier(data).subscribe({ - handleChange(subject, args) { - args.forEach((arg: any) => { - if (arg.addedCount > 0) { - if (schemaProperties) { - for (let i = arg.addedCount - 1; i >= 0; i--) { - const item = subject[arg.index + i]; - const originalItem = Object.assign({}, item); - - assignProxyToItemsInArray( - item, - originalItem, - schema, - rootSchema, - target, - rootProperty, - ); - - Object.assign(item, originalItem); - } - } - - // Notify observers of the target object's root property - Observable.notify(target, rootProperty); - } - }); - }, - }); - } - if (schemaProperties !== null) { + observeArray(data, data, schema, rootSchema, target, rootProperty); return data; } @@ -236,7 +393,7 @@ function assignObservablesToArray( // their items are individually proxied, and FAST's own push/splice/etc. // already carry splice records — double-wrapping would produce duplicate // splice notifications. - return new Proxy(data, { + const primitiveArrayProxy = new Proxy(data, { set: (arr: any, prop: string | symbol, value: any) => { const idx = typeof prop === "string" ? Number(prop) : NaN; @@ -251,6 +408,10 @@ function assignObservablesToArray( return true; }, }); + + observeArray(data, primitiveArrayProxy, schema, rootSchema, target, rootProperty); + + return primitiveArrayProxy; } /** diff --git a/packages/fast-element/test/declarative/fixtures/extensions/observer-map-deep-merge/main.ts b/packages/fast-element/test/declarative/fixtures/extensions/observer-map-deep-merge/main.ts index ecca41f9a1f..42fefda9ec4 100644 --- a/packages/fast-element/test/declarative/fixtures/extensions/observer-map-deep-merge/main.ts +++ b/packages/fast-element/test/declarative/fixtures/extensions/observer-map-deep-merge/main.ts @@ -3,7 +3,7 @@ import { deepMerge } from "@microsoft/fast-element/declarative-utilities.js"; import { FASTElement } from "@microsoft/fast-element/fast-element.js"; import { enableHydration } from "@microsoft/fast-element/hydration.js"; import { Observable, observable } from "@microsoft/fast-element/observable.js"; -import { observerMap } from "@microsoft/fast-element/observer-map.js"; +import { observerMap, Schema } from "@microsoft/fast-element/observer-map.js"; import { Updates } from "@microsoft/fast-element/updates.js"; interface Product { @@ -41,6 +41,10 @@ interface User { }>; } +interface SharedArrayFixtureData { + items: string[]; +} + class DeepMergeTestElement extends FASTElement { users: User[] = [ { @@ -362,6 +366,232 @@ class DeepMergeTestElement extends FASTElement { }; } + private createProduct(id: number, name: string): Product { + return { + id, + name, + price: 10, + inStock: true, + tags: ["accessories"], + metadata: { + views: 1, + rating: 4, + }, + }; + } + + private createOrder(id: number, name: string = "Replacement") { + return { + id, + date: "2024-06-01", + total: 10, + items: [this.createProduct(id * 10, name)], + }; + } + + private createUser(id: number, name: string): User { + return { + id, + name, + email: `${name.toLowerCase().replace(/ /g, ".")}@example.com`, + profile: { + age: 30, + location: { + city: "Seattle", + country: "USA", + }, + preferences: { + theme: "light", + notifications: true, + }, + }, + orders: [this.createOrder(id * 100)], + }; + } + + public async mutateStaleOrdersAfterDeepMerge() { + const oldOrders = this.users[0].orders; + let notifications = 0; + const notifier = Observable.getNotifier(this); + const subscriber = { + handleChange() { + notifications++; + }, + }; + + notifier.subscribe(subscriber, "users"); + + this.updateUserOrders(); + await Updates.next(); + + notifications = 0; + oldOrders.push(this.createOrder(900, "Stale order")); + await Updates.next(); + + notifier.unsubscribe(subscriber, "users"); + + return { + notifications, + currentOrderCount: this.users[0].orders.length, + staleOrderCount: oldOrders.length, + }; + } + + public async mutateStaleNestedItemsAfterSecondDeepMerge() { + deepMerge(this.users[0], { + orders: [this.createOrder(901, "First replacement")], + }); + await Updates.next(); + + const oldItems = this.users[0].orders[0].items; + + deepMerge(this.users[0], { + orders: [this.createOrder(902, "Second replacement")], + }); + await Updates.next(); + + let notifications = 0; + const notifier = Observable.getNotifier(this); + const subscriber = { + handleChange() { + notifications++; + }, + }; + + notifier.subscribe(subscriber, "users"); + + oldItems.push(this.createProduct(9000, "Stale item")); + await Updates.next(); + + notifier.unsubscribe(subscriber, "users"); + + return { + notifications, + currentItemCount: this.users[0].orders[0].items.length, + staleItemCount: oldItems.length, + }; + } + + public async mutateStaleUsersAfterDirectAssignment() { + const oldUsers = this.users; + let notifications = 0; + const notifier = Observable.getNotifier(this); + const subscriber = { + handleChange() { + notifications++; + }, + }; + + notifier.subscribe(subscriber, "users"); + + this.users = [this.createUser(30, "Replacement User")]; + await Updates.next(); + + notifications = 0; + oldUsers.push(this.createUser(31, "Stale User")); + await Updates.next(); + + notifier.unsubscribe(subscriber, "users"); + + return { + notifications, + currentUserCount: this.users.length, + staleUserCount: oldUsers.length, + }; + } + + public async mutateStaleOrdersAfterProxyAssignment() { + const oldOrders = this.users[0].orders; + let notifications = 0; + const notifier = Observable.getNotifier(this); + const subscriber = { + handleChange() { + notifications++; + }, + }; + + notifier.subscribe(subscriber, "users"); + + this.users[0].orders = [this.createOrder(901, "Replacement order")]; + await Updates.next(); + + notifications = 0; + oldOrders.push(this.createOrder(902, "Stale proxy order")); + await Updates.next(); + + notifier.unsubscribe(subscriber, "users"); + + return { + notifications, + currentOrderCount: this.users[0].orders.length, + staleOrderCount: oldOrders.length, + }; + } + + public async testSharedArrayOwnerReplacement() { + const sharedItems = ["shared"]; + const ownerA = document.createElement( + "shared-array-owner-element", + ) as SharedArrayOwnerElement; + const ownerB = document.createElement( + "shared-array-owner-element", + ) as SharedArrayOwnerElement; + let ownerANotifications = 0; + let ownerBNotifications = 0; + const ownerANotifier = Observable.getNotifier(ownerA); + const ownerBNotifier = Observable.getNotifier(ownerB); + const ownerASubscriber = { + handleChange() { + ownerANotifications++; + }, + }; + const ownerBSubscriber = { + handleChange() { + ownerBNotifications++; + }, + }; + + document.body.append(ownerA, ownerB); + ownerA.data = { + items: [], + }; + ownerB.data = { + items: [], + }; + await Updates.next(); + + ownerANotifier.subscribe(ownerASubscriber, "data"); + ownerBNotifier.subscribe(ownerBSubscriber, "data"); + + ownerA.data.items = sharedItems; + ownerB.data.items = sharedItems; + await Updates.next(); + + deepMerge(ownerA.data, { + items: ["replacement"], + }); + await Updates.next(); + + ownerANotifications = 0; + ownerBNotifications = 0; + + sharedItems.push("still owned by B"); + await Updates.next(); + + ownerANotifier.unsubscribe(ownerASubscriber, "data"); + ownerBNotifier.unsubscribe(ownerBSubscriber, "data"); + ownerA.remove(); + ownerB.remove(); + + return { + ownerANotifications, + ownerBNotifications, + ownerAItems: [...ownerA.data.items], + ownerBItems: [...ownerB.data.items], + sharedItems: [...sharedItems], + }; + } + public async replaceOrdersAndMutateNestedData() { this.updateUserOrders(); @@ -496,11 +726,37 @@ class DeepMergeTestElement extends FASTElement { } } +class SharedArrayOwnerElement extends FASTElement { + public data: SharedArrayFixtureData = { + items: [], + }; +} + +const sharedArrayOwnerSchema = new Schema("shared-array-owner-element"); +sharedArrayOwnerSchema.addPath({ + rootPropertyName: "data", + pathConfig: { + type: "repeat", + path: "data.items", + currentContext: "item", + parentContext: null, + }, + childrenMap: null, +}); + const hydration = enableHydration(); void hydration.whenHydrated().then(() => { (window as any).hydrationCompleted = true; }); +SharedArrayOwnerElement.define( + { + name: "shared-array-owner-element", + schema: sharedArrayOwnerSchema, + }, + [observerMap({ schema: sharedArrayOwnerSchema })], +); + DeepMergeTestElement.define( { name: "deep-merge-test-element", diff --git a/packages/fast-element/test/declarative/fixtures/extensions/observer-map-deep-merge/observer-map-deep-merge.spec.ts b/packages/fast-element/test/declarative/fixtures/extensions/observer-map-deep-merge/observer-map-deep-merge.spec.ts index 23a188fd16c..530ecd35b59 100644 --- a/packages/fast-element/test/declarative/fixtures/extensions/observer-map-deep-merge/observer-map-deep-merge.spec.ts +++ b/packages/fast-element/test/declarative/fixtures/extensions/observer-map-deep-merge/observer-map-deep-merge.spec.ts @@ -352,6 +352,100 @@ test.describe("Deep Merge Test Fixture", () => { expect(result.duplicateAccessors).toEqual([]); }); + test("should ignore stale array mutations after replacing arrays with deepMerge", async ({ + page, + }) => { + const hydrationCompleted = page.waitForFunction( + () => (window as any).hydrationCompleted === true, + ); + await page.goto("/fixtures/extensions/observer-map-deep-merge/"); + await hydrationCompleted; + + const result = await page + .locator("deep-merge-test-element") + .evaluate((element: any) => element.mutateStaleOrdersAfterDeepMerge()); + + expect(result.notifications).toBe(0); + expect(result.currentOrderCount).toBe(1); + expect(result.staleOrderCount).toBe(3); + }); + + test("should ignore stale nested array mutations after replacing arrays with deepMerge", async ({ + page, + }) => { + const hydrationCompleted = page.waitForFunction( + () => (window as any).hydrationCompleted === true, + ); + await page.goto("/fixtures/extensions/observer-map-deep-merge/"); + await hydrationCompleted; + + const result = await page + .locator("deep-merge-test-element") + .evaluate((element: any) => + element.mutateStaleNestedItemsAfterSecondDeepMerge(), + ); + + expect(result.notifications).toBe(0); + expect(result.currentItemCount).toBe(1); + expect(result.staleItemCount).toBe(2); + }); + + test("should ignore stale root array mutations after direct property assignment", async ({ + page, + }) => { + const hydrationCompleted = page.waitForFunction( + () => (window as any).hydrationCompleted === true, + ); + await page.goto("/fixtures/extensions/observer-map-deep-merge/"); + await hydrationCompleted; + + const result = await page + .locator("deep-merge-test-element") + .evaluate((element: any) => element.mutateStaleUsersAfterDirectAssignment()); + + expect(result.notifications).toBe(0); + expect(result.currentUserCount).toBe(1); + expect(result.staleUserCount).toBe(3); + }); + + test("should ignore stale array mutations after proxy assignment replacement", async ({ + page, + }) => { + const hydrationCompleted = page.waitForFunction( + () => (window as any).hydrationCompleted === true, + ); + await page.goto("/fixtures/extensions/observer-map-deep-merge/"); + await hydrationCompleted; + + const result = await page + .locator("deep-merge-test-element") + .evaluate((element: any) => element.mutateStaleOrdersAfterProxyAssignment()); + + expect(result.notifications).toBe(0); + expect(result.currentOrderCount).toBe(1); + expect(result.staleOrderCount).toBe(3); + }); + + test("should keep shared arrays notifying owners that still reference them", async ({ + page, + }) => { + const hydrationCompleted = page.waitForFunction( + () => (window as any).hydrationCompleted === true, + ); + await page.goto("/fixtures/extensions/observer-map-deep-merge/"); + await hydrationCompleted; + + const result = await page + .locator("deep-merge-test-element") + .evaluate((element: any) => element.testSharedArrayOwnerReplacement()); + + expect(result.ownerANotifications).toBe(0); + expect(result.ownerBNotifications).toBe(1); + expect(result.ownerAItems).toEqual(["replacement"]); + expect(result.ownerBItems).toEqual(["shared", "still owned by B"]); + expect(result.sharedItems).toEqual(["shared", "still owned by B"]); + }); + test("should avoid reentrant observerMap deepMerge array changes during notification", async ({ page, }) => {