diff --git a/.changeset/dirty-pianos-sparkle.md b/.changeset/dirty-pianos-sparkle.md new file mode 100644 index 0000000000..b3e4dd1d8c --- /dev/null +++ b/.changeset/dirty-pianos-sparkle.md @@ -0,0 +1,5 @@ +--- +'svelte': minor +--- + +feat: allow state created in deriveds/effects to be written/read locally without self-invalidation diff --git a/benchmarking/compare/index.js b/benchmarking/compare/index.js index a5fc6d10a9..9d8d279c35 100644 --- a/benchmarking/compare/index.js +++ b/benchmarking/compare/index.js @@ -2,7 +2,6 @@ import fs from 'node:fs'; import path from 'node:path'; import { execSync, fork } from 'node:child_process'; import { fileURLToPath } from 'node:url'; -import { benchmarks } from '../benchmarks.js'; // if (execSync('git status --porcelain').toString().trim()) { // console.error('Working directory is not clean'); diff --git a/benchmarking/compare/runner.js b/benchmarking/compare/runner.js index 6fa58e2bac..a2e8646379 100644 --- a/benchmarking/compare/runner.js +++ b/benchmarking/compare/runner.js @@ -1,7 +1,7 @@ -import { benchmarks } from '../benchmarks.js'; +import { reactivity_benchmarks } from '../benchmarks/reactivity/index.js'; const results = []; -for (const benchmark of benchmarks) { +for (const benchmark of reactivity_benchmarks) { const result = await benchmark(); console.error(result.benchmark); results.push(result); diff --git a/documentation/docs/98-reference/.generated/client-errors.md b/documentation/docs/98-reference/.generated/client-errors.md index 0beb3cb9a9..62d9c3302a 100644 --- a/documentation/docs/98-reference/.generated/client-errors.md +++ b/documentation/docs/98-reference/.generated/client-errors.md @@ -122,12 +122,6 @@ Property descriptors defined on `$state` objects must contain `value` and always 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 ``` diff --git a/packages/svelte/messages/client-errors/errors.md b/packages/svelte/messages/client-errors/errors.md index ab4d1519c1..bc8ec36256 100644 --- a/packages/svelte/messages/client-errors/errors.md +++ b/packages/svelte/messages/client-errors/errors.md @@ -80,10 +80,6 @@ See the [migration guide](/docs/svelte/v5-migration-guide#Components-are-no-long > 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 or a template expression is forbidden. If the value should not be reactive, declare it without `$state` diff --git a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js index a93be76872..b9c8dfd99c 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js @@ -219,7 +219,10 @@ export function client_component(analysis, options) { for (const [name, binding] of analysis.instance.scope.declarations) { if (binding.kind === 'legacy_reactive') { legacy_reactive_declarations.push( - b.const(name, b.call('$.mutable_state', undefined, analysis.immutable ? b.true : undefined)) + b.const( + name, + b.call('$.mutable_source', undefined, analysis.immutable ? b.true : undefined) + ) ); } if (binding.kind === 'store_sub') { 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 5ff9945fa6..150c56e166 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 @@ -69,14 +69,7 @@ function build_assignment(operator, left, right, context) { is_non_coercive_operator(operator) && should_proxy(value, context.state.scope); - return b.call( - // inside the constructor, we use `$.simple_set` rather than using `$.set`, - // that only assign the value and eventually call onchange since nothing is tracking the signal at this point - context.state.in_constructor ? '$.simple_set' : '$.set', - left, - value, - needs_proxy && b.true - ); + return b.call('$.set', left, value, needs_proxy && b.true); } } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/VariableDeclaration.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/VariableDeclaration.js index 5e3c329dbf..6656f55a82 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/VariableDeclaration.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/VariableDeclaration.js @@ -305,7 +305,7 @@ function create_state_declarators(declarator, { scope, analysis }, value) { return [ b.declarator( declarator.id, - b.call('$.mutable_state', value, analysis.immutable ? b.true : undefined) + b.call('$.mutable_source', value, analysis.immutable ? b.true : undefined) ) ]; } @@ -320,7 +320,7 @@ function create_state_declarators(declarator, { scope, analysis }, value) { return b.declarator( path.node, binding?.kind === 'state' - ? b.call('$.mutable_state', value, analysis.immutable ? b.true : undefined) + ? b.call('$.mutable_source', value, analysis.immutable ? b.true : undefined) : value ); }) diff --git a/packages/svelte/src/internal/client/constants.js b/packages/svelte/src/internal/client/constants.js index 5f24792eae..c6f0e80ab9 100644 --- a/packages/svelte/src/internal/client/constants.js +++ b/packages/svelte/src/internal/client/constants.js @@ -20,6 +20,7 @@ export const LEGACY_DERIVED_PROP = 1 << 17; export const INSPECT_EFFECT = 1 << 18; export const HEAD_EFFECT = 1 << 19; export const EFFECT_HAS_DERIVED = 1 << 20; +export const EFFECT_IS_UPDATING = 1 << 21; export const STATE_SYMBOL = Symbol('$state'); export const PROXY_ONCHANGE_SYMBOL = Symbol('proxy onchange'); diff --git a/packages/svelte/src/internal/client/errors.js b/packages/svelte/src/internal/client/errors.js index 682816e1d6..8a5b5033a7 100644 --- a/packages/svelte/src/internal/client/errors.js +++ b/packages/svelte/src/internal/client/errors.js @@ -307,21 +307,6 @@ 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\nhttps://svelte.dev/e/state_unsafe_local_read`); - - error.name = 'Svelte error'; - throw error; - } else { - throw new Error(`https://svelte.dev/e/state_unsafe_local_read`); - } -} - /** * Updating state inside a derived or a template expression is forbidden. If the value should not be reactive, declare it without `$state` * @returns {never} diff --git a/packages/svelte/src/internal/client/index.js b/packages/svelte/src/internal/client/index.js index 9b95d5eebc..b576d50402 100644 --- a/packages/svelte/src/internal/client/index.js +++ b/packages/svelte/src/internal/client/index.js @@ -114,11 +114,10 @@ export { user_pre_effect } from './reactivity/effects.js'; export { - mutable_state, + mutable_source, mutate, set, - simple_set, - state, + source as state, update, update_pre, get_options diff --git a/packages/svelte/src/internal/client/proxy.js b/packages/svelte/src/internal/client/proxy.js index e23d12bec5..4221822e0f 100644 --- a/packages/svelte/src/internal/client/proxy.js +++ b/packages/svelte/src/internal/client/proxy.js @@ -2,6 +2,7 @@ import { DEV } from 'esm-env'; import { UNINITIALIZED } from '../../constants.js'; import { tracing_mode_flag } from '../flags/index.js'; +import { get, active_effect, active_reaction, set_active_reaction } from './runtime.js'; import { component_context } from './context.js'; import { array_prototype, @@ -15,7 +16,6 @@ import { check_ownership, widen_ownership } from './dev/ownership.js'; import { get_stack } from './dev/tracing.js'; import * as e from './errors.js'; import { batch_onchange, set, source, state } from './reactivity/sources.js'; -import { active_effect, get } from './runtime.js'; const array_methods = ['push', 'pop', 'shift', 'unshift', 'splice', 'reverse', 'sort']; @@ -40,15 +40,17 @@ function clone_options(options) { : undefined; } +/** @type {ProxyMetadata | null} */ +var parent_metadata = null; + /** * @template T * @param {T} value * @param {ValueOptions} [_options] - * @param {ProxyMetadata | null} [parent] * @param {Source} [prev] dev mode only * @returns {T} */ -export function proxy(value, _options, parent = null, prev) { +export function proxy(value, _options, prev) { // if non-proxyable, or is already a proxy, return `value` if (typeof value !== 'object' || value === null) { return value; @@ -85,6 +87,31 @@ export function proxy(value, _options, parent = null, prev) { var version = source(0); var stack = DEV && tracing_mode_flag ? get_stack('CreatedAt') : null; + var reaction = active_reaction; + + /** + * @template T + * @param {() => T} fn + */ + var with_parent = (fn) => { + var previous_reaction = active_reaction; + set_active_reaction(reaction); + + /** @type {T} */ + var result; + + if (DEV) { + var previous_metadata = parent_metadata; + parent_metadata = metadata; + result = fn(); + parent_metadata = previous_metadata; + } else { + result = fn(); + } + + set_active_reaction(previous_reaction); + return result; + }; if (is_proxied_array) { // We need to create the length source eagerly to ensure that @@ -100,7 +127,7 @@ export function proxy(value, _options, parent = null, prev) { if (DEV) { metadata = { - parent, + parent: parent_metadata, owners: null }; @@ -112,7 +139,7 @@ export function proxy(value, _options, parent = null, prev) { metadata.owners = prev_owners ? new Set(prev_owners) : null; } else { metadata.owners = - parent === null + parent_metadata === null ? component_context !== null ? new Set([component_context.function]) : null @@ -138,10 +165,13 @@ export function proxy(value, _options, parent = null, prev) { var s = sources.get(prop); if (s === undefined) { - s = source(descriptor.value, clone_options(options), stack); + s = with_parent(() => source(descriptor.value, clone_options(options), stack)); sources.set(prop, s); } else { - set(s, proxy(descriptor.value, options, metadata)); + set( + s, + with_parent(() => proxy(descriptor.value, options)) + ); } return true; @@ -152,7 +182,10 @@ export function proxy(value, _options, parent = null, prev) { if (s === undefined) { if (prop in target) { - sources.set(prop, source(UNINITIALIZED, clone_options(options), stack)); + sources.set( + prop, + with_parent(() => source(UNINITIALIZED, clone_options(options), stack)) + ); } } else { // When working with arrays, we need to also ensure we update the length when removing @@ -217,7 +250,9 @@ export function proxy(value, _options, 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)) { let opt = clone_options(options); - s = source(proxy(exists ? target[prop] : UNINITIALIZED, opt, metadata), opt, stack); + s = with_parent(() => + source(proxy(exists ? target[prop] : UNINITIALIZED, opt), opt, stack) + ); sources.set(prop, s); } @@ -296,7 +331,7 @@ export function proxy(value, _options, parent = null, prev) { ) { if (s === undefined) { let opt = clone_options(options); - s = source(has ? proxy(target[prop], opt, metadata) : UNINITIALIZED, opt, stack); + s = with_parent(() => source(has ? proxy(target[prop], opt) : UNINITIALIZED, opt, stack)); sources.set(prop, s); } @@ -334,7 +369,7 @@ export function proxy(value, _options, parent = null, prev) { // If the item exists in the original, we need to create a uninitialized source, // else a later read of the property would result in a source being created with // the value of the original item at that index. - other_s = source(UNINITIALIZED, clone_options(options), stack); + other_s = with_parent(() => source(UNINITIALIZED, clone_options(options), stack)); sources.set(i + '', other_s); } } @@ -347,8 +382,11 @@ export function proxy(value, _options, parent = null, prev) { if (s === undefined) { if (!has || get_descriptor(target, prop)?.writable) { const opt = clone_options(options); - s = source(undefined, opt, stack); - set(s, proxy(value, opt, metadata)); + s = with_parent(() => source(undefined, opt, stack)); + set( + s, + with_parent(() => proxy(value, opt)) + ); sources.set(prop, s); } } else { @@ -358,7 +396,10 @@ export function proxy(value, _options, parent = null, prev) { if (typeof s.v === 'object' && s.v !== null && STATE_SYMBOL in s.v) { s.v[PROXY_ONCHANGE_SYMBOL](options?.onchange, true); } - set(s, proxy(value, clone_options(options), metadata)); + set( + s, + with_parent(() => proxy(value, clone_options(options))) + ); } })(); if (DEV) { diff --git a/packages/svelte/src/internal/client/reactivity/sources.js b/packages/svelte/src/internal/client/reactivity/sources.js index dcee1f12cd..881a5e173d 100644 --- a/packages/svelte/src/internal/client/reactivity/sources.js +++ b/packages/svelte/src/internal/client/reactivity/sources.js @@ -11,8 +11,8 @@ import { untrack, increment_write_version, update_effect, - derived_sources, - set_derived_sources, + reaction_sources, + set_reaction_sources, check_dirtiness, untracking, is_destroying_effect @@ -28,7 +28,8 @@ import { MAYBE_DIRTY, BLOCK_EFFECT, ROOT_EFFECT, - PROXY_ONCHANGE_SYMBOL + PROXY_ONCHANGE_SYMBOL, + EFFECT_IS_UPDATING } from '../constants.js'; import * as e from '../errors.js'; import { legacy_mode_flag, tracing_mode_flag } from '../../flags/index.js'; @@ -79,6 +80,7 @@ export function batch_onchange(fn) { * @param {Error | null} [stack] * @returns {Source} */ +// TODO rename this to `state` throughout the codebase export function source(v, o, stack) { /** @type {Value} */ var signal = { @@ -91,6 +93,14 @@ export function source(v, o, stack) { o }; + if (active_reaction !== null && active_reaction.f & EFFECT_IS_UPDATING) { + if (reaction_sources === null) { + set_reaction_sources([signal]); + } else { + reaction_sources.push(signal); + } + } + if (DEV && tracing_mode_flag) { signal.created = stack ?? get_stack('CreatedAt'); signal.debug = null; @@ -99,14 +109,7 @@ export function source(v, o, stack) { return signal; } -/** - * @template V - * @param {V} v - * @param {ValueOptions} [o] - */ -export function state(v, o) { - return push_derived_source(source(v, o)); -} +export { source as state }; /** * @param {Source} source @@ -138,33 +141,6 @@ export function mutable_source(initial_value, immutable = false) { return s; } -/** - * @template V - * @param {V} v - * @param {boolean} [immutable] - * @returns {Source} - */ -export function mutable_state(v, immutable = false) { - return push_derived_source(mutable_source(v, immutable)); -} - -/** - * @template V - * @param {Source} source - */ -/*#__NO_SIDE_EFFECTS__*/ -function push_derived_source(source) { - if (active_reaction !== null && !untracking && (active_reaction.f & DERIVED) !== 0) { - if (derived_sources === null) { - set_derived_sources([source]); - } else { - derived_sources.push(source); - } - } - - return source; -} - /** * @template V * @param {Value} source @@ -191,43 +167,20 @@ export function set(source, value, should_proxy = false) { !untracking && is_runes() && (active_reaction.f & (DERIVED | BLOCK_EFFECT)) !== 0 && - // If the source was created locally within the current derived, then - // we allow the mutation. - (derived_sources === null || !derived_sources.includes(source)) + !reaction_sources?.includes(source) ) { e.state_unsafe_mutation(); } let new_value = should_proxy ? DEV - ? proxy(value, source.o, null, source) + ? proxy(value, source.o, source) : proxy(value, source.o) : value; return internal_set(source, new_value); } -/** - * @template V - * @param {Source} source - * @param {V} value - * @param {boolean} [should_proxy] - * @returns {V} - */ -export function simple_set(source, value, should_proxy = false) { - let new_value = should_proxy - ? DEV - ? proxy(value, source.o, null, source) - : proxy(value, source.o) - : value; - - source.v = new_value; - - source.o?.onchange?.(); - - return new_value; -} - /** * @template V * @param {Source} source diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 0a65c6e45a..74b58ee1a9 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -22,7 +22,8 @@ import { ROOT_EFFECT, LEGACY_DERIVED_PROP, DISCONNECTED, - BOUNDARY_EFFECT + BOUNDARY_EFFECT, + EFFECT_IS_UPDATING } from './constants.js'; import { flush_tasks } from './dom/task.js'; import { internal_set, old_values } from './reactivity/sources.js'; @@ -87,17 +88,17 @@ export function set_active_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. + * When sources are created within a reaction, reading and writing + * them should not cause a re-run * @type {null | Source[]} */ -export let derived_sources = null; +export let reaction_sources = null; /** * @param {Source[] | null} sources */ -export function set_derived_sources(sources) { - derived_sources = sources; +export function set_reaction_sources(sources) { + reaction_sources = sources; } /** @@ -367,6 +368,9 @@ function schedule_possible_effect_self_invalidation(signal, effect, root = true) for (var i = 0; i < reactions.length; i++) { var reaction = reactions[i]; + + if (reaction_sources?.includes(signal)) continue; + if ((reaction.f & DERIVED) !== 0) { schedule_possible_effect_self_invalidation(/** @type {Derived} */ (reaction), effect, false); } else if (effect === reaction) { @@ -391,9 +395,10 @@ export function update_reaction(reaction) { var previous_untracked_writes = untracked_writes; var previous_reaction = active_reaction; var previous_skip_reaction = skip_reaction; - var prev_derived_sources = derived_sources; + var previous_reaction_sources = reaction_sources; var previous_component_context = component_context; var previous_untracking = untracking; + var flags = reaction.f; new_deps = /** @type {null | Value[]} */ (null); @@ -403,11 +408,13 @@ export function update_reaction(reaction) { (flags & UNOWNED) !== 0 && (untracking || !is_updating_effect || active_reaction === null); active_reaction = (flags & (BRANCH_EFFECT | ROOT_EFFECT)) === 0 ? reaction : null; - derived_sources = null; + reaction_sources = null; set_component_context(reaction.ctx); untracking = false; read_version++; + reaction.f |= EFFECT_IS_UPDATING; + try { var result = /** @type {Function} */ (0, reaction.fn)(); var deps = reaction.deps; @@ -477,9 +484,11 @@ export function update_reaction(reaction) { untracked_writes = previous_untracked_writes; active_reaction = previous_reaction; skip_reaction = previous_skip_reaction; - derived_sources = prev_derived_sources; + reaction_sources = previous_reaction_sources; set_component_context(previous_component_context); untracking = previous_untracking; + + reaction.f ^= EFFECT_IS_UPDATING; } } @@ -866,9 +875,6 @@ export function get(signal) { // Register the dependency on the current reaction signal. if (active_reaction !== null && !untracking) { - if (derived_sources !== null && derived_sources.includes(signal)) { - e.state_unsafe_local_read(); - } var deps = active_reaction.deps; if (signal.rv < read_version) { signal.rv = read_version; diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index ef4cf16d3b..72f99c90e5 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -8,7 +8,12 @@ import { render_effect, user_effect } from '../../src/internal/client/reactivity/effects'; -import { state, set, update, update_pre } from '../../src/internal/client/reactivity/sources'; +import { + source as state, + set, + update, + update_pre +} from '../../src/internal/client/reactivity/sources'; import type { Derived, Effect, Value } from '../../src/internal/client/types'; import { proxy } from '../../src/internal/client/proxy'; import { derived } from '../../src/internal/client/reactivity/deriveds'; @@ -487,6 +492,26 @@ describe('signals', () => { }; }); + test('schedules rerun when updating deeply nested value', (runes) => { + if (!runes) return () => {}; + + const value = proxy({ a: { b: { c: 0 } } }); + user_effect(() => { + value.a.b.c += 1; + }); + + return () => { + let errored = false; + try { + flushSync(); + } catch (e: any) { + assert.include(e.message, 'effect_update_depth_exceeded'); + errored = true; + } + assert.equal(errored, true); + }; + }); + test('schedules rerun when writing to signal before reading it', (runes) => { if (!runes) return () => {}; @@ -958,14 +983,30 @@ describe('signals', () => { }; }); - test('deriveds cannot depend on state they own', () => { + test('deriveds do not depend on state they own', () => { return () => { + let s; + const d = derived(() => { - const s = state(0); + s = state(0); return $.get(s); }); - assert.throws(() => $.get(d), 'state_unsafe_local_read'); + assert.equal($.get(d), 0); + + set(s!, 1); + assert.equal($.get(d), 0); + }; + }); + + test('effects do not depend on state they own', () => { + user_effect(() => { + const value = state(0); + set(value, $.get(value) + 1); + }); + + return () => { + flushSync(); }; }); 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 e50e0a7ffc..2133974176 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 @@ -19,9 +19,9 @@ export default function Class_state_field_constructor_assignment($$anchor, $$pro constructor() { this.a = 1; - $.simple_set(this.#b, 2); + $.set(this.#b, 2); } } $.pop(); -} +} \ No newline at end of file