diff --git a/.changeset/clever-glasses-hug.md b/.changeset/clever-glasses-hug.md new file mode 100644 index 00000000..b80857ed --- /dev/null +++ b/.changeset/clever-glasses-hug.md @@ -0,0 +1,9 @@ +--- +'example-kitchen-sink': patch +'@remote-dom/polyfill': patch +'@remote-dom/signals': patch +'@remote-dom/compat': patch +'@remote-dom/core': patch +--- + +Fix elements are inserted or removed at incorrect positions, leading to UI inconsistencies. diff --git a/e2e/mutations.e2e.ts b/e2e/mutations.e2e.ts new file mode 100644 index 00000000..ef94313d --- /dev/null +++ b/e2e/mutations.e2e.ts @@ -0,0 +1,18 @@ +import {expect, test} from '@playwright/test'; + +const testSet: Array<[string, string, string]> = [ + ['iframe', 'react-mutations-1', 'Data: 1\nData: 2\nData: 3\nData: 4'], + ['iframe', 'react-mutations-2', 'Data: 1\nData: 2\nData: 3\nData: 4'], + ['iframe', 'react-mutations-3', 'Data: 1\nData: 2'], +]; + +testSet.forEach(([sandbox, example, expectedText]) => { + test(`mutations are applied correctly with ${sandbox} sandbox and ${example} example`, async ({ + page, + }) => { + await page.goto(`/?sandbox=${sandbox}&example=${example}`); + await expect(page.getByTestId('test-done')).toBeAttached(); + const testStack = page.getByTestId('test-stack'); + expect(await testStack.innerText()).toBe(expectedText); + }); +}); diff --git a/examples/kitchen-sink/app/host/components.tsx b/examples/kitchen-sink/app/host/components.tsx index d12829e0..e24f4dd4 100644 --- a/examples/kitchen-sink/app/host/components.tsx +++ b/examples/kitchen-sink/app/host/components.tsx @@ -51,11 +51,13 @@ export function Button({ } export function Stack({ + testId, spacing, children, }: {children?: ComponentChildren} & StackProperties) { return (
{children} @@ -160,6 +162,9 @@ export function ControlPanel({ + + + @@ -194,6 +199,9 @@ const EXAMPLE_FILE_NAMES = new Map([ ['htm', 'htm.ts'], ['preact', 'preact.tsx'], ['react', 'react.tsx'], + ['react-mutations-1', 'react-mutations.tsx'], + ['react-mutations-2', 'react-mutations.tsx'], + ['react-mutations-3', 'react-mutations.tsx'], ['svelte', 'App.svelte'], ['vue', 'App.vue'], ]); diff --git a/examples/kitchen-sink/app/host/state.ts b/examples/kitchen-sink/app/host/state.ts index 2d4bae57..4de6f46f 100644 --- a/examples/kitchen-sink/app/host/state.ts +++ b/examples/kitchen-sink/app/host/state.ts @@ -14,6 +14,9 @@ const ALLOWED_EXAMPLE_VALUES = new Set([ 'react', 'svelte', 'vue', + 'react-mutations-1', + 'react-mutations-2', + 'react-mutations-3', ]); export function createState( diff --git a/examples/kitchen-sink/app/remote/elements.ts b/examples/kitchen-sink/app/remote/elements.ts index 940dd067..11d72460 100644 --- a/examples/kitchen-sink/app/remote/elements.ts +++ b/examples/kitchen-sink/app/remote/elements.ts @@ -49,6 +49,7 @@ export const Modal = createRemoteElement< export const Stack = createRemoteElement({ properties: { spacing: {type: Boolean}, + testId: {type: String}, }, }); diff --git a/examples/kitchen-sink/app/remote/examples/react-mutations.tsx b/examples/kitchen-sink/app/remote/examples/react-mutations.tsx new file mode 100644 index 00000000..f16b9ae4 --- /dev/null +++ b/examples/kitchen-sink/app/remote/examples/react-mutations.tsx @@ -0,0 +1,110 @@ +/** @jsxRuntime automatic */ +/** @jsxImportSource react */ + +import {createRemoteComponent} from '@remote-dom/react'; +import {createRoot} from 'react-dom/client'; + +import {useEffect, useState} from 'react'; +import type {RenderAPI} from '../../types.ts'; +import {Stack as StackElement, Text as TextElement} from '../elements.ts'; +import {useRenders} from './utils/react-hooks.ts'; + +const Stack = createRemoteComponent('ui-stack', StackElement); +const Text = createRemoteComponent('ui-text', TextElement); + +const data1 = Data: 1; +const data2 = Data: 2; +const data3 = Data: 3; +const data4 = Data: 4; +const done = ; +const loading1 = Loading: 1; +const loading2 = Loading: 2; + +const Example1 = () => { + const renders = useRenders(2); + + return ( + + <> + {renders === 1 && loading1} + {renders === 2 && ( + <> + {data1} + {data2} + + )} + + <> + {renders === 1 && loading2} + {renders === 2 && ( + <> + {data3} + {data4} + + )} + + {renders === 2 && done} + + ); +}; + +const Example2 = () => { + const [loading, setLoading] = useState(true); + + useEffect(() => { + setLoading(false); + }, [setLoading]); + + return ( + + <> + {loading && loading1} + {!loading && ( + <> + {data1} + {data2} + + )} + + <> + {loading && loading2} + {!loading && ( + <> + {data3} + {data4} + + )} + + {!loading && done} + + ); +}; + +const Example3 = () => { + const renders = useRenders(2); + + return ( + + {renders === 2 && data1} + {data2} + {renders === 1 && data3} + {renders === 2 && done} + + ); +}; + +function App({api}: {api: RenderAPI}) { + const {example} = api; + + return example === 'react-mutations-1' ? ( + + ) : example === 'react-mutations-2' ? ( + + ) : example === 'react-mutations-3' ? ( + + ) : null; +} + +export function renderUsingReact(root: Element, api: RenderAPI) { + createRoot(root).render(); +} diff --git a/examples/kitchen-sink/app/remote/examples/utils/react-hooks.ts b/examples/kitchen-sink/app/remote/examples/utils/react-hooks.ts new file mode 100644 index 00000000..b91f583a --- /dev/null +++ b/examples/kitchen-sink/app/remote/examples/utils/react-hooks.ts @@ -0,0 +1,19 @@ +import {useEffect, useState} from 'react'; + +export const useRenders = (max: number) => { + const [renders, setRenders] = useState(1); + + useEffect(() => { + if (renders >= max) { + return; + } + + const timeout = setTimeout(() => { + setRenders((r) => r + 1); + }, 200); + + return () => clearTimeout(timeout); + }, [setRenders, renders]); + + return renders; +}; diff --git a/examples/kitchen-sink/app/remote/render.ts b/examples/kitchen-sink/app/remote/render.ts index 42647656..72e53911 100644 --- a/examples/kitchen-sink/app/remote/render.ts +++ b/examples/kitchen-sink/app/remote/render.ts @@ -22,6 +22,12 @@ export async function render(root: Element, api: RenderAPI) { const {renderUsingReact} = await import('./examples/react.tsx'); return renderUsingReact(root, api); } + case 'react-mutations-1': + case 'react-mutations-2': + case 'react-mutations-3': { + const {renderUsingReact} = await import('./examples/react-mutations.tsx'); + return renderUsingReact(root, api); + } case 'svelte': { const {renderUsingSvelte} = await import('./examples/svelte.ts'); return renderUsingSvelte(root, api); diff --git a/examples/kitchen-sink/app/types.ts b/examples/kitchen-sink/app/types.ts index ea78dd21..2b43cdcd 100644 --- a/examples/kitchen-sink/app/types.ts +++ b/examples/kitchen-sink/app/types.ts @@ -25,9 +25,13 @@ export type RenderExample = | 'htm' | 'preact' | 'react' + | 'react-mutations' | 'svelte' | 'vue' - | 'react-remote-ui'; + | 'react-remote-ui' + | 'react-mutations-1' + | 'react-mutations-2' + | 'react-mutations-3'; /** * The object that the “host” page will pass to the “remote” environment. This @@ -123,4 +127,6 @@ export interface StackProperties { * Whether children should have space between them. */ spacing?: boolean; + + testId?: string; } diff --git a/packages/compat/source/adapter/host.ts b/packages/compat/source/adapter/host.ts index ac91637d..8c639da1 100644 --- a/packages/compat/source/adapter/host.ts +++ b/packages/compat/source/adapter/host.ts @@ -70,7 +70,22 @@ export function adaptToLegacyRemoteChannel( connection: RemoteConnection, options?: LegacyRemoteChannelOptions, ): LegacyRemoteChannel { - const tree = new Map(); + type TreeEntry = {id: string; slot?: string}; + + // child node list of a given parent + const tree = new Map(); + + function getSiblings(parentId: string) { + const siblings = tree.get(parentId); + + if (siblings === undefined) { + const newSiblings: TreeEntry[] = []; + tree.set(parentId, newSiblings); + return newSiblings; + } + + return siblings; + } function mutate(records: RemoteMutationRecord[]) { for (const record of records) { @@ -78,14 +93,23 @@ export function adaptToLegacyRemoteChannel( switch (mutationType) { case MUTATION_TYPE_INSERT_CHILD: { - const node = record[2]; - const index = record[3]; + const [, parentId, node, nextSiblingId] = record; + const siblings = getSiblings(parentId); + + if (siblings.some((existing) => existing.id === node.id)) { + return; + } + + const index = + nextSiblingId === undefined + ? siblings.length + : siblings.findIndex((child) => child.id === nextSiblingId); persistNode(parentId, node, index); break; } case MUTATION_TYPE_REMOVE_CHILD: { - const index = record[2]; - removeNode(parentId, index); + const id = record[2]; + removeNode(parentId, id); break; } } @@ -99,11 +123,8 @@ export function adaptToLegacyRemoteChannel( node: RemoteNodeSerialization, index: number, ) { - if (!tree.has(parentId)) { - tree.set(parentId, []); - } - const parentNode = tree.get(parentId)!; - parentNode.splice(index, 0, { + const siblings = getSiblings(parentId); + siblings.splice(index, 0, { id: node.id, slot: 'attributes' in node ? node.attributes?.slot : undefined, }); @@ -115,12 +136,14 @@ export function adaptToLegacyRemoteChannel( } } - function removeNode(parentId: string, index: number) { - const parentNode = tree.get(parentId); - if (!parentNode?.[index]) return; + function removeNode(parentId: string, id: string) { + const siblings = getSiblings(parentId); + const index = siblings.findIndex((child) => child.id === id); + if (index === -1) { + return; + } - const id = parentNode[index].id; - parentNode.splice(index, 1); + siblings.splice(index, 1); cleanupNode(id); } @@ -146,12 +169,12 @@ export function adaptToLegacyRemoteChannel( payload as LegacyActionArgumentMap[typeof LEGACY_ACTION_MOUNT]; const records = nodes.map( - (node, index) => + (node) => [ MUTATION_TYPE_INSERT_CHILD, ROOT_ID, adaptLegacyNodeSerialization(node, options), - index, + undefined, ] satisfies RemoteMutationRecord, ); @@ -166,27 +189,33 @@ export function adaptToLegacyRemoteChannel( const records = []; - const parentNode = tree.get(parentId); + const siblings = getSiblings(parentId); - if (parentNode) { - const existingChildIndex = parentNode.findIndex( - ({id}) => id === child.id, - ); + const existingChildIndex = siblings.findIndex( + ({id}) => id === child.id, + ); - if (existingChildIndex >= 0) { - records.push([ - MUTATION_TYPE_REMOVE_CHILD, - parentId, - existingChildIndex, - ] satisfies RemoteMutationRecord); + let nextSiblingIndex = index; + + if (existingChildIndex >= 0) { + records.push([ + MUTATION_TYPE_REMOVE_CHILD, + parentId, + child.id, + ] satisfies RemoteMutationRecord); + + if (existingChildIndex <= index) { + nextSiblingIndex++; } } + const nextSibling = siblings[nextSiblingIndex]; + records.push([ MUTATION_TYPE_INSERT_CHILD, parentId, adaptLegacyNodeSerialization(child, options), - index, + nextSibling?.id, ] satisfies RemoteMutationRecord); mutate(records); @@ -195,12 +224,13 @@ export function adaptToLegacyRemoteChannel( } case LEGACY_ACTION_REMOVE_CHILD: { - const [parentID, removeIndex] = + const [parentId = ROOT_ID, index] = payload as LegacyActionArgumentMap[typeof LEGACY_ACTION_REMOVE_CHILD]; - - mutate([ - [MUTATION_TYPE_REMOVE_CHILD, parentID ?? ROOT_ID, removeIndex], - ]); + const id = tree.get(parentId)?.[index]?.id; + if (id === undefined) { + return; + } + mutate([[MUTATION_TYPE_REMOVE_CHILD, parentId, id]]); break; } @@ -215,41 +245,41 @@ export function adaptToLegacyRemoteChannel( } case LEGACY_ACTION_UPDATE_PROPS: { - const [id, props] = + const [parentId, props] = payload as LegacyActionArgumentMap[typeof LEGACY_ACTION_UPDATE_PROPS]; - const parentNode = tree.get(id); + const siblings = getSiblings(parentId); const records = []; for (const [key, value] of Object.entries(props)) { - const index = parentNode?.findIndex(({slot}) => slot === key) ?? -1; + const slotNodeId = siblings.find(({slot}) => slot === key)?.id; if (isFragment(value)) { - if (index >= 0) { + if (slotNodeId !== undefined) { records.push([ MUTATION_TYPE_REMOVE_CHILD, - id, - index, + parentId, + slotNodeId, ] satisfies RemoteMutationRecord); } records.push([ MUTATION_TYPE_INSERT_CHILD, - id, + parentId, adaptLegacyPropFragmentSerialization(key, value, options), - tree.get(id)?.length ?? 0, + undefined, ] satisfies RemoteMutationRecord); } else { - if (index >= 0) { + if (slotNodeId !== undefined) { records.push([ MUTATION_TYPE_REMOVE_CHILD, - id, - index, + parentId, + slotNodeId, ] satisfies RemoteMutationRecord); } else { records.push([ MUTATION_TYPE_UPDATE_PROPERTY, - id, + parentId, key, value, ] satisfies RemoteMutationRecord); diff --git a/packages/compat/source/tests/adapter.test.ts b/packages/compat/source/tests/adapter.test.ts index f1e86f3d..ab0e3945 100644 --- a/packages/compat/source/tests/adapter.test.ts +++ b/packages/compat/source/tests/adapter.test.ts @@ -52,7 +52,7 @@ describe('adaptToLegacyRemoteChannel()', () => { type: NODE_TYPE_TEXT, data: 'I am a text', }, - 0, + undefined, ], ]); @@ -98,7 +98,7 @@ describe('adaptToLegacyRemoteChannel()', () => { }, ], }, - 0, + undefined, ], ]); @@ -151,7 +151,7 @@ describe('adaptToLegacyRemoteChannel()', () => { type: NODE_TYPE_COMMENT, data: 'added by remote-ui legacy adaptor to replace a fragment rendered as a child', }, - 0, + undefined, ], ]); @@ -226,7 +226,7 @@ describe('adaptToLegacyRemoteChannel()', () => { }, ], }, - 0, + undefined, ], ]); @@ -314,7 +314,7 @@ describe('adaptToLegacyRemoteChannel()', () => { }, ], }, - 0, + undefined, ], ]); @@ -449,7 +449,7 @@ describe('adaptToLegacyRemoteChannel()', () => { ], properties: {}, }, - 0, + undefined, ], ]); @@ -559,7 +559,7 @@ describe('adaptToLegacyRemoteChannel()', () => { properties: {}, children: [{id: '2', type: NODE_TYPE_TEXT, data: 'I am a button'}], }, - 1, + undefined, ], ]); @@ -639,7 +639,7 @@ describe('adaptToLegacyRemoteChannel()', () => { ); expect(receiver.connection.mutate).toHaveBeenCalledWith([ - [MUTATION_TYPE_REMOVE_CHILD, '1', 0], + [MUTATION_TYPE_REMOVE_CHILD, '1', '2'], [ MUTATION_TYPE_INSERT_CHILD, '1', @@ -650,7 +650,7 @@ describe('adaptToLegacyRemoteChannel()', () => { properties: {}, children: [], }, - 1, + undefined, ], ]); @@ -755,7 +755,7 @@ describe('adaptToLegacyRemoteChannel()', () => { ], properties: {}, }, - 1, + undefined, ], ]); @@ -834,7 +834,7 @@ describe('adaptToLegacyRemoteChannel()', () => { channel(ACTION_REMOVE_CHILD, '2', 0); expect(receiver.connection.mutate).toHaveBeenCalledWith([ - [MUTATION_TYPE_REMOVE_CHILD, '2', 0], + [MUTATION_TYPE_REMOVE_CHILD, '2', '1'], ]); expect(receiver.root.children).toStrictEqual([ @@ -881,7 +881,7 @@ describe('adaptToLegacyRemoteChannel()', () => { channel(ACTION_REMOVE_CHILD, '3', 1); expect(receiver.connection.mutate).toHaveBeenCalledWith([ - [MUTATION_TYPE_REMOVE_CHILD, '3', 1], + [MUTATION_TYPE_REMOVE_CHILD, '3', '2'], ]); expect(receiver.root.children).toStrictEqual([ @@ -1034,7 +1034,7 @@ describe('adaptToLegacyRemoteChannel()', () => { }, ], }, - 0, + undefined, ], ]); @@ -1102,7 +1102,7 @@ describe('adaptToLegacyRemoteChannel()', () => { properties: {}, children: [], }, - 0, + undefined, ], ]); }); @@ -1167,7 +1167,7 @@ describe('adaptToLegacyRemoteChannel()', () => { }, ], }, - 0, + undefined, ], ]); @@ -1250,7 +1250,7 @@ describe('adaptToLegacyRemoteChannel()', () => { channel(ACTION_REMOVE_CHILD, '2', 0); expect(receiver.connection.mutate).toHaveBeenCalledWith([ - [MUTATION_TYPE_REMOVE_CHILD, '2', 0], + [MUTATION_TYPE_REMOVE_CHILD, '2', '1'], ]); expect(receiver.root.children).toStrictEqual([ @@ -1277,7 +1277,7 @@ describe('adaptToLegacyRemoteChannel()', () => { channel(ACTION_REMOVE_CHILD, '2', 0); expect(receiver.connection.mutate).toHaveBeenCalledWith([ - [MUTATION_TYPE_REMOVE_CHILD, '2', 0], + [MUTATION_TYPE_REMOVE_CHILD, '2', '0'], ]); expect(receiver.root.children).toStrictEqual([ @@ -1342,7 +1342,7 @@ describe('adaptToLegacyRemoteChannel()', () => { type: NODE_TYPE_TEXT, data: 'I am the third child', }, - 1, + '0', ], ]); @@ -1382,7 +1382,7 @@ describe('adaptToLegacyRemoteChannel()', () => { channel(ACTION_REMOVE_CHILD, '2', 0); expect(receiver.connection.mutate).toHaveBeenCalledWith([ - [MUTATION_TYPE_REMOVE_CHILD, '2', 0], + [MUTATION_TYPE_REMOVE_CHILD, '2', '1'], ]); expect(receiver.root.children).toStrictEqual([ @@ -1415,7 +1415,7 @@ describe('adaptToLegacyRemoteChannel()', () => { channel(ACTION_REMOVE_CHILD, '2', 0); expect(receiver.connection.mutate).toHaveBeenCalledWith([ - [MUTATION_TYPE_REMOVE_CHILD, '2', 0], + [MUTATION_TYPE_REMOVE_CHILD, '2', '3'], ]); expect(receiver.root.children).toStrictEqual([ @@ -1467,7 +1467,7 @@ describe('adaptToLegacyRemoteChannel()', () => { channel(ACTION_REMOVE_CHILD, ROOT_ID, 0); expect(receiver.connection.mutate).toHaveBeenCalledWith([ - [MUTATION_TYPE_REMOVE_CHILD, ROOT_ID, 0], + [MUTATION_TYPE_REMOVE_CHILD, ROOT_ID, '1'], ]); expect(receiver.root.children).toStrictEqual([]); diff --git a/packages/core/source/elements/RemoteMutationObserver.ts b/packages/core/source/elements/RemoteMutationObserver.ts index 518d949f..adeaed4f 100644 --- a/packages/core/source/elements/RemoteMutationObserver.ts +++ b/packages/core/source/elements/RemoteMutationObserver.ts @@ -34,49 +34,40 @@ import type {RemoteConnection, RemoteMutationRecord} from '../types.ts'; export class RemoteMutationObserver extends MutationObserver { constructor(private readonly connection: RemoteConnection) { super((records) => { - const addedNodes: Node[] = []; const remoteRecords: RemoteMutationRecord[] = []; for (const record of records) { const targetId = remoteId(record.target); if (record.type === 'childList') { - const position = record.previousSibling - ? indexOf(record.previousSibling, record.target.childNodes) + 1 - : 0; - record.removedNodes.forEach((node) => { + if (!REMOTE_IDS.has(node)) { + /** + * This happens if the node was not recognized during the + * `serializeRemoteNode` of a (probably direct and extensive) + * previous mutation-record, when it was no longer in the DOM + * at that time of processing. + */ + return; + } + disconnectRemoteNode(node); remoteRecords.push([ MUTATION_TYPE_REMOVE_CHILD, targetId, - position, + remoteId(node), ]); }); - // A mutation observer will queue some changes, so we might get one record - // for attaching a parent element, and additional records for attaching descendants. - // We serialize the entire tree when a new node was added, so we don’t want to - // send additional “insert child” records when we see those descendants — they - // will already be included the insertion of the parent. - record.addedNodes.forEach((node, index) => { - if ( - addedNodes.some((addedNode) => { - return addedNode === node || addedNode.contains(node); - }) - ) { - return; - } - - addedNodes.push(node); + record.addedNodes.forEach((node) => { connectRemoteNode(node, connection); remoteRecords.push([ MUTATION_TYPE_INSERT_CHILD, targetId, serializeRemoteNode(node), - position + index, + record.nextSibling ? remoteId(record.nextSibling) : undefined, ]); }); } else if (record.type === 'characterData') { @@ -134,7 +125,7 @@ export class RemoteMutationObserver extends MutationObserver { MUTATION_TYPE_INSERT_CHILD, ROOT_ID, serializeRemoteNode(node), - i, + undefined, ]); } @@ -150,11 +141,3 @@ export class RemoteMutationObserver extends MutationObserver { }); } } - -function indexOf(node: Node, list: NodeList) { - for (let i = 0; i < list.length; i++) { - if (list[i] === node) return i; - } - - return -1; -} diff --git a/packages/core/source/elements/RemoteRootElement.ts b/packages/core/source/elements/RemoteRootElement.ts index 393f83ca..10a1bcaa 100644 --- a/packages/core/source/elements/RemoteRootElement.ts +++ b/packages/core/source/elements/RemoteRootElement.ts @@ -54,7 +54,7 @@ export class RemoteRootElement extends HTMLElement { MUTATION_TYPE_INSERT_CHILD, ROOT_ID, serializeRemoteNode(node), - i, + undefined, ]); } diff --git a/packages/core/source/polyfill.ts b/packages/core/source/polyfill.ts index 93b3bfc0..0f37c50d 100644 --- a/packages/core/source/polyfill.ts +++ b/packages/core/source/polyfill.ts @@ -19,7 +19,7 @@ const hooks = window[HOOKS]; Window.setGlobal(window); -hooks.insertChild = (parent, node, index) => { +hooks.insertChild = (parent, node) => { const connection = remoteConnection(parent); if (connection == null) return; @@ -30,18 +30,20 @@ hooks.insertChild = (parent, node, index) => { MUTATION_TYPE_INSERT_CHILD, remoteId(parent), serializeRemoteNode(node), - index, + node.nextSibling ? remoteId(node.nextSibling) : undefined, ], ]); }; -hooks.removeChild = (parent, node, index) => { +hooks.removeChild = (parent, node) => { const connection = remoteConnection(parent); if (connection == null) return; disconnectRemoteNode(node); - connection.mutate([[MUTATION_TYPE_REMOVE_CHILD, remoteId(parent), index]]); + connection.mutate([ + [MUTATION_TYPE_REMOVE_CHILD, remoteId(parent), remoteId(node)], + ]); }; hooks.setText = (text, data) => { diff --git a/packages/core/source/receivers/DOMRemoteReceiver.ts b/packages/core/source/receivers/DOMRemoteReceiver.ts index 0962b9c6..08f29f66 100644 --- a/packages/core/source/receivers/DOMRemoteReceiver.ts +++ b/packages/core/source/receivers/DOMRemoteReceiver.ts @@ -105,27 +105,36 @@ export class DOMRemoteReceiver { ? call(element as any, method, ...args) : (element as any)[method](...args); }, - insertChild: (id, child, index) => { - const parent = id === ROOT_ID ? this.root : attached.get(id)!; + insertChild: (parentId, child, nextSiblingId) => { + const parent = + parentId === ROOT_ID ? this.root : attached.get(parentId)!; + const normalizedChild = attach(child); - const existingTimeout = destroyTimeouts.get(id); + if (parent.contains(normalizedChild)) { + return; + } + + const existingTimeout = destroyTimeouts.get(parentId); if (existingTimeout) clearTimeout(existingTimeout); - parent.insertBefore(attach(child), parent.childNodes[index] || null); + if (nextSiblingId === undefined) { + parent.appendChild(normalizedChild); + } else { + parent.insertBefore(normalizedChild, attached.get(nextSiblingId)!); + } }, - removeChild: (id, index) => { - const parent = id === ROOT_ID ? this.root : attached.get(id)!; - const child = parent.childNodes[index]!; + removeChild: (parentId, id) => { + const child = attached.get(id) as ChildNode; child.remove(); if (cache?.maxAge) { - const existingTimeout = destroyTimeouts.get(id); + const existingTimeout = destroyTimeouts.get(parentId); if (existingTimeout) clearTimeout(existingTimeout); const timeout = setTimeout(() => { detach(child); }, cache.maxAge); - destroyTimeouts.set(id, timeout as any); + destroyTimeouts.set(parentId, timeout as any); } else { detach(child); } diff --git a/packages/core/source/receivers/RemoteReceiver.ts b/packages/core/source/receivers/RemoteReceiver.ts index 5d8ad358..48e7627a 100644 --- a/packages/core/source/receivers/RemoteReceiver.ts +++ b/packages/core/source/receivers/RemoteReceiver.ts @@ -162,21 +162,21 @@ export class RemoteReceiver { return implementationMethod(...args); }, - insertChild: (id, child, index) => { - const parent = attached.get(id) as Writable; + insertChild: (parentId, child, nextSiblingId) => { + const parent = attached.get(parentId) as Writable; + const children = parent.children as Writable; - const {children} = parent; + if (children.some((existing) => existing.id === child.id)) { + return; + } const normalizedChild = attach(child, parent); - if (index === children.length) { - (children as Writable).push(normalizedChild); + if (nextSiblingId === undefined) { + children.push(normalizedChild); } else { - (children as Writable).splice( - index, - 0, - normalizedChild, - ); + const sibling = attached.get(nextSiblingId) as RemoteReceiverNode; + children.splice(children.indexOf(sibling), 0, normalizedChild); } parent.version += 1; @@ -184,15 +184,14 @@ export class RemoteReceiver { runSubscribers(parent); }, - removeChild: (id, index) => { - const parent = attached.get(id) as Writable; + removeChild: (parentId, id) => { + const parent = attached.get(parentId) as Writable; + const children = parent.children as Writable; - const {children} = parent; + const node = attached.get(id) as Writable; + const index = parent.children.indexOf(node); - const [removed] = (children as Writable).splice( - index, - 1, - ); + const [removed] = children.splice(index, 1); if (!removed) { return; diff --git a/packages/core/source/tests/elements.test.ts b/packages/core/source/tests/elements.test.ts index 1c85e3d7..2dbd6f87 100644 --- a/packages/core/source/tests/elements.test.ts +++ b/packages/core/source/tests/elements.test.ts @@ -783,7 +783,7 @@ describe('RemoteElement', () => { expect.objectContaining({ attributes: {name}, }), - 0, + undefined, ], ]); }); @@ -809,7 +809,7 @@ describe('RemoteElement', () => { expect.objectContaining({ attributes: {slot}, }), - 0, + undefined, ], ]); }); diff --git a/packages/core/source/types.ts b/packages/core/source/types.ts index ab690e4c..d311635e 100644 --- a/packages/core/source/types.ts +++ b/packages/core/source/types.ts @@ -20,7 +20,7 @@ export type RemoteMutationRecordInsertChild = [ /** * The ID of the parent node. */ - id: string, + parentId: string, /** * A description of the child node being inserted. @@ -33,7 +33,7 @@ export type RemoteMutationRecordInsertChild = [ /** * The index in the parents’ children to insert the new child. */ - index: number, + nextSiblingId: string | undefined, ]; /** @@ -45,12 +45,12 @@ export type RemoteMutationRecordRemoveChild = [ /** * The ID of the parent node. */ - id: string, + parentId: string, /** - * The index of the child to remove. + * The ID of the child to remove. */ - index: number, + id: string, ]; /** diff --git a/packages/polyfill/source/ParentNode.ts b/packages/polyfill/source/ParentNode.ts index b40d0c34..9ab94045 100644 --- a/packages/polyfill/source/ParentNode.ts +++ b/packages/polyfill/source/ParentNode.ts @@ -80,7 +80,7 @@ export class ParentNode extends ChildNode { } if (this.nodeType === NodeType.ELEMENT_NODE) { - this[HOOKS].removeChild?.(this as any, child as any, childNodesIndex); + this[HOOKS].removeChild?.(this as any, child as any); } } @@ -163,7 +163,6 @@ export class ParentNode extends ChildNode { } } } else { - insertIndex = childNodes.length; childNodes.push(child); if (isElement) this.children.push(child); } @@ -176,7 +175,7 @@ export class ParentNode extends ChildNode { } if (this.nodeType === NodeType.ELEMENT_NODE) { - this[HOOKS].insertChild?.(this as any, child as any, insertIndex); + this[HOOKS].insertChild?.(this as any, child as any); } } } diff --git a/packages/polyfill/source/hooks.ts b/packages/polyfill/source/hooks.ts index 3f3ad888..4f74d655 100644 --- a/packages/polyfill/source/hooks.ts +++ b/packages/polyfill/source/hooks.ts @@ -9,8 +9,8 @@ export interface Hooks { removeAttribute(element: Element, name: string, ns?: string | null): void; createText(text: Text, data: string): void; setText(text: Text, data: string): void; - insertChild(parent: Element, node: Element | Text, index: number): void; - removeChild(parent: Element, node: Element | Text, index: number): void; + insertChild(parent: Element, node: Element | Text): void; + removeChild(parent: Element, node: Element | Text): void; addEventListener( element: EventTarget, type: string, diff --git a/packages/signals/source/SignalRemoteReceiver.ts b/packages/signals/source/SignalRemoteReceiver.ts index 6af8aeaf..f7f17015 100644 --- a/packages/signals/source/SignalRemoteReceiver.ts +++ b/packages/signals/source/SignalRemoteReceiver.ts @@ -158,25 +158,34 @@ export class SignalRemoteReceiver { return implementationMethod(...args); }, - insertChild: (id, child, index) => { - const parent = attached.get(id) as SignalRemoteReceiverParent; - const newChildren = [...parent.children.peek()]; + insertChild: (parentId, child, nextSiblingId) => { + const parent = attached.get(parentId) as SignalRemoteReceiverParent; + const children = [...parent.children.peek()]; + + if (children.some((existing) => existing.id === child.id)) { + return; + } const normalizedChild = attach(child, parent); - if (index === newChildren.length) { - newChildren.push(normalizedChild); + if (nextSiblingId === undefined) { + children.push(normalizedChild); } else { - newChildren.splice(index, 0, normalizedChild); + const nextSibling = attached.get( + nextSiblingId, + ) as SignalRemoteReceiverNode; + children.splice(children.indexOf(nextSibling), 0, normalizedChild); } - (parent.children as any).value = newChildren; + (parent.children as any).value = children; }, - removeChild: (id, index) => { - const parent = attached.get(id) as SignalRemoteReceiverParent; - + removeChild: (parentId, id) => { + const parent = attached.get(parentId) as SignalRemoteReceiverParent; const newChildren = [...parent.children.peek()]; + const node = attached.get(id) as SignalRemoteReceiverNode; + const index = newChildren.indexOf(node); + const [removed] = newChildren.splice(index, 1); if (!removed) {