From 8742823e39f567ab2952fdabd9fc9148f88a6fc2 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 9 May 2024 12:20:52 -0400 Subject: [PATCH] fix: make `$effect.active()` true when updating deriveds (#11500) * fix: make `$effect.active()` true when updating deriveds * WIP * this seems to work? * prevent effects being created in unowned deriveds * update test * fix issue --------- Co-authored-by: Dominic Gannaway --- .changeset/serious-goats-tap.md | 5 +++ .../svelte/messages/client-errors/errors.md | 4 ++ .../svelte/src/internal/client/dev/inspect.js | 2 +- packages/svelte/src/internal/client/errors.js | 16 ++++++++ .../internal/client/reactivity/deriveds.js | 1 + .../src/internal/client/reactivity/effects.js | 39 ++++++++++++++---- .../samples/effect-active-derived/_config.js | 41 +++++++++++++++++++ .../samples/effect-active-derived/main.svelte | 31 ++++++++++++++ 8 files changed, 129 insertions(+), 10 deletions(-) create mode 100644 .changeset/serious-goats-tap.md create mode 100644 packages/svelte/tests/runtime-runes/samples/effect-active-derived/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/effect-active-derived/main.svelte diff --git a/.changeset/serious-goats-tap.md b/.changeset/serious-goats-tap.md new file mode 100644 index 0000000000..b746f1fa69 --- /dev/null +++ b/.changeset/serious-goats-tap.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: make `$effect.active()` true when updating deriveds diff --git a/packages/svelte/messages/client-errors/errors.md b/packages/svelte/messages/client-errors/errors.md index 119328c7d0..9333d2cfe9 100644 --- a/packages/svelte/messages/client-errors/errors.md +++ b/packages/svelte/messages/client-errors/errors.md @@ -24,6 +24,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) diff --git a/packages/svelte/src/internal/client/dev/inspect.js b/packages/svelte/src/internal/client/dev/inspect.js index 8b6b6e45a6..c9b9411786 100644 --- a/packages/svelte/src/internal/client/dev/inspect.js +++ b/packages/svelte/src/internal/client/dev/inspect.js @@ -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; diff --git a/packages/svelte/src/internal/client/errors.js b/packages/svelte/src/internal/client/errors.js index c8e9db5e90..76b715d013 100644 --- a/packages/svelte/src/internal/client/errors.js +++ b/packages/svelte/src/internal/client/errors.js @@ -111,6 +111,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 diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 6ebd945301..1e3a7276a4 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -121,6 +121,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); diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js index 823c9dbc7c..fe361dec41 100644 --- a/packages/svelte/src/internal/client/reactivity/effects.js +++ b/packages/svelte/src/internal/client/reactivity/effects.js @@ -26,7 +26,9 @@ 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'; @@ -34,12 +36,10 @@ 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); } @@ -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); } @@ -118,7 +130,15 @@ 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; + } + + if (current_effect) { + return (current_effect.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0; + } + + return false; } /** @@ -126,12 +146,13 @@ export function effect_active() { * @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; @@ -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); } diff --git a/packages/svelte/tests/runtime-runes/samples/effect-active-derived/_config.js b/packages/svelte/tests/runtime-runes/samples/effect-active-derived/_config.js new file mode 100644 index 0000000000..d82b1dc471 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/effect-active-derived/_config.js @@ -0,0 +1,41 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + html: ` + + + + `, + + test({ assert, target }) { + const [outer, inner, reset] = target.querySelectorAll('button'); + + flushSync(() => outer?.click()); + flushSync(() => inner?.click()); + + assert.htmlEqual( + target.innerHTML, + ` + + + +

v is true

+ ` + ); + + flushSync(() => reset?.click()); + flushSync(() => inner?.click()); + flushSync(() => outer?.click()); + + assert.htmlEqual( + target.innerHTML, + ` + + + +

v is true

+ ` + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/effect-active-derived/main.svelte b/packages/svelte/tests/runtime-runes/samples/effect-active-derived/main.svelte new file mode 100644 index 0000000000..e4d46edd77 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/effect-active-derived/main.svelte @@ -0,0 +1,31 @@ + + + + + + + + +{#if outer && v} +

v is true

+{/if}