Skip to content

fix: make $effect.active() true when updating deriveds #11500

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 6 commits into from
May 9, 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/serious-goats-tap.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: make `$effect.active()` true when updating deriveds
4 changes: 4 additions & 0 deletions packages/svelte/messages/client-errors/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@

> `%rune%` cannot be used inside an effect cleanup function

## effect_in_unowned_derived

> Effect cannot be created inside a `$derived` value that was not itself created inside an effect

## effect_orphan

> `%rune%` can only be used inside an effect (e.g. during component initialisation)
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/internal/client/dev/inspect.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export let inspect_captured_signals = [];
*/
// eslint-disable-next-line no-console
export function inspect(get_value, inspector = console.log) {
validate_effect(current_effect, '$inspect');
validate_effect('$inspect');

let initial = true;

Expand Down
16 changes: 16 additions & 0 deletions packages/svelte/src/internal/client/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,22 @@ export function effect_in_teardown(rune) {
}
}

/**
* Effect cannot be created inside a `$derived` value that was not itself created inside an effect
* @returns {never}
*/
export function effect_in_unowned_derived() {
if (DEV) {
const error = new Error(`${"effect_in_unowned_derived"}\n${"Effect cannot be created inside a `$derived` value that was not itself created inside an effect"}`);

error.name = 'Svelte error';
throw error;
} else {
// TODO print a link to the documentation
throw new Error("effect_in_unowned_derived");
}
}

