Skip to content
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

fix: wrap each block expression in derived to encapsulte effects #14967

Merged
merged 3 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from 2 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/tender-apples-scream.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: wrap each block expression in derived to encapsulte effects
Rich-Harris marked this conversation as resolved.
Show resolved Hide resolved
19 changes: 11 additions & 8 deletions packages/svelte/src/internal/client/dom/blocks/each.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@ import { source, mutable_source, internal_set } from '../../reactivity/sources.j
import { array_from, is_array } from '../../../shared/utils.js';
import { INERT } from '../../constants.js';
import { queue_micro_task } from '../task.js';
import { active_effect, active_reaction } from '../../runtime.js';
import { active_effect, active_reaction, get } from '../../runtime.js';
import { DEV } from 'esm-env';
import { derived_safe_equal } from '../../reactivity/deriveds.js';

/**
* The row of a keyed each block that is currently updating. We track this
Expand Down Expand Up @@ -135,15 +136,17 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f

var was_empty = false;

block(() => {
// 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
var each_array = derived_safe_equal(() => {
var collection = get_collection();

var array = is_array(collection)
? collection
: collection == null
? []
: array_from(collection);
return is_array(collection) ? collection : collection == null ? [] : array_from(collection);
});

block(() => {
var array = get(each_array);
var length = array.length;

if (was_empty && length === 0) {
Expand Down Expand Up @@ -254,7 +257,7 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f
// that a mutation occurred and it's made the collection MAYBE_DIRTY, so reading the
// collection again can provide consistency to the reactive graph again as the deriveds
// will now be `CLEAN`.
get_collection();
get(each_array);
});

if (hydrating) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { flushSync } from 'svelte';
import { test } from '../../test';

export default test({
async test({ assert, target, logs }) {
const [btn1] = target.querySelectorAll('button');

btn1.click();
flushSync();

await Promise.resolve();
await Promise.resolve();

assert.deepEqual(logs, ['cleanup']);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<script>
import { createSubscriber } from 'svelte/reactivity';

class MyStore {
#subscribe;
#data = $state([
['a', [1, 2]],
['b', [3, 4]]
]);
#id;

constructor(options) {
options?.someBoolean;
this.#id = options?.id;
this.#subscribe = createSubscriber(() => {
debugger
return () => {
console.log('cleanup');
};
});
}

get data() {
this.#subscribe();
return this.#data;
}
set data(v) {
this.#data = v;
}
}

let storeOptions = $state({
someBoolean: false,
id: 0
});

let myStore = $derived(new MyStore(storeOptions));
</script>

<button
onclick={() => {
storeOptions.someBoolean = !storeOptions.someBoolean;
}}>+</button
>

{#each myStore.data as _}{/each}
Loading