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

[Rich-Harris]

Inside a derived expression, $effect.active() should be true, since this likely determines whether or not effects that affect the derived value are created

Demo here

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

🦋 Changeset detected

Latest commit: f719ea3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Rich-Harris]

Hmm, this isn't quite ready. The motivation for this change was so that things like activeElement.current would update in cases like this, but that doesn't work because we currently disallow $effect inside $derived unless it's the first time it's running. I think that's a mistake — we should probably only disallow $effect inside an unowned $derived.

[dummdidumm]

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?

[Rich-Harris]

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

[trueadm]

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.

[Rich-Harris]

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.

[Rich-Harris]

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

[Rich-Harris]

While trying to wrap my head round #11500 (review) I found a weird bug that I'm not sure how to fix. Going to take a break for a while because it's melted my brain.

[trueadm]

I can take a look tomorrow.

[Rich-Harris]

thanks — I updated the test so we have somewhere to start

[trueadm]

I pushed a fix. The problem was that deriveds aren't added to the effect tree, so when we traverse the tree when flushing, we never encounter deriveds. This is fine though, as all we need to do is also push the effect to the parent effect in the case of being created in a derived.