msb-public
/
svelte
mirror of https://github.com/sveltejs/svelte
1
0
Code Issues Projects Releases Wiki Activity

fix: overhaul `each` blocks (#17150)

Browse Source 
* state.items -> state.onscreen

* offscreen_items -> state.offscreen

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* always push

* fix

* note to self

* tweak/fix

* tidy up

* simplify

* unused

* clarity

* note to self

* simplify

* one test to go

* holy shit i think it works

* unused

* reduce number of bitwise checks

* revert

* small tweak

* better comment

* update note to self

* fix: each block losing reactivity when items removed while promise pending (#17151)

Co-authored-by: dylan <dylanbradshaw107@hotmail.com>
Co-authored-by: Rich Harris <rich.harris@vercel.com>

* add commented-out test, todo investigate

---------

Co-authored-by: dylan1951 <58990501+dylan1951@users.noreply.github.com>
Co-authored-by: dylan <dylanbradshaw107@hotmail.com>
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com>
pull/17161/head
Rich Harris 3 days ago committed by GitHub
parent 1e0c8c544c
commit c02ad03bfd
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 380 additions and 278 deletions
Show Stats Download Patch File Download Diff File

5
.changeset/clean-toes-joke.md
Unescape View File

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: each block losing reactivity when items removed while promise pending

400
packages/svelte/src/internal/client/dom/blocks/each.js
Unescape View File

@ -38,7 +38,7 @@ import { source, mutable_source, internal_set } from '../../reactivity/sources.j
import { array_from, is_array } from '../../../shared/utils.js';
import { COMMENT_NODE, INERT } from '#client/constants';
import { queue_micro_task } from '../task.js';
import { active_effect, get } from '../../runtime.js';
import { get } from '../../runtime.js';
import { DEV } from 'esm-env';
import { derived_safe_equal } from '../../reactivity/deriveds.js';
import { current_batch } from '../../reactivity/batch.js';
@ -67,41 +67,51 @@ export function index(_, i) {
* Pause multiple effects simultaneously, and coordinate their
* subsequent destruction. Used in each blocks
* @param {EachState} state
* @param {EachItem[]} items
* @param {EachItem[]} to_destroy
* @param {null | Node} controlled_anchor
*/
function pause_effects(state, items, controlled_anchor) {
var items_map = state.items;
function pause_effects(state, to_destroy, controlled_anchor) {
/** @type {TransitionManager[]} */
var transitions = [];
var length = items.length;
var length = to_destroy.length;
for (var i = 0; i < length; i++) {
pause_children(items[i].e, transitions, true);
}
var is_controlled = length > 0 && transitions.length === 0 && controlled_anchor !== null;
// If we have a controlled anchor, it means that the each block is inside a single
// DOM element, so we can apply a fast-path for clearing the contents of the element.
if (is_controlled) {
var parent_node = /** @type {Element} */ (
/** @type {Element} */ (controlled_anchor).parentNode
);
clear_text_content(parent_node);
parent_node.append(/** @type {Element} */ (controlled_anchor));
items_map.clear();
link(state, items[0].prev, items[length - 1].next);
pause_children(to_destroy[i].e, transitions, true);
}
run_out_transitions(transitions, () => {
// If we're in a controlled each block (i.e. the block is the only child of an
// element), and we are removing all items, _and_ there are no out transitions,
// we can use the fast path — emptying the element and replacing the anchor
var fast_path = transitions.length === 0 && controlled_anchor !== null;
// TODO only destroy effects if no pending batch needs them. otherwise,
// just set `item.o` back to `false`
if (fast_path) {
var anchor = /** @type {Element} */ (controlled_anchor);
var parent_node = /** @type {Element} */ (anchor.parentNode);
clear_text_content(parent_node);
parent_node.append(anchor);
state.items.clear();
link(state, to_destroy[0].prev, to_destroy[length - 1].next);
}
for (var i = 0; i < length; i++) {
var item = items[i];
if (!is_controlled) {
items_map.delete(item.k);
var item = to_destroy[i];
if (!fast_path) {
state.items.delete(item.k);
link(state, item.prev, item.next);
}
destroy_effect(item.e, !is_controlled);
destroy_effect(item.e, !fast_path);
}
if (state.first === to_destroy[0]) {
state.first = to_destroy[0].prev;
}
});
}
@ -123,6 +133,8 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f
var state = { flags, items: new Map(), first: null };
var is_controlled = (flags & EACH_IS_CONTROLLED) !== 0;
var is_reactive_value = (flags & EACH_ITEM_REACTIVE) !== 0;
var is_reactive_index = (flags & EACH_INDEX_REACTIVE) !== 0;
if (is_controlled) {
var parent_node = /** @type {Element} */ (node);
@ -136,14 +148,9 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f
hydrate_next();
}
/** @type {Effect | null} */
/** @type {{ fragment: DocumentFragment | null, effect: Effect } | null} */
var fallback = null;
var was_empty = false;
/** @type {Map<any, EachItem>} */
var offscreen_items = new Map();
// TODO: ideally we could use derived for runes mode but because of the ability
// to use a store which can be mutated, we can't do that here as mutating a store
// will still result in the collection array being the same from the store
@ -156,51 +163,36 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f
/** @type {V[]} */
var array;
/** @type {Effect} */
var each_effect;
var first_run = true;
function commit() {
reconcile(
each_effect,
array,
state,
offscreen_items,
anchor,
render_fn,
flags,
get_key,
get_collection
);
if (fallback_fn !== null) {
reconcile(each_effect, array, state, anchor, flags, get_key);
if (fallback !== null) {
if (array.length === 0) {
if (fallback) {
resume_effect(fallback);
if (fallback.fragment) {
anchor.before(fallback.fragment);
fallback.fragment = null;
} else {
fallback = branch(() => fallback_fn(anchor));
resume_effect(fallback.effect);
}
} else if (fallback !== null) {
pause_effect(fallback, () => {
each_effect.first = fallback.effect;
} else {
pause_effect(fallback.effect, () => {
// TODO only null out if no pending batch needs it,
// otherwise re-add `fallback.fragment` and move the
// effect into it
fallback = null;
});
}
}
}
block(() => {
// store a reference to the effect so that we can update the start/end nodes in reconciliation
each_effect ??= /** @type {Effect} */ (active_effect);
var each_effect = block(() => {
array = /** @type {V[]} */ (get(each_array));
var length = array.length;
if (was_empty && length === 0) {
// ignore updates if the array is empty,
// and it already was empty on previous run
return;
}
was_empty = length === 0;
/** `true` if there was a hydration mismatch. Needs to be a `let` or else it isn't treeshaken out */
let mismatch = false;
@ -217,34 +209,46 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f
}
}
// this is separate to the previous block because `hydrating` might change
if (hydrating) {
/** @type {EachItem | null} */
var prev = null;
/** @type {EachItem} */
var item;
for (var i = 0; i < length; i++) {
if (
hydrate_node.nodeType === COMMENT_NODE &&
/** @type {Comment} */ (hydrate_node).data === HYDRATION_END
) {
// The server rendered fewer items than expected,
// so break out and continue appending non-hydrated items
anchor = /** @type {Comment} */ (hydrate_node);
mismatch = true;
set_hydrating(false);
break;
var keys = new Set();
var batch = /** @type {Batch} */ (current_batch);
var prev = null;
var defer = should_defer_append();
for (var i = 0; i < length; i += 1) {
if (
hydrating &&
hydrate_node.nodeType === COMMENT_NODE &&
/** @type {Comment} */ (hydrate_node).data === HYDRATION_END
) {
// The server rendered fewer items than expected,
// so break out and continue appending non-hydrated items
anchor = /** @type {Comment} */ (hydrate_node);
mismatch = true;
set_hydrating(false);
}
var value = array[i];
var key = get_key(value, i);
var item = first_run ? null : state.items.get(key);
if (item) {
// update before reconciliation, to trigger any async updates
if (is_reactive_value) {
internal_set(item.v, value);
}
var value = array[i];
var key = get_key(value, i);
if (is_reactive_index) {
internal_set(/** @type {Value<number>} */ (item.i), i);
} else {
item.i = i;
}
batch.skipped_effects.delete(item.e);
} else {
item = create_item(
hydrate_node,
state,
first_run ? anchor : null,
prev,
null,
value,
key,
i,
@ -252,65 +256,60 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f
flags,
get_collection
);
state.items.set(key, item);
prev = item;
}
if (first_run) {
item.o = true;
if (prev === null) {
state.first = item;
} else {
prev.next = item;
}
// remove excess nodes
if (length > 0) {
set_hydrate_node(skip_nodes());
prev = item;
}
state.items.set(key, item);
}
keys.add(key);
}
if (hydrating) {
if (length === 0 && fallback_fn) {
fallback = branch(() => fallback_fn(anchor));
if (length === 0 && fallback_fn && !fallback) {
if (first_run) {
fallback = {
fragment: null,
effect: branch(() => fallback_fn(anchor))
};
} else {
var fragment = document.createDocumentFragment();
var target = create_text();
fragment.append(target);
fallback = {
fragment,
effect: branch(() => fallback_fn(target))
};
}
} else {
if (should_defer_append()) {
var keys = new Set();
var batch = /** @type {Batch} */ (current_batch);
for (i = 0; i < length; i += 1) {
value = array[i];
key = get_key(value, i);
var existing = state.items.get(key) ?? offscreen_items.get(key);
if (existing) {
// update before reconciliation, to trigger any async updates
if ((flags & (EACH_ITEM_REACTIVE | EACH_INDEX_REACTIVE)) !== 0) {
update_item(existing, value, i, flags);
}
} else {
item = create_item(
null,
state,
null,
null,
value,
key,
i,
render_fn,
flags,
get_collection,
true
);
offscreen_items.set(key, item);
}
}
keys.add(key);
}
// remove excess nodes
if (hydrating && length > 0) {
set_hydrate_node(skip_nodes());
}
for (const [key, item] of state.items) {
if (!keys.has(key)) {
batch.skipped_effects.add(item.e);
}
}
for (const [key, item] of state.items) {
if (!keys.has(key)) {
batch.skipped_effects.add(item.e);
}
}
if (!first_run) {
if (defer) {
batch.oncommit(commit);
batch.ondiscard(() => {
// TODO presumably we need to do something here?
});
} else {
commit();
}
@ -330,6 +329,8 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f
get(each_array);
});
first_run = false;
if (hydrating) {
anchor = hydrate_node;
}
@ -341,32 +342,17 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f
* @param {Effect} each_effect
* @param {Array<V>} array
* @param {EachState} state
* @param {Map<any, EachItem>} offscreen_items
* @param {Element | Comment | Text} anchor
* @param {(anchor: Node, item: MaybeSource<V>, index: number | Source<number>, collection: () => V[]) => void} render_fn
* @param {number} flags
* @param {(value: V, index: number) => any} get_key
* @param {() => V[]} get_collection
* @returns {void}
*/
function reconcile(
each_effect,
array,
state,
offscreen_items,
anchor,
render_fn,
flags,
get_key,
get_collection
) {
function reconcile(each_effect, array, state, anchor, flags, get_key) {
var is_animated = (flags & EACH_IS_ANIMATED) !== 0;
var should_update = (flags & (EACH_ITEM_REACTIVE | EACH_INDEX_REACTIVE)) !== 0;
var length = array.length;
var items = state.items;
var first = state.first;
var current = first;
var current = state.first;
/** @type {undefined | Set<EachItem>} */
var seen;
@ -399,12 +385,10 @@ function reconcile(
for (i = 0; i < length; i += 1) {
value = array[i];
key = get_key(value, i);
item = items.get(key);
item = /** @type {EachItem} */ (items.get(key));
if (item !== undefined) {
item.a?.measure();
(to_animate ??= new Set()).add(item);
}
item.a?.measure();
(to_animate ??= new Set()).add(item);
}
}
@ -412,40 +396,20 @@ function reconcile(
value = array[i];
key = get_key(value, i);
item = items.get(key);
if (item === undefined) {
var pending = offscreen_items.get(key);
item = /** @type {EachItem} */ (items.get(key));
if (pending !== undefined) {
offscreen_items.delete(key);
items.set(key, pending);
state.first ??= item;
var next = prev ? prev.next : current;
if (!item.o) {
item.o = true;
link(state, prev, pending);
link(state, pending, next);
var next = prev ? prev.next : current;
move(pending, next, anchor);
prev = pending;
} else {
var child_anchor = current ? /** @type {TemplateNode} */ (current.e.nodes_start) : anchor;
prev = create_item(
child_anchor,
state,
prev,
prev === null ? state.first : prev.next,
value,
key,
i,
render_fn,
flags,
get_collection
);
}
link(state, prev, item);
link(state, item, next);
items.set(key, prev);
move(item, next, anchor);
prev = item;
matched = [];
stashed = [];
@ -454,10 +418,6 @@ function reconcile(
continue;
}
if (should_update) {
update_item(item, value, i, flags);
}
if ((item.e.f & INERT) !== 0) {
resume_effect(item.e);
if (is_animated) {
@ -575,63 +535,30 @@ function reconcile(
});
}
// TODO i have an inkling that the rest of this function is wrong...
// the offscreen items need to be linked, so that they all update correctly.
// the last onscreen item should link to the first offscreen item, etc
each_effect.first = state.first && state.first.e;
each_effect.last = prev && prev.e;
for (var unused of offscreen_items.values()) {
destroy_effect(unused.e);
}
offscreen_items.clear();
}
/**
* @param {EachItem} item
* @param {any} value
* @param {number} index
* @param {number} type
* @returns {void}
*/
function update_item(item, value, index, type) {
if ((type & EACH_ITEM_REACTIVE) !== 0) {
internal_set(item.v, value);
}
if ((type & EACH_INDEX_REACTIVE) !== 0) {
internal_set(/** @type {Value<number>} */ (item.i), index);
} else {
item.i = index;
if (prev) {
prev.e.next = null;
}
}
/**
* @template V
* @param {Node | null} anchor
* @param {EachState} state
* @param {EachItem | null} prev
* @param {EachItem | null} next
* @param {V} value
* @param {unknown} key
* @param {number} index
* @param {(anchor: Node, item: V | Source<V>, index: number | Value<number>, collection: () => V[]) => void} render_fn
* @param {number} flags
* @param {() => V[]} get_collection
* @param {boolean} [deferred]
* @returns {EachItem}
*/
function create_item(
anchor,
state,
prev,
next,
value,
key,
index,
render_fn,
flags,
get_collection,
deferred
) {
function create_item(anchor, prev, value, key, index, render_fn, flags, get_collection) {
var previous_each_item = current_each_item;
var reactive = (flags & EACH_ITEM_REACTIVE) !== 0;
var mutable = (flags & EACH_ITEM_IMMUTABLE) === 0;
@ -657,8 +584,9 @@ function create_item(
a: null,
// @ts-expect-error
e: null,
o: false,
prev,
next
next: null
};
current_each_item = item;
@ -669,25 +597,15 @@ function create_item(
fragment.append((anchor = create_text()));
}
item.e = branch(() => render_fn(/** @type {Node} */ (anchor), v, i, get_collection), hydrating);
item.e = branch(() => render_fn(/** @type {Node} */ (anchor), v, i, get_collection));
item.e.prev = prev && prev.e;
item.e.next = next && next.e;
if (prev === null) {
if (!deferred) {
state.first = item;
}
} else {
if (prev !== null) {
prev.next = item;
prev.e.next = item.e;
}
if (next !== null) {
next.prev = item;
next.e.prev = item.e;
}
return item;
} finally {
current_each_item = previous_each_item;

70
packages/svelte/src/internal/client/reactivity/effects.js vendored
Unescape View File

@ -80,10 +80,9 @@ function push_effect(effect, parent_effect) {
* @param {number} type
* @param {null | (() => void | (() => void))} fn
* @param {boolean} sync
* @param {boolean} push
* @returns {Effect}
*/
function create_effect(type, fn, sync, push = true) {
function create_effect(type, fn, sync) {
var parent = active_effect;
if (DEV) {
@ -133,43 +132,41 @@ function create_effect(type, fn, sync, push = true) {
schedule_effect(effect);
}
if (push) {
/** @type {Effect | null} */
var e = effect;
/** @type {Effect | null} */
var e = effect;
// if an effect has already ran and doesn't need to be kept in the tree
// (because it won't re-run, has no DOM, and has no teardown etc)
// then we skip it and go to its child (if any)
if (
sync &&
e.deps === null &&
e.teardown === null &&
e.nodes_start === null &&
e.first === e.last && // either `null`, or a singular child
(e.f & EFFECT_PRESERVED) === 0
) {
e = e.first;
if ((type & BLOCK_EFFECT) !== 0 && (type & EFFECT_TRANSPARENT) !== 0 && e !== null) {
e.f |= EFFECT_TRANSPARENT;
}
// if an effect has already ran and doesn't need to be kept in the tree
// (because it won't re-run, has no DOM, and has no teardown etc)
// then we skip it and go to its child (if any)
if (
sync &&
e.deps === null &&
e.teardown === null &&
e.nodes_start === null &&
e.first === e.last && // either `null`, or a singular child
(e.f & EFFECT_PRESERVED) === 0
) {
e = e.first;
if ((type & BLOCK_EFFECT) !== 0 && (type & EFFECT_TRANSPARENT) !== 0 && e !== null) {
e.f |= EFFECT_TRANSPARENT;
}
}
if (e !== null) {
e.parent = parent;
if (e !== null) {
e.parent = parent;
if (parent !== null) {
push_effect(e, parent);
}
if (parent !== null) {
push_effect(e, parent);
}
// if we're in a derived, add the effect there too
if (
active_reaction !== null &&
(active_reaction.f & DERIVED) !== 0 &&
(type & ROOT_EFFECT) === 0
) {
var derived = /** @type {Derived} */ (active_reaction);
(derived.effects ??= []).push(e);
}
// if we're in a derived, add the effect there too
if (
active_reaction !== null &&
(active_reaction.f & DERIVED) !== 0 &&
(type & ROOT_EFFECT) === 0
) {
var derived = /** @type {Derived} */ (active_reaction);
(derived.effects ??= []).push(e);
}
}
@ -406,10 +403,9 @@ export function block(fn, flags = 0) {
/**
* @param {(() => void)} fn
* @param {boolean} [push]
*/
export function branch(fn, push = true) {
return create_effect(BRANCH_EFFECT | EFFECT_PRESERVED, fn, true, push);
export function branch(fn) {
return create_effect(BRANCH_EFFECT | EFFECT_PRESERVED, fn, true);
}
/**

2
packages/svelte/src/internal/client/types.d.ts vendored
Unescape View File

@ -83,6 +83,8 @@ export type EachItem = {
i: number | Source<number>;
/** key */
k: unknown;
/** true if onscreen */
o: boolean;
prev: EachItem | null;
next: EachItem | null;
};

20
packages/svelte/tests/runtime-runes/samples/async-each-derived/_config.js
Unescape View File

@ -0,0 +1,20 @@
import { tick } from 'svelte';
import { test } from '../../test';
export default test({
async test({ assert, target }) {
await tick(); // settle initial await
const checkBox = target.querySelector('input');
checkBox?.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`
<input type="checkbox"/>
<p>true</p>
`
);
}
});

11
packages/svelte/tests/runtime-runes/samples/async-each-derived/main.svelte
Unescape View File

@ -0,0 +1,11 @@
<script>
let checked = $state(false);
const foo = $derived(await checked);
</script>
<input type="checkbox" bind:checked />
{#each checked === foo && [1]}
<p>{checked}</p>
{/each}

102
packages/svelte/tests/runtime-runes/samples/async-each-overlap/_config.js
Unescape View File

@ -0,0 +1,102 @@
import { tick } from 'svelte';
import { test } from '../../test';
export default test({
skip: true,
async test({ assert, target }) {
const [add, shift] = target.querySelectorAll('button');
add.click();
await tick();
add.click();
await tick();
add.click();
await tick();
// TODO pending count / number of pushes is off
assert.htmlEqual(
target.innerHTML,
`
<button>add</button>
<button>shift</button>
<p>pending=6 values.length=1 values=[1]</p>
<div>not keyed:
<div>1</div>
</div>
<div>keyed:
<div>1</div>
</div>
`
);
shift.click();
await tick();
shift.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`
<button>add</button>
<button>shift</button>
<p>pending=4 values.length=2 values=[1,2]</p>
<div>not keyed:
<div>1</div>
<div>2</div>
</div>
<div>keyed:
<div>1</div>
<div>2</div>
</div>
`
);
shift.click();
await tick();
shift.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`
<button>add</button>
<button>shift</button>
<p>pending=2 values.length=3 values=[1,2,3]</p>
<div>not keyed:
<div>1</div>
<div>2</div>
<div>3</div>
</div>
<div>keyed:
<div>1</div>
<div>2</div>
<div>3</div>
</div>
`
);
shift.click();
await tick();
shift.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`
<button>add</button>
<button>shift</button>
<p>pending=0 values.length=4 values=[1,2,3,4]</p>
<div>not keyed:
<div>1</div>
<div>2</div>
<div>3</div>
<div>4</div>
</div>
<div>keyed:
<div>1</div>
<div>2</div>
<div>3</div>
<div>4</div>
</div>
`
);
}
});

48
packages/svelte/tests/runtime-runes/samples/async-each-overlap/main.svelte
Unescape View File

@ -0,0 +1,48 @@
<script>
let values = $state([1]);
const queue = [];
function push(v) {
if (v === 1) return v;
const p = Promise.withResolvers();
queue.push(() => p.resolve(v));
return p.promise;
}
function shift() {
const fn = queue.shift();
if (fn) fn();
}
function addValue() {
values.push(values.length+1);
}
</script>
<button onclick={addValue}>add</button>
<button onclick={shift}>shift</button>
<p>
pending={$effect.pending()}
values.length={values.length}
values=[{values}]
</p>
<div>
not keyed:
{#each values as v}
<div>
{await push(v)}
</div>
{/each}
</div>
<div>
keyed:
{#each values as v(v)}
<div>
{await push(v)}
</div>
{/each}
</div>
Write Preview
Loading…
Cancel
Save