Fix potentially wrong order of RemoteMutationRecords

.changeset/kind-sloths-hope.md

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

packages/core/package.json

@@ -1,13 +1,13 @@
 {
-  "name": "@remote-dom/core",
+  "name": "@mfalkenberg/remote-dom-core",
   "type": "module",
   "license": "MIT",
   "description": "A collection of DOM-based utilities for synchronizing elements between JavaScript environments",
   "publishConfig": {
     "access": "public",
     "@remote-dom/registry": "https://registry.npmjs.org"
   },
-  "version": "1.5.2",
+  "version": "1.5.6",
   "engines": {
     "node": ">=14.0.0"
   },
@@ -70,6 +70,7 @@
       ]
     }
   },
+  "files": ["build", "source"],
   "sideEffects": [
     "./source/polyfill.ts",
     "./build/cjs/polyfill.cjs",
@@ -80,7 +81,7 @@
     "build": "rollup --config ./rollup.config.js"
   },
   "dependencies": {
-    "@remote-dom/polyfill": "workspace:^1.4.2",
+    "@remote-dom/polyfill": "^1.4.2",
     "htm": "^3.1.1"
   },
   "peerDependencies": {

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/core/source/receivers/RemoteReceiver.ts

@@ -61,10 +61,6 @@ export interface RemoteReceiverRoot {
   readonly type: typeof NODE_TYPE_ROOT;
   readonly children: readonly RemoteReceiverNode[];
   readonly properties: NonNullable<RemoteElementSerialization['properties']>;
-  readonly attributes: NonNullable<RemoteElementSerialization['attributes']>;
-  readonly eventListeners: NonNullable<
-    RemoteElementSerialization['eventListeners']
-  >;
   readonly version: number;
 }
 
@@ -106,8 +102,6 @@ export class RemoteReceiver {
     children: [],
     version: 0,
     properties: {},
-    attributes: {},
-    eventListeners: {},
   };
 
   /**

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 = node!.parentNode;
+      currentNode = currentNode.parentNode;
     }
   }
 }

packages/signals/source/SignalRemoteReceiver.ts

@@ -72,12 +72,6 @@ export interface SignalRemoteReceiverRoot {
   readonly properties: ReadonlySignal<
     NonNullable<RemoteElementSerialization['properties']>
   >;
-  readonly attributes: ReadonlySignal<
-    NonNullable<RemoteElementSerialization['attributes']>
-  >;
-  readonly eventListeners: ReadonlySignal<
-    NonNullable<RemoteElementSerialization['eventListeners']>
-  >;
   readonly children: ReadonlySignal<readonly SignalRemoteReceiverNode[]>;
 }
 
@@ -117,8 +111,6 @@ export class SignalRemoteReceiver {
     id: ROOT_ID,
     type: NODE_TYPE_ROOT,
     properties: signal({}),
-    attributes: signal({}),
-    eventListeners: signal({}),
     children: signal([]),
   };