/**
* `%rune%` can only be used inside an effect (e.g. during component initialisation)
* @param {string} rune
Expand Down
1 change: 1 addition & 0 deletions packages/svelte/src/internal/client/reactivity/deriveds.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ export function update_derived(derived, force_schedule) {
*/
export function destroy_derived(signal) {
destroy_derived_children(signal);
destroy_effect_children(signal);
remove_reactions(signal, 0);
set_signal_status(signal, DESTROYED);

Expand Down
39 changes: 30 additions & 9 deletions packages/svelte/src/internal/client/reactivity/effects.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,20 @@ import {
EFFECT_RAN,
BLOCK_EFFECT,
ROOT_EFFECT,
EFFECT_TRANSPARENT
EFFECT_TRANSPARENT,
DERIVED,
UNOWNED
} from '../constants.js';
import { set } from './sources.js';
import { remove } from '../dom/reconciler.js';
import * as e from '../errors.js';
import { DEV } from 'esm-env';

/**
* @param {import('#client').Effect | null} effect
* @param {'$effect' | '$effect.pre' | '$inspect'} rune
* @returns {asserts effect}
*/
export function validate_effect(effect, rune) {
if (effect === null) {
export function validate_effect(rune) {
if (current_effect === null && current_reaction === null) {
e.effect_orphan(rune);
}

Expand Down Expand Up @@ -93,6 +93,18 @@ function create_effect(type, fn, sync) {
}

if (current_reaction !== null && !is_root) {
var flags = current_reaction.f;
if ((flags & DERIVED) !== 0) {
if ((flags & UNOWNED) !== 0) {
e.effect_in_unowned_derived();
}
// If we are inside a derived, then we also need to attach the
// effect to the parent effect too.
if (current_effect !== null) {
push_effect(effect, current_effect);
}
}

push_effect(effect, current_reaction);
}

Expand All @@ -118,20 +130,29 @@ function create_effect(type, fn, sync) {
* @returns {boolean}
*/
export function effect_active() {
return current_effect ? (current_effect.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0 : false;
if (current_reaction && (current_reaction.f & DERIVED) !== 0) {
return (current_reaction.f & UNOWNED) === 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be?

if ((current_reaction.f & UNOWNED) === 0) return true

Then it goes to the next block to check for an active current effect.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we do, no.

As things stand today, if it did fall through to the current effect, and as a result $effect.active() returned true leading to a new effect being created, that new effect would be a child of the unowned derived. As such, it would never run — it would just sit there taking up memory until the derived was GC'd.

So we'd need a corresponding change inside create_effect:

diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js
index 0ab8da808..13c93b33c 100644
--- a/packages/svelte/src/internal/client/reactivity/effects.js
+++ b/packages/svelte/src/internal/client/reactivity/effects.js
@@ -92,8 +92,14 @@ function create_effect(type, fn, sync) {
 		effect.component_function = dev_current_component_function;
 	}
 
-	if (current_reaction !== null && !is_root) {
-		push_effect(effect, current_reaction);
+	let reaction = current_reaction;
+
+	if (reaction !== null && (reaction.f & (DERIVED | UNOWNED)) === (DERIVED | UNOWNED)) {
+		reaction = current_effect;
+	}
+
+	if (reaction !== null && !is_root) {
+		push_effect(effect, reaction);
 	}
 
 	if (sync) {
@@ -119,7 +125,7 @@ function create_effect(type, fn, sync) {
  */
 export function effect_active() {
 	if (current_reaction && (current_reaction.f & DERIVED) !== 0) {
-		return (current_reaction.f & UNOWNED) === 0;
+		if ((current_reaction.f & UNOWNED) === 0) return true;
 	}
 
 	if (current_effect) {

With that, effects created inside unowned deriveds do run as long as the derived value is being read inside an effect, as in this somewhat contrived example.

But there's a catch. If we read the derived outside an effect, the child effect isn't created because $effect.active() is false. Meanwhile, it updates to the latest value. If we then read it inside an effect, the value is already up to date, so the function doesn't re-run, and the child effect still isn't created. If you read the derived value inside an effect first, the child effect is created. This feels like a sufficiently confusing difference in behaviour that we'd be much better off enforcing that unowned deriveds can't contain effects.

Interestingly: in the demo above (on this branch and also with the changes above), the effect inside the owned derived is created but doesn't run if the derived is read inside an effect. But it does run if it's read outside an effect (try clicking 'read values' followed by 'increment'). That feels like a bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed that change to an effect-active-derived-allow-unowned branch for posterity

}

if (current_effect) {
return (current_effect.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0;
}

return false;
Copy link
Member

Choose a reason for hiding this comment

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

the order here seems slightly different to before. Before, if a derived was unowned but executed as part of an effect (i.e. current_effect was set) then it would be counted as active. Now it won't be anymore. Is that on purpose? If so, why?

Copy link
Member Author

Choose a reason for hiding this comment

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

See https://github.com/sveltejs/svelte/pull/11500/files#r1594543348. I think it's important that child effects belong directly to the derived, and not to a surrounding effect, otherwise you can end up with some confusing inconsistencies

}

/**
* Internal representation of `$effect(...)`
* @param {() => void | (() => void)} fn
*/
export function user_effect(fn) {
validate_effect(current_effect, '$effect');
validate_effect('$effect');

// Non-nested `$effect(...)` in a component should be deferred
// until the component is mounted
const defer =
current_effect.f & RENDER_EFFECT &&
current_effect !== null &&
(current_effect.f & RENDER_EFFECT) !== 0 &&
// TODO do we actually need this? removing them changes nothing
current_component_context !== null &&
!current_component_context.m;
Expand All @@ -150,7 +171,7 @@ export function user_effect(fn) {
* @returns {import('#client').Effect}
*/
export function user_pre_effect(fn) {
validate_effect(current_effect, '$effect.pre');
validate_effect('$effect.pre');
return render_effect(fn);
}

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

export default test({
html: `
<button>toggle outer</button>
<button>toggle inner</button>
<button>reset</button>
`,

test({ assert, target }) {
const [outer, inner, reset] = target.querySelectorAll('button');

flushSync(() => outer?.click());
flushSync(() => inner?.click());

assert.htmlEqual(
target.innerHTML,
`
<button>toggle outer</button>
<button>toggle inner</button>
<button>reset</button>
<p>v is true</p>
`
);

flushSync(() => reset?.click());
flushSync(() => inner?.click());
flushSync(() => outer?.click());

assert.htmlEqual(
target.innerHTML,
`
<button>toggle outer</button>
<button>toggle inner</button>
<button>reset</button>
<p>v is true</p>
`
);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<script>
let value = $state(false);
const fn = () => {
if ($effect.active()) {
$effect(() => {
value = true;
});
}
return value;
};

let outer = $state(false);
let inner = $state(false);
let v = $derived(inner ? fn() : false);
</script>

<button onclick={() => outer = !outer}>
toggle outer
</button>

<button onclick={() => inner = !inner}>
toggle inner
</button>

<button onclick={() => outer = inner = value = false}>
reset
</button>

{#if outer && v}
<p>v is true</p>
{/if}