From 975918c602990aa838eaed8dcf102bf414dffc60 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Sat, 24 Aug 2024 16:48:49 +0100 Subject: [PATCH] fix: ensure assignments to state field inside constructor trigger effect (#12985) * fix: ensure assignments to state field inside constructor trigger effects * address feedback * address feedback * address feedback * add test * alternative approach * lint * error on reading local source in derived * build * add changeset for self-dependency error * revert unused changes * revert unused changes * Update packages/svelte/messages/client-errors/errors.md * regenerate * tweak --------- Co-authored-by: Rich Harris --- .changeset/clean-shirts-yawn.md | 5 +++ .changeset/five-maps-reflect.md | 5 +++ .../svelte/messages/client-errors/errors.md | 4 +++ .../client/visitors/AssignmentExpression.js | 14 -------- .../client/visitors/MemberExpression.js | 9 ----- packages/svelte/src/internal/client/errors.js | 16 +++++++++ packages/svelte/src/internal/client/proxy.js | 4 +-- .../src/internal/client/reactivity/sources.js | 35 +++++++++++++++---- .../src/internal/client/reactivity/store.js | 2 +- .../svelte/src/internal/client/runtime.js | 20 +++++++++++ .../class-state-effect-derived/_config.js | 20 +++++++++++ .../class-state-effect-derived/main.svelte | 16 +++++++++ .../samples/class-state-effect/_config.js | 20 +++++++++++ .../samples/class-state-effect/main.svelte | 14 ++++++++ .../_config.js | 20 +++++++++++ .../main.svelte | 20 +++++++++++ .../_expected/client/index.svelte.js | 2 +- 17 files changed, 192 insertions(+), 34 deletions(-) create mode 100644 .changeset/clean-shirts-yawn.md create mode 100644 .changeset/five-maps-reflect.md create mode 100644 packages/svelte/tests/runtime-runes/samples/class-state-effect-derived/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/class-state-effect-derived/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/class-state-effect/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/class-state-effect/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/class-state-extended-effect-derived/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/class-state-extended-effect-derived/main.svelte diff --git a/.changeset/clean-shirts-yawn.md b/.changeset/clean-shirts-yawn.md new file mode 100644 index 0000000000..12b113baa1 --- /dev/null +++ b/.changeset/clean-shirts-yawn.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +breaking: throw error if derived creates state and then depends on it diff --git a/.changeset/five-maps-reflect.md b/.changeset/five-maps-reflect.md new file mode 100644 index 0000000000..bdc8d6ec18 --- /dev/null +++ b/.changeset/five-maps-reflect.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: ensure assignments to state field inside constructor trigger effects diff --git a/packages/svelte/messages/client-errors/errors.md b/packages/svelte/messages/client-errors/errors.md index ee05f6e664..b334234714 100644 --- a/packages/svelte/messages/client-errors/errors.md +++ b/packages/svelte/messages/client-errors/errors.md @@ -72,6 +72,10 @@ > Cannot set prototype of `$state` object +## state_unsafe_local_read + +> Reading state that was created inside the same derived is forbidden. Consider using `untrack` to read locally created state + ## state_unsafe_mutation > Updating state inside a derived is forbidden. If the value should not be reactive, declare it without `$state` diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js index 4365870bf7..e4c630adb6 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/AssignmentExpression.js @@ -57,20 +57,6 @@ function build_assignment(operator, left, right, context) { return b.assignment(operator, /** @type {Pattern} */ (context.visit(left)), value); } } - } else if (left.property.type === 'Identifier' && context.state.in_constructor) { - const public_state = context.state.public_state.get(left.property.name); - - if (public_state !== undefined && should_proxy(right, context.state.scope)) { - const value = /** @type {Expression} */ (context.visit(right)); - - return b.assignment( - operator, - /** @type {Pattern} */ (context.visit(left)), - public_state.kind === 'raw_state' - ? value - : build_proxy_reassignment(value, public_state.id) - ); - } } } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/MemberExpression.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/MemberExpression.js index 801841dfb6..501ecda555 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/MemberExpression.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/MemberExpression.js @@ -13,15 +13,6 @@ export function MemberExpression(node, context) { if (field) { return context.state.in_constructor ? b.member(node, 'v') : b.call('$.get', node); } - } else if (node.object.type === 'ThisExpression') { - // rewrite `this.foo` as `this.#foo.v` inside a constructor - if (node.property.type === 'Identifier' && !node.computed) { - const field = context.state.public_state.get(node.property.name); - - if (field && context.state.in_constructor) { - return b.member(b.member(b.this, field.id), 'v'); - } - } } context.next(); diff --git a/packages/svelte/src/internal/client/errors.js b/packages/svelte/src/internal/client/errors.js index 67b5c0550f..ae09bb24d2 100644 --- a/packages/svelte/src/internal/client/errors.js +++ b/packages/svelte/src/internal/client/errors.js @@ -310,6 +310,22 @@ export function state_prototype_fixed() { } } +/** + * Reading state that was created inside the same derived is forbidden. Consider using `untrack` to read locally created state + * @returns {never} + */ +export function state_unsafe_local_read() { + if (DEV) { + const error = new Error(`state_unsafe_local_read\nReading state that was created inside the same derived is forbidden. Consider using \`untrack\` to read locally created state`); + + error.name = 'Svelte error'; + throw error; + } else { + // TODO print a link to the documentation + throw new Error("state_unsafe_local_read"); + } +} + /** * Updating state inside a derived is forbidden. If the value should not be reactive, declare it without `$state` * @returns {never} diff --git a/packages/svelte/src/internal/client/proxy.js b/packages/svelte/src/internal/client/proxy.js index 333cd20b58..812f3d64a3 100644 --- a/packages/svelte/src/internal/client/proxy.js +++ b/packages/svelte/src/internal/client/proxy.js @@ -118,7 +118,7 @@ export function proxy(value, parent = null, prev) { // create a source, but only if it's an own property and not a prototype property if (s === undefined && (!exists || get_descriptor(target, prop)?.writable)) { - s = source(proxy(exists ? target[prop] : UNINITIALIZED, metadata)); + s = source(proxy(exists ? target[prop] : UNINITIALIZED, metadata), null); sources.set(prop, s); } @@ -170,7 +170,7 @@ export function proxy(value, parent = null, prev) { (current_effect !== null && (!has || get_descriptor(target, prop)?.writable)) ) { if (s === undefined) { - s = source(has ? proxy(target[prop], metadata) : UNINITIALIZED); + s = source(has ? proxy(target[prop], metadata) : UNINITIALIZED, null); sources.set(prop, s); } diff --git a/packages/svelte/src/internal/client/reactivity/sources.js b/packages/svelte/src/internal/client/reactivity/sources.js index 86427a7509..4b3750ac99 100644 --- a/packages/svelte/src/internal/client/reactivity/sources.js +++ b/packages/svelte/src/internal/client/reactivity/sources.js @@ -1,4 +1,4 @@ -/** @import { Derived, Effect, Source, Value } from '#client' */ +/** @import { Derived, Effect, Reaction, Source, Value } from '#client' */ import { DEV } from 'esm-env'; import { current_component_context, @@ -13,7 +13,9 @@ import { set_signal_status, untrack, increment_version, - update_effect + update_effect, + derived_sources, + set_derived_sources } from '../runtime.js'; import { equals, safe_equals } from './equality.js'; import { @@ -32,27 +34,39 @@ let inspect_effects = new Set(); /** * @template V * @param {V} v + * @param {Reaction | null} [owner] * @returns {Source} */ /*#__NO_SIDE_EFFECTS__*/ -export function source(v) { - return { +export function source(v, owner = current_reaction) { + var source = { f: 0, // TODO ideally we could skip this altogether, but it causes type errors v, reactions: null, equals, version: 0 }; + + if (owner !== null && (owner.f & DERIVED) !== 0) { + if (derived_sources === null) { + set_derived_sources([source]); + } else { + derived_sources.push(source); + } + } + + return source; } /** * @template V * @param {V} initial_value + * @param {Reaction | null} [owner] * @returns {Source} */ /*#__NO_SIDE_EFFECTS__*/ -export function mutable_source(initial_value) { - const s = source(initial_value); +export function mutable_source(initial_value, owner) { + const s = source(initial_value, owner); s.equals = safe_equals; // bind the signal to the component context, in case we need to @@ -84,7 +98,14 @@ export function mutate(source, value) { * @returns {V} */ export function set(source, value) { - if (current_reaction !== null && is_runes() && (current_reaction.f & DERIVED) !== 0) { + if ( + current_reaction !== null && + is_runes() && + (current_reaction.f & DERIVED) !== 0 && + // If the source was created locally within the current derived, then + // we allow the mutation. + (derived_sources === null || !derived_sources.includes(source)) + ) { e.state_unsafe_mutation(); } diff --git a/packages/svelte/src/internal/client/reactivity/store.js b/packages/svelte/src/internal/client/reactivity/store.js index e28202aaf0..d95e42b0db 100644 --- a/packages/svelte/src/internal/client/reactivity/store.js +++ b/packages/svelte/src/internal/client/reactivity/store.js @@ -19,7 +19,7 @@ import { mutable_source, set } from './sources.js'; export function store_get(store, store_name, stores) { const entry = (stores[store_name] ??= { store: null, - source: mutable_source(undefined), + source: mutable_source(undefined, null), unsubscribe: noop }); diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 71d50209d1..de6476b71f 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -80,6 +80,20 @@ export function set_current_effect(effect) { current_effect = effect; } +/** + * When sources are created within a derived, we record them so that we can safely allow + * local mutations to these sources without the side-effect error being invoked unnecessarily. + * @type {null | Source[]} + */ +export let derived_sources = null; + +/** + * @param {Source[] | null} sources + */ +export function set_derived_sources(sources) { + derived_sources = sources; +} + /** * The dependencies of the reaction that is currently being executed. In many cases, * the dependencies are unchanged between runs, and so this will be `null` unless @@ -281,12 +295,14 @@ export function update_reaction(reaction) { var previous_untracked_writes = current_untracked_writes; var previous_reaction = current_reaction; var previous_skip_reaction = current_skip_reaction; + var prev_derived_sources = derived_sources; new_deps = /** @type {null | Value[]} */ (null); skipped_deps = 0; current_untracked_writes = null; current_reaction = (reaction.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0 ? reaction : null; current_skip_reaction = !is_flushing_effect && (reaction.f & UNOWNED) !== 0; + derived_sources = null; try { var result = /** @type {Function} */ (0, reaction.fn)(); @@ -323,6 +339,7 @@ export function update_reaction(reaction) { current_untracked_writes = previous_untracked_writes; current_reaction = previous_reaction; current_skip_reaction = previous_skip_reaction; + derived_sources = prev_derived_sources; } } @@ -700,6 +717,9 @@ export function get(signal) { // Register the dependency on the current reaction signal. if (current_reaction !== null) { + if (derived_sources !== null && derived_sources.includes(signal)) { + e.state_unsafe_local_read(); + } var deps = current_reaction.deps; // If the signal is accessing the same dependencies in the same diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-effect-derived/_config.js b/packages/svelte/tests/runtime-runes/samples/class-state-effect-derived/_config.js new file mode 100644 index 0000000000..8255aceae2 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-effect-derived/_config.js @@ -0,0 +1,20 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + html: ``, + + test({ assert, target, logs }) { + const btn = target.querySelector('button'); + + btn?.click(); + flushSync(); + assert.htmlEqual(target.innerHTML, ``); + + btn?.click(); + flushSync(); + assert.htmlEqual(target.innerHTML, ``); + + assert.deepEqual(logs, [0, 10, 11, 12]); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-effect-derived/main.svelte b/packages/svelte/tests/runtime-runes/samples/class-state-effect-derived/main.svelte new file mode 100644 index 0000000000..df0095a21d --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-effect-derived/main.svelte @@ -0,0 +1,16 @@ + + + diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-effect/_config.js b/packages/svelte/tests/runtime-runes/samples/class-state-effect/_config.js new file mode 100644 index 0000000000..8255aceae2 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-effect/_config.js @@ -0,0 +1,20 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + html: ``, + + test({ assert, target, logs }) { + const btn = target.querySelector('button'); + + btn?.click(); + flushSync(); + assert.htmlEqual(target.innerHTML, ``); + + btn?.click(); + flushSync(); + assert.htmlEqual(target.innerHTML, ``); + + assert.deepEqual(logs, [0, 10, 11, 12]); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-effect/main.svelte b/packages/svelte/tests/runtime-runes/samples/class-state-effect/main.svelte new file mode 100644 index 0000000000..b30b09be9d --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-effect/main.svelte @@ -0,0 +1,14 @@ + + + diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-extended-effect-derived/_config.js b/packages/svelte/tests/runtime-runes/samples/class-state-extended-effect-derived/_config.js new file mode 100644 index 0000000000..8255aceae2 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-extended-effect-derived/_config.js @@ -0,0 +1,20 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + html: ``, + + test({ assert, target, logs }) { + const btn = target.querySelector('button'); + + btn?.click(); + flushSync(); + assert.htmlEqual(target.innerHTML, ``); + + btn?.click(); + flushSync(); + assert.htmlEqual(target.innerHTML, ``); + + assert.deepEqual(logs, [0, 10, 11, 12]); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-extended-effect-derived/main.svelte b/packages/svelte/tests/runtime-runes/samples/class-state-extended-effect-derived/main.svelte new file mode 100644 index 0000000000..52a82b4247 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-extended-effect-derived/main.svelte @@ -0,0 +1,20 @@ + + + diff --git a/packages/svelte/tests/snapshot/samples/class-state-field-constructor-assignment/_expected/client/index.svelte.js b/packages/svelte/tests/snapshot/samples/class-state-field-constructor-assignment/_expected/client/index.svelte.js index 02b109636f..197b9d1e73 100644 --- a/packages/svelte/tests/snapshot/samples/class-state-field-constructor-assignment/_expected/client/index.svelte.js +++ b/packages/svelte/tests/snapshot/samples/class-state-field-constructor-assignment/_expected/client/index.svelte.js @@ -18,7 +18,7 @@ export default function Class_state_field_constructor_assignment($$anchor, $$pro #b = $.source(); constructor() { - this.#a.v = 1; + this.a = 1; this.#b.v = 2; } }