From 96f76354f49385d699823ce865049b2840d95c1c Mon Sep 17 00:00:00 2001 From: Marco Falkenberg Date: Thu, 19 Dec 2024 16:05:57 +0100 Subject: [PATCH] Fix potentially wrong order of RemoteMutationRecords --- .changeset/kind-sloths-hope.md | 5 + .../source/elements/RemoteMutationObserver.ts | 30 +++++ packages/core/source/elements/internals.ts | 16 ++- .../elements/tests/MutationObserverMock.ts | 14 +++ .../tests/RemoteMutationObserver.test.ts | 117 ++++++++++++++++++ packages/polyfill/source/Node.ts | 2 +- 6 files changed, 182 insertions(+), 2 deletions(-) create mode 100644 .changeset/kind-sloths-hope.md create mode 100644 packages/core/source/elements/tests/MutationObserverMock.ts create mode 100644 packages/core/source/elements/tests/RemoteMutationObserver.test.ts diff --git a/.changeset/kind-sloths-hope.md b/.changeset/kind-sloths-hope.md new file mode 100644 index 00000000..4526c6bf --- /dev/null +++ b/.changeset/kind-sloths-hope.md @@ -0,0 +1,5 @@ +--- +'@remote-dom/core': patch +--- + +fix: Fix errors when lists of remote elements change diff --git a/packages/core/source/elements/RemoteMutationObserver.ts b/packages/core/source/elements/RemoteMutationObserver.ts index 518d949f..0a787f06 100644 --- a/packages/core/source/elements/RemoteMutationObserver.ts +++ b/packages/core/source/elements/RemoteMutationObserver.ts @@ -4,6 +4,7 @@ import { disconnectRemoteNode, serializeRemoteNode, REMOTE_IDS, + getStructuralMutationIndex, } from './internals.ts'; import { ROOT_ID, @@ -36,10 +37,14 @@ export class RemoteMutationObserver extends MutationObserver { super((records) => { const addedNodes: Node[] = []; const remoteRecords: RemoteMutationRecord[] = []; + const netStructuralMutations = new Map(); for (const record of records) { const targetId = remoteId(record.target); + const targetsNetStructuralMutations = + netStructuralMutations.get(targetId) ?? 0; + if (record.type === 'childList') { const position = record.previousSibling ? indexOf(record.previousSibling, record.target.childNodes) + 1 @@ -48,6 +53,10 @@ export class RemoteMutationObserver extends MutationObserver { record.removedNodes.forEach((node) => { disconnectRemoteNode(node); + netStructuralMutations.set( + targetId, + targetsNetStructuralMutations + 1, + ); remoteRecords.push([ MUTATION_TYPE_REMOVE_CHILD, targetId, @@ -72,6 +81,10 @@ export class RemoteMutationObserver extends MutationObserver { addedNodes.push(node); connectRemoteNode(node, connection); + netStructuralMutations.set( + targetId, + targetsNetStructuralMutations - 1, + ); remoteRecords.push([ MUTATION_TYPE_INSERT_CHILD, targetId, @@ -100,10 +113,27 @@ export class RemoteMutationObserver extends MutationObserver { } } + const hasCompensatedStructuralMutations = Array.from( + netStructuralMutations.values(), + ).includes(0); + + if (hasCompensatedStructuralMutations) { + this.sortStructuralMutations(remoteRecords); + } + connection.mutate(remoteRecords); }); } + /** + * See this issue why sorting is required: https://github.com/Shopify/remote-dom/issues/519 + */ + private sortStructuralMutations(records: RemoteMutationRecord[]) { + records.sort( + (l, r) => getStructuralMutationIndex(l) - getStructuralMutationIndex(r), + ); + } + /** * Starts watching changes to the element, and communicates changes to the * host environment. By default, this method will also communicate any initial diff --git a/packages/core/source/elements/internals.ts b/packages/core/source/elements/internals.ts index 8a680846..ef78466c 100644 --- a/packages/core/source/elements/internals.ts +++ b/packages/core/source/elements/internals.ts @@ -3,8 +3,14 @@ import { UPDATE_PROPERTY_TYPE_PROPERTY, UPDATE_PROPERTY_TYPE_ATTRIBUTE, UPDATE_PROPERTY_TYPE_EVENT_LISTENER, + MUTATION_TYPE_INSERT_CHILD, + MUTATION_TYPE_REMOVE_CHILD, } from '../constants.ts'; -import type {RemoteConnection, RemoteNodeSerialization} from '../types.ts'; +import type { + RemoteConnection, + RemoteMutationRecord, + RemoteNodeSerialization, +} from '../types.ts'; export const REMOTE_CONNECTIONS = new WeakMap(); @@ -301,3 +307,11 @@ export function callRemoteElementMethod( return connection.call(id, method, ...args); } + +export function getStructuralMutationIndex(record: RemoteMutationRecord) { + return record[0] === MUTATION_TYPE_INSERT_CHILD + ? record[3] + : record[0] === MUTATION_TYPE_REMOVE_CHILD + ? record[2] + : -1; +} diff --git a/packages/core/source/elements/tests/MutationObserverMock.ts b/packages/core/source/elements/tests/MutationObserverMock.ts new file mode 100644 index 00000000..82e09267 --- /dev/null +++ b/packages/core/source/elements/tests/MutationObserverMock.ts @@ -0,0 +1,14 @@ +import {vi} from 'vitest'; + +export class MutationObserverMock { + public readonly emitMutation = vi.fn<[MutationRecord[]], void>(); + + public constructor(cb: MutationCallback) { + this.emitMutation.mockImplementation((records) => cb(records, {} as never)); + } +} + +type MutationObserver = typeof globalThis.MutationObserver; + +globalThis.MutationObserver = + MutationObserverMock as unknown as MutationObserver; diff --git a/packages/core/source/elements/tests/RemoteMutationObserver.test.ts b/packages/core/source/elements/tests/RemoteMutationObserver.test.ts new file mode 100644 index 00000000..8f96606b --- /dev/null +++ b/packages/core/source/elements/tests/RemoteMutationObserver.test.ts @@ -0,0 +1,117 @@ +import '../../polyfill'; +import './MutationObserverMock'; +import {MutationObserverMock} from './MutationObserverMock'; +import {beforeEach, describe, expect, it, vi} from 'vitest'; +import {RemoteMutationObserver} from '../RemoteMutationObserver'; +import { + MUTATION_TYPE_INSERT_CHILD, + MUTATION_TYPE_REMOVE_CHILD, +} from '../../constants'; + +let observer: RemoteMutationObserver & MutationObserverMock; +let observedRoot: HTMLDivElement; + +let node1: Node; +let node2: Node; +let node3: Node; + +const mockedConnection = { + mutate: vi.fn(), + call: vi.fn(), +}; + +beforeEach(() => { + vi.resetAllMocks(); + + observer = new RemoteMutationObserver( + mockedConnection, + ) as RemoteMutationObserver & MutationObserverMock; + + observedRoot = div(); + node1 = div(); + node2 = div(); + node3 = div(); +}); + +interface TestMutation { + addedNodes?: Node[]; + removedNodes?: Node[]; + previousSibling?: Node; +} + +function createMutationRecord(mutation: TestMutation): MutationRecord { + const {addedNodes = [], removedNodes = [], previousSibling} = mutation; + + return { + type: 'childList', + addedNodes: addedNodes as never, + removedNodes: removedNodes as never, + previousSibling: previousSibling ?? null, + target: observedRoot, + attributeName: null, + attributeNamespace: null, + nextSibling: null, + oldValue: null, + }; +} + +const div = () => document.createElement('div'); + +const insert = (node: Node, after?: Node) => + createMutationRecord({ + addedNodes: [node], + previousSibling: after, + }); + +const remove = (node: Node, after?: Node) => + createMutationRecord({ + removedNodes: [node], + previousSibling: after, + }); + +const after = (after: Node) => ({ + remove: (node: Node) => remove(node, after), + insert: (node: Node) => insert(node, after), +}); + +const atStart = { + remove: (node: Node) => remove(node), + insert: (node: Node) => insert(node), +}; + +function givenFinalNodes(...nodes: Node[]) { + type ExpectedRemoteMutation = [type: number, index: number]; + + observedRoot.append(...nodes); + + return { + createdByMutations(...mutations: MutationRecord[]) { + observer.emitMutation(mutations); + + return { + expectRemoteMutations(...expected: ExpectedRemoteMutation[]) { + const remoteMutations = expected.map(([type, index]) => + type === MUTATION_TYPE_INSERT_CHILD + ? [type, expect.anything(), expect.anything(), index] + : [type, expect.anything(), index], + ); + + expect(mockedConnection.mutate).toHaveBeenCalledWith([ + ...remoteMutations, + ]); + }, + }; + }, + }; +} + +describe('RemoteMutationObserver', () => { + it('orders structural mutations by their index', () => { + givenFinalNodes(node1, node2) + .createdByMutations(after(node2).remove(node3), atStart.insert(node1)) + .expectRemoteMutations( + [MUTATION_TYPE_INSERT_CHILD, 0], + [MUTATION_TYPE_REMOVE_CHILD, 2], + ); + }); +}); diff --git a/packages/polyfill/source/Node.ts b/packages/polyfill/source/Node.ts index 2b275244..864f4922 100644 --- a/packages/polyfill/source/Node.ts +++ b/packages/polyfill/source/Node.ts @@ -160,7 +160,7 @@ export class Node extends EventTarget { while (true) { if (currentNode == null) return false; if (currentNode === this) return true; - currentNode = currentNode!.parentNode; + currentNode = currentNode.parentNode; } } }