Skip to content

fix: improve indexed each array reconcilation #10422

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.

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

Merged
merged 2 commits into from
Feb 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 .changeset/silver-points-approve.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"svelte": patch
---

fix: improve indexed each array reconcilation
33 changes: 6 additions & 27 deletions packages/svelte/src/internal/client/each.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,11 @@ import {
set_current_hydration_fragment
} from './hydration.js';
import { clear_text_content, map_get, map_set } from './operations.js';
import { STATE_SYMBOL } from './proxy.js';
import { insert, remove } from './reconciler.js';
import { empty } from './render.js';
import {
destroy_signal,
execute_effect,
is_lazy_property,
lazy_property,
mutable_source,
push_destroy_fn,
render_effect,
Expand Down Expand Up @@ -132,7 +129,7 @@ function each(anchor_node, collection, flags, key_fn, render_fn, fallback_fn, re
};

/** @param {import('./types.js').EachBlock} block */
const clear_each = (block) => {
const render_each = (block) => {
const flags = block.f;
const is_controlled = (flags & EACH_IS_CONTROLLED) !== 0;
const anchor_node = block.a;
Expand Down Expand Up @@ -175,7 +172,7 @@ function each(anchor_node, collection, flags, key_fn, render_fn, fallback_fn, re
if (fallback_fn !== null) {
if (length === 0) {
if (block.v.length !== 0 || render === null) {
clear_each(block);
render_each(block);
create_fallback_effect();
return;
}
Expand All @@ -201,7 +198,7 @@ function each(anchor_node, collection, flags, key_fn, render_fn, fallback_fn, re
false
);

render = render_effect(clear_each, block, true);
render = render_effect(render_each, block, true);

if (mismatch) {
// Set a fragment so that Svelte continues to operate in hydration mode
Expand Down Expand Up @@ -279,14 +276,9 @@ function reconcile_indexed_array(
flags,
apply_transitions
) {
var is_proxied_array = STATE_SYMBOL in array && /** @type {any} */ (array[STATE_SYMBOL]).i;
var a_blocks = each_block.v;
var active_transitions = each_block.s;

if (is_proxied_array) {
flags &= ~EACH_ITEM_REACTIVE;
}

/** @type {number | void} */
var a = a_blocks.length;

Expand Down Expand Up @@ -334,7 +326,7 @@ function reconcile_indexed_array(
break;
}

item = is_proxied_array ? lazy_property(array, index) : array[index];
item = array[index];
block = each_item_block(item, null, index, render_fn, flags);
b_blocks[index] = block;

Expand All @@ -349,7 +341,7 @@ function reconcile_indexed_array(
for (; index < length; index++) {
if (index >= a) {
// Add block
item = is_proxied_array ? lazy_property(array, index) : array[index];
item = array[index];
block = each_item_block(item, null, index, render_fn, flags);
b_blocks[index] = block;
insert_each_item_block(block, dom, is_controlled, null);
Expand Down Expand Up @@ -789,17 +781,6 @@ function update_each_item_block(block, item, index, type) {
const block_v = block.v;
if ((type & EACH_ITEM_REACTIVE) !== 0) {
set_signal_value(block_v, item);
} else if (is_lazy_property(block_v)) {
// If we have lazy properties, it means that an array was used that has been
// proxied. Given this, we need to re-sync the old array by mutating the backing
// value to be the latest value to ensure the UI updates correctly. TODO: maybe
// we should bypass any internal mutation checks for this?
const o = block_v.o;
const p = block_v.p;
const prev = o[p];
if (prev !== item) {
o[p] = item;
}
}
const transitions = block.s;
const index_is_reactive = (type & EACH_INDEX_REACTIVE) !== 0;
Expand Down Expand Up @@ -856,9 +837,7 @@ export function destroy_each_item_block(

/**
* @template V
* @template O
* @template P
* @param {V | import('./types.js').LazyProperty<O, P>} item
* @param {V} item
* @param {unknown} key
* @param {number} index
* @param {(anchor: null, item: V, index: number | import('./types.js').Signal<number>) => void} render_fn
Expand Down
33 changes: 0 additions & 33 deletions packages/svelte/src/internal/client/runtime.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ const FLUSH_MICROTASK = 0;
const FLUSH_SYNC = 1;

export const UNINITIALIZED = Symbol();
export const LAZY_PROPERTY = Symbol();

// Used for controlling the flush of effects.
let current_scheduler_mode = FLUSH_MICROTASK;
Expand Down Expand Up @@ -1566,20 +1565,6 @@ export function is_signal(val) {
);
}

/**
* @template O
* @template P
* @param {any} val
* @returns {val is import('./types.js').LazyProperty<O, P>}
*/
export function is_lazy_property(val) {
return (
typeof val === 'object' &&
val !== null &&
/** @type {import('./types.js').LazyProperty<O, P>} */ (val).t === LAZY_PROPERTY
);
}

/**
* @template V
* @param {unknown} val
Expand Down Expand Up @@ -2063,21 +2048,6 @@ export function inspect(get_value, inspect = console.log) {
});
}

/**
* @template O
* @template P
* @param {O} o
* @param {P} p
* @returns {import('./types.js').LazyProperty<O, P>}
*/
export function lazy_property(o, p) {
return {
o,
p,
t: LAZY_PROPERTY
};
}

/**
* @template V
* @param {V} value
Expand All @@ -2088,9 +2058,6 @@ export function unwrap(value) {
// @ts-ignore
return get(value);
}
if (is_lazy_property(value)) {
return value.o[value.p];
}
// @ts-ignore
return value;
}
Expand Down
8 changes: 1 addition & 7 deletions packages/svelte/src/internal/client/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
SNIPPET_BLOCK
} from './block.js';
import type { READONLY_SYMBOL, STATE_SYMBOL } from './proxy.js';
import { DERIVED, EFFECT, RENDER_EFFECT, SOURCE, PRE_EFFECT, LAZY_PROPERTY } from './runtime.js';
import { DERIVED, EFFECT, RENDER_EFFECT, SOURCE, PRE_EFFECT } from './runtime.js';

// Put all internal types in this file. Once we convert to JSDoc, we can make this a d.ts file

Expand Down Expand Up @@ -123,12 +123,6 @@ export type MaybeSignal<T = unknown> = T | Signal<T>;

export type UnwrappedSignal<T> = T extends Signal<infer U> ? U : T;

export type LazyProperty<O, P> = {
o: O;
p: P;
t: typeof LAZY_PROPERTY;
};

export type EqualsFunctions<T = any> = (a: T, v: T) => boolean;

export type BlockType =
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import { flushSync } from 'svelte';
import { test } from '../../test';

export default test({
html: `<ul><li><button>Delete</button>\na\na</li><li><button>Delete</button>\nb\nb</li><li><button>Delete</button>\nc\nc</li><li><button>Delete</button>\nd\nd</li></ul>`,

async test({ assert, target }) {
/**
* @type {{ click: () => void; }}
*/
let btn1;

[btn1] = target.querySelectorAll('button');

flushSync(() => {
btn1.click();
});

assert.htmlEqual(
target.innerHTML,
`<ul><li><button>Delete</button>\nb\nb</li><li><button>Delete</button>\nc\nc</li><li><button>Delete</button>\nd\nd</li></ul>`
);

[btn1] = target.querySelectorAll('button');

flushSync(() => {
btn1.click();
});

assert.htmlEqual(
target.innerHTML,
`<ul><li><button>Delete</button>\nc\nc</li><li><button>Delete</button>\nd\nd</li></ul>`
);

[btn1] = target.querySelectorAll('button');

flushSync(() => {
btn1.click();
});

assert.htmlEqual(target.innerHTML, `<ul><li><button>Delete</button>\nd\nd</li></ul>`);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<script>
let entries = $state([
{
id: 'a',
subitems: ['a'],
},
{
id: 'b',
subitems: ['b'],
},
{
id: 'c',
subitems: ['c'],
},
{
id: 'd',
subitems: ['d'],
},
]);

function onDeleteEntry(entry) {
entries = entries.filter((innerEntry) => innerEntry.id !== entry.id);
}
</script>

<ul>
{#each entries as entry}
<li>
<button on:click={() => onDeleteEntry(entry)}>Delete</button>
{entry.id}
{#each entry.subitems as subitem}
{subitem}
{/each}
</li>
{/each}
</ul>