Skip to content

fix: ensure unowned deriveds can add themselves as reactions while connected #16249

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
Jun 27, 2025
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/little-kings-smoke.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: ensure unowned deriveds can add themselves as reactions while connected
7 changes: 6 additions & 1 deletion packages/svelte/src/internal/client/runtime.js
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,12 @@ export function update_reaction(reaction) {
reaction.deps = deps = new_deps;
}

if (!skip_reaction) {
if (
!skip_reaction ||
// Deriveds that already have reactions can cleanup, so we still add them as reactions
((flags & DERIVED) !== 0 &&
/** @type {import('#client').Derived} */ (reaction).reactions !== null)
) {
for (i = skipped_deps; i < deps.length; i++) {
(deps[i].reactions ??= []).push(reaction);
}
Expand Down
39 changes: 39 additions & 0 deletions packages/svelte/tests/signals/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,45 @@ describe('signals', () => {
};
});

test('unowned deriveds are not added as reactions but trigger effects', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Is the name of the test correct? Isn't the change that we actually add the derived as a reaction? I think it makes sense: the problem is probably that we check if the derived is UNOWNED before executing it so if the execution changes that (connecting it) when we reach the point of the code you changes skip_reaction should be false (because now the derived it's not unowned anymore) but it's still true and you can check that by looking if it has reactions (or i suppose we could also check if (reaction.f & UNOWNED) !== 0 in line?)

Copy link
Member

Choose a reason for hiding this comment

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

Btw scratch the last sentence, I confused it and also the flag is never set to false

var obj = state<any>(undefined);

class C1 {
#v = state(0);
get v() {
return $.get(this.#v);
}
set v(v: number) {
set(this.#v, v);
}
}

return () => {
let d = derived(() => $.get(obj)?.v || '-');

const log: number[] = [];
assert.equal($.get(d), '-');

let destroy = effect_root(() => {
render_effect(() => {
log.push($.get(d));
});
});

set(obj, new C1());
flushSync();
assert.equal($.get(d), '-');
$.get(obj).v = 1;
flushSync();
assert.equal($.get(d), 1);
assert.deepEqual(log, ['-', 1]);
destroy();
// ensure we're not leaking reactions
assert.equal(obj.reactions, null);
assert.equal(d.reactions, null);
};
});

test('derived from state', () => {
const log: number[] = [];

Expand Down