Skip to content

Use “moveBefore” for map shuffling. #343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
theengineear wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Use “moveBefore” for map shuffling. #343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- When re-rendering arrays of changing length, where the length is getting
smaller or is zero — the internal bookkeeping was getting messed up. This
caused runtime errors in these cases (#303).
- When re-rendering a map array with reordered elements, `connectedCallback`
and `disconnectedCallback` are no longer called because elements are only
moved in the DOM rather than being disconnected and reconnected. Added test
coverage for `connectedMoveCallback` and reference tracking during element
reordering (#254).
Copy link
Collaborator Author

@theengineear theengineear Dec 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case this issue doesn’t ring a bell — we specifically have an outstanding bug related to shuffling tabs for an editor which can currently cause monaco to be disposed and then recreated if moving tabs around!


## [1.1.2] - 2024-12-16

Expand Down
18 changes: 18 additions & 0 deletions test/test-scratch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ class TestElement extends XElement {
super.adoptedCallback();
this.adopted = true;
}

connectedMoveCallback() {
super.connectedMoveCallback();
Copy link
Collaborator Author

@theengineear theengineear Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only line in our test suite that exercises this callback — just for coverage.

this.moved = true;
}
}

customElements.define('test-element', TestElement);
Expand Down Expand Up @@ -241,3 +246,16 @@ it('authors can extend observed attributes', () => {
el.setAttribute('custom-observed-attribute', '');
assert(el.customObservedAttributeChange);
});

it('test connectedMoveCallback', () => {
const el = document.createElement('test-element');
const container = document.createElement('div');
const sibling = document.createElement('div');
document.body.append(container);
container.append(sibling);
container.append(el);
assert(!el.moved);
container.moveBefore(el, sibling);
assert(el.moved);
container.remove();
});
89 changes: 73 additions & 16 deletions test/test-template-engine.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,66 @@ describe('html rendering', () => {
assert(container.querySelector('#target').children[2].classList.contains('true'));
});

// This specifically tests out cases where we _cannot_ reuse DOM on shuffle.
Copy link
Collaborator Author

@theengineear theengineear Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was tricky! A bit harder to hit this case now that we do more DOM re-use here. Again, just making sure we maintain 100% coverage.

it('renders map arrays with changing templates', () => {
const getTemplate = ({ items }) => {
return html`
<div id="target">
${items.map(item => {
if (item.type === 'div') {
return [item.id, html`<div>${item.id}</div>`];
} else if (item.type === 'span') {
return [item.id, html`<span>${item.id}</span>`];
} else {
return [item.id, html`<p>${item.id}</p>`];
}
})}
</div>
`;
};
const container = document.createElement('div');

// Render with all “div” elements.
render(container, getTemplate({ items: [
{ id: 'a', type: 'div' },
{ id: 'b', type: 'div' },
{ id: 'c', type: 'div' },
] }));
assert(container.querySelector('#target').childElementCount === 3);
assert(container.querySelector('#target').children[0].localName === 'div');
assert(container.querySelector('#target').children[0].textContent === 'a');
assert(container.querySelector('#target').children[1].localName === 'div');
assert(container.querySelector('#target').children[1].textContent === 'b');
assert(container.querySelector('#target').children[2].localName === 'div');
assert(container.querySelector('#target').children[2].textContent === 'c');

// Change first and last to “span” elements. Different template, same order.
render(container, getTemplate({ items: [
{ id: 'a', type: 'span' },
{ id: 'b', type: 'div' },
{ id: 'c', type: 'span' },
] }));
assert(container.querySelector('#target').children[0].localName === 'span');
assert(container.querySelector('#target').children[0].textContent === 'a');
assert(container.querySelector('#target').children[1].localName === 'div');
assert(container.querySelector('#target').children[1].textContent === 'b');
assert(container.querySelector('#target').children[2].localName === 'span');
assert(container.querySelector('#target').children[2].textContent === 'c');

// Reorder _and_ change templates.
render(container, getTemplate({ items: [
{ id: 'b', type: 'span' },
{ id: 'c', type: 'p' },
{ id: 'a', type: 'div' },
] }));
assert(container.querySelector('#target').children[0].localName === 'span');
assert(container.querySelector('#target').children[0].textContent === 'b');
assert(container.querySelector('#target').children[1].localName === 'p');
assert(container.querySelector('#target').children[1].textContent === 'c');
assert(container.querySelector('#target').children[2].localName === 'div');
assert(container.querySelector('#target').children[2].textContent === 'a');
});

it('renders lists with changing length', () => {
const getTemplate = ({ items }) => {
return html`
Expand Down Expand Up @@ -845,14 +905,13 @@ describe('html rendering', () => {
assert(container.querySelector('#target').childElementCount === 2);
});

// TODO: #254: Uncomment “moves” lines when we leverage “moveBefore”.
it('native map does not cause disconnectedCallback on prefix removal', () => {
let connects = 0;
// let moves = 0;
let moves = 0;
let disconnects = 0;
class TestPrefixRemoval extends HTMLElement {
connectedCallback() { connects++; }
// connectedMoveCallback() { moves++; }
connectedMoveCallback() { moves++; }
disconnectedCallback() { disconnects++; }
}
customElements.define('test-prefix-removal', TestPrefixRemoval);
Expand All @@ -871,35 +930,34 @@ describe('html rendering', () => {

document.body.append(container);
assert(connects === 0);
// assert(moves === 0);
assert(moves === 0);
assert(disconnects === 0);

render(container, getTemplate({ items: [{ id: 'foo' }, { id: 'bar' }] }));
assert(connects === 2);
// assert(moves === 0);
assert(moves === 0);
assert(disconnects === 0);

render(container, getTemplate({ items: [{ id: 'bar' }] }));
assert(connects === 2);
// assert(moves === 1);
assert(moves === 1);
assert(disconnects === 1);

render(container, getTemplate({ items: [] }));
assert(connects === 2);
// assert(moves === 1);
assert(moves === 1);
assert(disconnects === 2);

container.remove();
});

// TODO: #254: Uncomment “moves” lines when we leverage “moveBefore”.
it('native map does not cause disconnectedCallback on suffix removal', () => {
let connects = 0;
// let moves = 0;
let moves = 0;
let disconnects = 0;
class TestSuffixRemoval extends HTMLElement {
connectedCallback() { connects++; }
// connectedMoveCallback() { moves++; }
connectedMoveCallback() { moves++; }
disconnectedCallback() { disconnects++; }
}
customElements.define('test-suffix-removal', TestSuffixRemoval);
Expand All @@ -918,29 +976,28 @@ describe('html rendering', () => {

document.body.append(container);
assert(connects === 0);
// assert(moves === 0);
assert(moves === 0);
assert(disconnects === 0);

render(container, getTemplate({ items: [{ id: 'foo' }, { id: 'bar' }] }));
assert(connects === 2);
// assert(moves === 0);
assert(moves === 0);
assert(disconnects === 0);

render(container, getTemplate({ items: [{ id: 'foo' }] }));
assert(connects === 2);
// assert(moves === 0);
assert(moves === 0);
assert(disconnects === 1);

render(container, getTemplate({ items: [] }));
assert(connects === 2);
// assert(moves === 0);
assert(moves === 0);
assert(disconnects === 2);

container.remove();
});

// TODO: #254: See https://chromestatus.com/feature/5135990159835136.
it.todo('native map does not cause disconnectedCallback on list shuffle', () => {
Copy link
Collaborator Author

@theengineear theengineear Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very satisfying to delete this “.todo” here and see it just work ❤️

it('native map does not cause disconnectedCallback on list shuffle', () => {
let connects = 0;
let moves = 0;
let disconnects = 0;
Expand Down
4 changes: 4 additions & 0 deletions types/x-element.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,10 @@ export default class XElement extends HTMLElement {
* Extends HTMLElement.prototype.connectedCallback.
*/
connectedCallback(): void;
/**
* Extends HTMLElement.prototype.connectedMoveCallback.
*/
connectedMoveCallback(): void;
/**
* Extends HTMLElement.prototype.attributeChangedCallback.
* @param {string} attribute
Expand Down
2 changes: 1 addition & 1 deletion types/x-element.d.ts.map
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion types/x-template.d.ts.map
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 4 additions & 5 deletions x-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,11 +153,10 @@ export default class XElement extends HTMLElement {
XElement.#connectHost(this);
}

// TODO: #254: Uncomment once we leverage “moveBefore”.
// /**
// * Extends HTMLElement.prototype.connectedMoveCallback.
// */
// connectedMoveCallback() {}
/**
* Extends HTMLElement.prototype.connectedMoveCallback.
*/
connectedMoveCallback() {}
Copy link
Collaborator Author

@theengineear theengineear Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor interface addition here for discoverability (matches our other patterns — i.e., you should be able to rely on super existing).


/**
* Extends HTMLElement.prototype.attributeChangedCallback.
Expand Down
113 changes: 30 additions & 83 deletions x-template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,98 +385,46 @@ class TemplateEngine {
arrayState.map = new Map();
}

// A mapping has already been created — we need to update the items.
const ids = new Set(); // Populated in “parseListValue”.
let index = 0;
for (const entry of entries) {
const idsToRemove = new Set(arrayState.map.keys());
const ids = new Set(); // Populated in “parseArrayEntry”.
let reference = startNode.nextSibling;
for (let index = 0; index < entries.length; index++) {
const entry = entries[index];
const [id, rawResult] = TemplateEngine.#parseArrayEntry(entry, index, ids);
let item = arrayState.map.get(id);
if (item) {
idsToRemove.delete(id);
if (!TemplateEngine.#canReuseDom(item.preparedResult, rawResult)) {
const referenceWasStartNode = reference === item.startNode;
TemplateEngine.#recreateArrayItem(item, rawResult);
reference = referenceWasStartNode ? item.startNode : reference;
} else {
TemplateEngine.#update(item.preparedResult, rawResult);
}
} else {
item = TemplateEngine.#createArrayItem(node, id, rawResult);
arrayState.map.set(id, item);
}
index++;
}
for (const [id, item] of arrayState.map.entries()) {
if (!ids.has(id)) {
TemplateEngine.#removeThrough(item.startNode, item.node);
arrayState.map.delete(id);
}
}
let lastItem;
for (const id of ids) {
const item = arrayState.map.get(id);
// TODO: We should be able to make the following code more performant.
const referenceNode = lastItem ? lastItem.node.nextSibling : startNode.nextSibling;
if (referenceNode !== item.startNode) {
if (item.startNode !== reference) {
// Move to the correct location
const nodesToMove = [item.startNode];
while (nodesToMove[nodesToMove.length - 1] !== item.node) {
nodesToMove.push(nodesToMove[nodesToMove.length - 1].nextSibling);
}
TemplateEngine.#insertAllBefore(referenceNode.parentNode, referenceNode, nodesToMove);
TemplateEngine.#moveAllBefore(reference.parentNode, reference, nodesToMove);
}
lastItem = item;

// Move our position forward.
reference = item.node.nextSibling;
}
}

// TODO: #254: Future state where the “moveBefore” API is better-supported.
// // Loops over given array of “entries” to manage an array of nodes.
// static #commitContentArrayEntries(node, startNode, entries) {
// const arrayState = TemplateEngine.#getState(startNode, TemplateEngine.#ARRAY_STATE);
// if (!arrayState.map) {
// // There is no mapping in our state — create an empty one as our base.
// TemplateEngine.#clearObject(arrayState);
// arrayState.map = new Map();
// }
//
// const idsToRemove = new Set(arrayState.map.keys());
// const ids = new Set(); // Populated in “parseArrayEntry”.
// let reference = startNode.nextSibling;
// for (let index = 0; index < entries.length; index++) {
// const entry = entries[index];
// const [id, rawResult] = TemplateEngine.#parseArrayEntry(entry, index, ids);
// let item = arrayState.map.get(id);
// if (item) {
// // Update existing item.
// idsToRemove.delete(id);
// if (!TemplateEngine.#canReuseDom(item.preparedResult, rawResult)) {
// const referenceWasStartNode = reference === item.startNode;
// TemplateEngine.#recreateArrayItem(item, rawResult);
// reference = referenceWasStartNode ? item.startNode : reference;
// } else {
// TemplateEngine.#update(item.preparedResult, rawResult);
// }
// } else {
// // Create new item.
// item = TemplateEngine.#createArrayItem(node, id, rawResult);
// arrayState.map.set(id, item);
// }
// // Move to the correct location
// if (item.startNode !== reference) {
// const nodesToMove = [item.startNode];
// while (nodesToMove[nodesToMove.length - 1] !== item.node) {
// nodesToMove.push(nodesToMove[nodesToMove.length - 1].nextSibling);
// }
// TemplateEngine.#moveAllBefore(reference.parentNode, reference, nodesToMove);
// }
//
// // Move our position forward.
// reference = item.node.nextSibling;
// }
//
// // Remove any ids which are not longer in the entries.
// for (const id of idsToRemove) {
// const item = arrayState.map.get(id);
// TemplateEngine.#removeThrough(item.startNode, item.node);
// arrayState.map.delete(id);
// }
// }
// Remove any ids which are not longer in the entries.
for (const id of idsToRemove) {
const item = arrayState.map.get(id);
TemplateEngine.#removeThrough(item.startNode, item.node);
arrayState.map.delete(id);
}
}

static #commitContentFragmentValue(node, startNode, value) {
const previousSibling = node.previousSibling;
Expand Down Expand Up @@ -669,16 +617,15 @@ class TemplateEngine {
return { startNode, node };
}

// TODO: #254: Future state when we leverage “moveBefore”.
// static #moveAllBefore(parentNode, referenceNode, nodes) {
// // Iterate backwards over the live node collection since we’re mutating it.
// // Note that passing “null” as the reference node moves nodes to the end.
// for (let iii = nodes.length - 1; iii >= 0; iii--) {
// const node = nodes[iii];
// parentNode.moveBefore(node, referenceNode);
// referenceNode = node;
// }
// }
static #moveAllBefore(parentNode, referenceNode, nodes) {
// Iterate backwards over the live node collection since we’re mutating it.
// Note that passing “null” as the reference node moves nodes to the end.
for (let iii = nodes.length - 1; iii >= 0; iii--) {
const node = nodes[iii];
parentNode.moveBefore(node, referenceNode);
Copy link
Collaborator Author

@theengineear theengineear Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really the key change. We leverage the new moveBefore API to move these items without disconnecting / reconnecting them!

referenceNode = node;
}
}

static #insertAllBefore(parentNode, referenceNode, nodes) {
// Iterate backwards over the live node collection since we’re mutating it.
Expand Down