Fix potentially wrong order of RemoteMutationRecords

Author: mfal

.changeset/kind-sloths-hope.md

@@ -0,0 +1,5 @@
+---
+'@remote-dom/core': patch
+---
+
+fix: Fix errors when lists of remote elements change

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<string, number>();
 
       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

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<Node, RemoteConnection>();
 
@@ -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;
+}

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;

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],
+      );
+  });
+});

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;
     }
   }
 }