diff --git a/.changeset/witty-turtles-bake.md b/.changeset/witty-turtles-bake.md new file mode 100644 index 0000000000..04637516f8 --- /dev/null +++ b/.changeset/witty-turtles-bake.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +chore: turn reactive_declaration_non_reactive_property into a runtime warning diff --git a/documentation/docs/98-reference/.generated/client-warnings.md b/documentation/docs/98-reference/.generated/client-warnings.md index e9f1a88c3c..6e97211341 100644 --- a/documentation/docs/98-reference/.generated/client-warnings.md +++ b/documentation/docs/98-reference/.generated/client-warnings.md @@ -86,6 +86,40 @@ Mutating a value outside the component that created it is strongly discouraged. %component% mutated a value owned by %owner%. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead ``` +### reactive_declaration_non_reactive_property + +``` +A `$:` statement (%location%) read reactive state that was not visible to the compiler. Updates to this state will not cause the statement to re-run. The behaviour of this code will change if you migrate it to runes mode +``` + +In legacy mode, a `$:` [reactive statement](https://svelte.dev/docs/svelte/legacy-reactive-assignments) re-runs when the state it _references_ changes. This is determined at compile time, by analysing the code. + +In runes mode, effects and deriveds re-run when there are changes to the values that are read during the function's _execution_. + +Often, the result is the same — for example these can be considered equivalent: + +```js +$: sum = a + b; +``` + +```js +const sum = $derived(a + b); +``` + +In some cases — such as the one that triggered the above warning — they are _not_ the same: + +```js +const add = () => a + b; + +// the compiler can't 'see' that `sum` depends on `a` and `b`, but +// they _would_ be read while executing the `$derived` version +$: sum = add(); +``` + +Similarly, reactive properties of [deep state](https://svelte.dev/docs/svelte/$state#Deep-state) are not visible to the compiler. As such, changes to these properties will cause effects and deriveds to re-run but will _not_ cause `$:` statements to re-run. + +When you [migrate this component](https://svelte.dev/docs/svelte/v5-migration-guide) to runes mode, the behaviour will change accordingly. + ### state_proxy_equality_mismatch ``` diff --git a/documentation/docs/98-reference/.generated/compile-warnings.md b/documentation/docs/98-reference/.generated/compile-warnings.md index 775e0681c9..cc948e2547 100644 --- a/documentation/docs/98-reference/.generated/compile-warnings.md +++ b/documentation/docs/98-reference/.generated/compile-warnings.md @@ -726,12 +726,6 @@ Reactive declarations only exist at the top level of the instance script Reassignments of module-level declarations will not cause reactive statements to update ``` -### reactive_declaration_non_reactive_property - -``` -Properties of objects and arrays are not reactive unless in runes mode. Changes to this property will not cause the reactive statement to update -``` - ### script_context_deprecated ``` diff --git a/packages/svelte/messages/client-warnings/warnings.md b/packages/svelte/messages/client-warnings/warnings.md index e9014207fd..d82cdc47a1 100644 --- a/packages/svelte/messages/client-warnings/warnings.md +++ b/packages/svelte/messages/client-warnings/warnings.md @@ -54,6 +54,38 @@ The easiest way to log a value as it changes over time is to use the [`$inspect` > %component% mutated a value owned by %owner%. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead +## reactive_declaration_non_reactive_property + +> A `$:` statement (%location%) read reactive state that was not visible to the compiler. Updates to this state will not cause the statement to re-run. The behaviour of this code will change if you migrate it to runes mode + +In legacy mode, a `$:` [reactive statement](https://svelte.dev/docs/svelte/legacy-reactive-assignments) re-runs when the state it _references_ changes. This is determined at compile time, by analysing the code. + +In runes mode, effects and deriveds re-run when there are changes to the values that are read during the function's _execution_. + +Often, the result is the same — for example these can be considered equivalent: + +```js +$: sum = a + b; +``` + +```js +const sum = $derived(a + b); +``` + +In some cases — such as the one that triggered the above warning — they are _not_ the same: + +```js +const add = () => a + b; + +// the compiler can't 'see' that `sum` depends on `a` and `b`, but +// they _would_ be read while executing the `$derived` version +$: sum = add(); +``` + +Similarly, reactive properties of [deep state](https://svelte.dev/docs/svelte/$state#Deep-state) are not visible to the compiler. As such, changes to these properties will cause effects and deriveds to re-run but will _not_ cause `$:` statements to re-run. + +When you [migrate this component](https://svelte.dev/docs/svelte/v5-migration-guide) to runes mode, the behaviour will change accordingly. + ## state_proxy_equality_mismatch > Reactive `$state(...)` proxies and the values they proxy have different identities. Because of this, comparisons with `%operator%` will produce unexpected results diff --git a/packages/svelte/messages/compile-warnings/script.md b/packages/svelte/messages/compile-warnings/script.md index 293f065ba7..2c891b4fc7 100644 --- a/packages/svelte/messages/compile-warnings/script.md +++ b/packages/svelte/messages/compile-warnings/script.md @@ -26,10 +26,6 @@ > Reassignments of module-level declarations will not cause reactive statements to update -## reactive_declaration_non_reactive_property - -> Properties of objects and arrays are not reactive unless in runes mode. Changes to this property will not cause the reactive statement to update - ## state_referenced_locally > State referenced in its own scope will never update. Did you mean to reference it inside a closure? diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/MemberExpression.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/MemberExpression.js index 1cc20c96da..6ea8f238e1 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/MemberExpression.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/MemberExpression.js @@ -26,30 +26,5 @@ export function MemberExpression(node, context) { context.state.analysis.needs_context = true; } - if (context.state.reactive_statement) { - const left = object(node); - - if (left !== null) { - const binding = context.state.scope.get(left.name); - - if (binding && binding.kind === 'normal') { - const parent = /** @type {Node} */ (context.path.at(-1)); - - if ( - binding.scope === context.state.analysis.module.scope || - binding.declaration_kind === 'import' || - (binding.initial && - binding.initial.type !== 'ArrayExpression' && - binding.initial.type !== 'ObjectExpression' && - binding.scope.function_depth <= 1) - ) { - if (parent.type !== 'MemberExpression' && parent.type !== 'CallExpression') { - w.reactive_declaration_non_reactive_property(node); - } - } - } - } - } - context.next(); } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/LabeledStatement.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/LabeledStatement.js index 87f56262a8..8ca6534457 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/LabeledStatement.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/LabeledStatement.js @@ -1,6 +1,8 @@ +/** @import { Location } from 'locate-character' */ /** @import { Expression, LabeledStatement, Statement } from 'estree' */ /** @import { ReactiveStatement } from '#compiler' */ /** @import { ComponentContext } from '../types' */ +import { dev, is_ignored, locator } from '../../../../state.js'; import * as b from '../../../../utils/builders.js'; import { build_getter } from '../utils.js'; @@ -48,6 +50,11 @@ export function LabeledStatement(node, context) { sequence.push(serialized); } + const location = + dev && !is_ignored(node, 'reactive_declaration_non_reactive_property') + ? locator(/** @type {number} */ (node.start)) + : undefined; + // these statements will be topologically ordered later context.state.legacy_reactive_statements.set( node, @@ -55,7 +62,9 @@ export function LabeledStatement(node, context) { b.call( '$.legacy_pre_effect', sequence.length > 0 ? b.thunk(b.sequence(sequence)) : b.thunk(b.block([])), - b.thunk(b.block(body)) + b.thunk(b.block(body)), + location && b.literal(location.line), + location && b.literal(location.column) ) ) ); diff --git a/packages/svelte/src/compiler/warnings.js b/packages/svelte/src/compiler/warnings.js index bd28956230..f6ed7de7d5 100644 --- a/packages/svelte/src/compiler/warnings.js +++ b/packages/svelte/src/compiler/warnings.js @@ -102,7 +102,6 @@ export const codes = [ "perf_avoid_nested_class", "reactive_declaration_invalid_placement", "reactive_declaration_module_script_dependency", - "reactive_declaration_non_reactive_property", "state_referenced_locally", "store_rune_conflict", "css_unused_selector", @@ -641,14 +640,6 @@ export function reactive_declaration_module_script_dependency(node) { w(node, "reactive_declaration_module_script_dependency", "Reassignments of module-level declarations will not cause reactive statements to update"); } -/** - * Properties of objects and arrays are not reactive unless in runes mode. Changes to this property will not cause the reactive statement to update - * @param {null | NodeLike} node - */ -export function reactive_declaration_non_reactive_property(node) { - w(node, "reactive_declaration_non_reactive_property", "Properties of objects and arrays are not reactive unless in runes mode. Changes to this property will not cause the reactive statement to update"); -} - /** * State referenced in its own scope will never update. Did you mean to reference it inside a closure? * @param {null | NodeLike} node diff --git a/packages/svelte/src/constants.js b/packages/svelte/src/constants.js index 03fddc5ebd..f8a7143b20 100644 --- a/packages/svelte/src/constants.js +++ b/packages/svelte/src/constants.js @@ -44,7 +44,8 @@ export const IGNORABLE_RUNTIME_WARNINGS = /** @type {const} */ ([ 'hydration_attribute_changed', 'hydration_html_changed', 'ownership_invalid_binding', - 'ownership_invalid_mutation' + 'ownership_invalid_mutation', + 'reactive_declaration_non_reactive_property' ]); /** diff --git a/packages/svelte/src/internal/client/dev/location.js b/packages/svelte/src/internal/client/dev/location.js new file mode 100644 index 0000000000..b2e16c371e --- /dev/null +++ b/packages/svelte/src/internal/client/dev/location.js @@ -0,0 +1,25 @@ +import { DEV } from 'esm-env'; +import { FILENAME } from '../../../constants.js'; +import { dev_current_component_function } from '../runtime.js'; + +/** + * + * @param {number} [line] + * @param {number} [column] + */ +export function get_location(line, column) { + if (!DEV || line === undefined) return undefined; + + var filename = dev_current_component_function?.[FILENAME]; + var location = filename && `${filename}:${line}:${column}`; + + return sanitize_location(location); +} + +/** + * Prevent devtools trying to make `location` a clickable link by inserting a zero-width space + * @param {string | undefined} location + */ +export function sanitize_location(location) { + return location?.replace(/\//g, '/\u200b'); +} diff --git a/packages/svelte/src/internal/client/dom/blocks/html.js b/packages/svelte/src/internal/client/dom/blocks/html.js index aa13336b29..2f815b454e 100644 --- a/packages/svelte/src/internal/client/dom/blocks/html.js +++ b/packages/svelte/src/internal/client/dom/blocks/html.js @@ -9,6 +9,7 @@ import { hash } from '../../../../utils.js'; import { DEV } from 'esm-env'; import { dev_current_component_function } from '../../runtime.js'; import { get_first_child, get_next_sibling } from '../operations.js'; +import { sanitize_location } from '../../dev/location.js'; /** * @param {Element} element @@ -28,9 +29,7 @@ function check_hash(element, server_hash, value) { location = `in ${dev_current_component_function[FILENAME]}`; } - w.hydration_html_changed( - location?.replace(/\//g, '/\u200b') // prevent devtools trying to make it a clickable link by inserting a zero-width space - ); + w.hydration_html_changed(sanitize_location(location)); } /** diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js index 69b006fc0f..912aab37b6 100644 --- a/packages/svelte/src/internal/client/reactivity/effects.js +++ b/packages/svelte/src/internal/client/reactivity/effects.js @@ -16,7 +16,8 @@ import { set_is_flushing_effect, set_signal_status, untrack, - skip_reaction + skip_reaction, + capture_signals } from '../runtime.js'; import { DIRTY, @@ -39,10 +40,13 @@ import { } from '../constants.js'; import { set } from './sources.js'; import * as e from '../errors.js'; +import * as w from '../warnings.js'; import { DEV } from 'esm-env'; import { define_property } from '../../shared/utils.js'; import { get_next_sibling } from '../dom/operations.js'; import { destroy_derived } from './deriveds.js'; +import { FILENAME } from '../../../constants.js'; +import { get_location } from '../dev/location.js'; /** * @param {'$effect' | '$effect.pre' | '$inspect'} rune @@ -261,14 +265,21 @@ export function effect(fn) { * Internal representation of `$: ..` * @param {() => any} deps * @param {() => void | (() => void)} fn + * @param {number} [line] + * @param {number} [column] */ -export function legacy_pre_effect(deps, fn) { +export function legacy_pre_effect(deps, fn, line, column) { var context = /** @type {ComponentContextLegacy} */ (component_context); /** @type {{ effect: null | Effect, ran: boolean }} */ var token = { effect: null, ran: false }; context.l.r1.push(token); + if (DEV && line !== undefined) { + var location = get_location(line, column); + var explicit_deps = capture_signals(deps); + } + token.effect = render_effect(() => { deps(); @@ -278,7 +289,18 @@ export function legacy_pre_effect(deps, fn) { token.ran = true; set(context.l.r2, true); - untrack(fn); + + if (DEV && location) { + var implicit_deps = capture_signals(() => untrack(fn)); + + for (var signal of implicit_deps) { + if (!explicit_deps.has(signal)) { + w.reactive_declaration_non_reactive_property(/** @type {string} */ (location)); + } + } + } else { + untrack(fn); + } }); } diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 2779898d6f..a3ec5bca34 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -894,15 +894,17 @@ export function safe_get(signal) { } /** - * Invokes a function and captures all signals that are read during the invocation, - * then invalidates them. - * @param {() => any} fn + * Capture an array of all the signals that are read when `fn` is called + * @template T + * @param {() => T} fn */ -export function invalidate_inner_signals(fn) { +export function capture_signals(fn) { var previous_captured_signals = captured_signals; captured_signals = new Set(); + var captured = captured_signals; var signal; + try { untrack(fn); if (previous_captured_signals !== null) { @@ -913,7 +915,19 @@ export function invalidate_inner_signals(fn) { } finally { captured_signals = previous_captured_signals; } - for (signal of captured) { + + return captured; +} + +/** + * Invokes a function and captures all signals that are read during the invocation, + * then invalidates them. + * @param {() => any} fn + */ +export function invalidate_inner_signals(fn) { + var captured = capture_signals(() => untrack(fn)); + + for (var signal of captured) { // Go one level up because derived signals created as part of props in legacy mode if ((signal.f & LEGACY_DERIVED_PROP) !== 0) { for (const dep of /** @type {Derived} */ (signal).deps || []) { diff --git a/packages/svelte/src/internal/client/warnings.js b/packages/svelte/src/internal/client/warnings.js index 0478313719..5e38db52f0 100644 --- a/packages/svelte/src/internal/client/warnings.js +++ b/packages/svelte/src/internal/client/warnings.js @@ -153,6 +153,19 @@ export function ownership_invalid_mutation(component, owner) { } } +/** + * A `$:` statement (%location%) read reactive state that was not visible to the compiler. Updates to this state will not cause the statement to re-run. The behaviour of this code will change if you migrate it to runes mode + * @param {string} location + */ +export function reactive_declaration_non_reactive_property(location) { + if (DEV) { + console.warn(`%c[svelte] reactive_declaration_non_reactive_property\n%cA \`$:\` statement (${location}) read reactive state that was not visible to the compiler. Updates to this state will not cause the statement to re-run. The behaviour of this code will change if you migrate it to runes mode`, bold, normal); + } else { + // TODO print a link to the documentation + console.warn("reactive_declaration_non_reactive_property"); + } +} + /** * Reactive `$state(...)` proxies and the values they proxy have different identities. Because of this, comparisons with `%operator%` will produce unexpected results * @param {string} operator diff --git a/packages/svelte/tests/runtime-legacy/samples/reactive-statement-non-reactive-import-statement/_config.js b/packages/svelte/tests/runtime-legacy/samples/reactive-statement-non-reactive-import-statement/_config.js new file mode 100644 index 0000000000..7a77424bb2 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/reactive-statement-non-reactive-import-statement/_config.js @@ -0,0 +1,26 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + compileOptions: { + dev: true + }, + + html: '
42
42
', + + test({ assert, target }) { + const [update, reset] = target.querySelectorAll('button'); + flushSync(() => update.click()); + + assert.htmlEqual( + target.innerHTML, + '42
42
' + ); + + flushSync(() => reset.click()); + }, + + warnings: [ + 'A `$:` statement (main.svelte:4:1) read reactive state that was not visible to the compiler. Updates to this state will not cause the statement to re-run. The behaviour of this code will change if you migrate it to runes mode' + ] +}); diff --git a/packages/svelte/tests/runtime-legacy/samples/reactive-statement-non-reactive-import-statement/data.svelte.js b/packages/svelte/tests/runtime-legacy/samples/reactive-statement-non-reactive-import-statement/data.svelte.js new file mode 100644 index 0000000000..70fd0a6abc --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/reactive-statement-non-reactive-import-statement/data.svelte.js @@ -0,0 +1,11 @@ +export const obj = $state({ + prop: 42 +}); + +export function update() { + obj.prop += 1; +} + +export function reset() { + obj.prop = 42; +} diff --git a/packages/svelte/tests/runtime-legacy/samples/reactive-statement-non-reactive-import-statement/main.svelte b/packages/svelte/tests/runtime-legacy/samples/reactive-statement-non-reactive-import-statement/main.svelte new file mode 100644 index 0000000000..2a9b1e1f23 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/reactive-statement-non-reactive-import-statement/main.svelte @@ -0,0 +1,13 @@ + + +{a}
+{b}
+ + diff --git a/packages/svelte/tests/validator/samples/reactive-statement-non-reactive-import-statement/_config.js b/packages/svelte/tests/validator/samples/reactive-statement-non-reactive-import-statement/_config.js deleted file mode 100644 index f47bee71df..0000000000 --- a/packages/svelte/tests/validator/samples/reactive-statement-non-reactive-import-statement/_config.js +++ /dev/null @@ -1,3 +0,0 @@ -import { test } from '../../test'; - -export default test({}); diff --git a/packages/svelte/tests/validator/samples/reactive-statement-non-reactive-import-statement/input.svelte b/packages/svelte/tests/validator/samples/reactive-statement-non-reactive-import-statement/input.svelte deleted file mode 100644 index 2582974b71..0000000000 --- a/packages/svelte/tests/validator/samples/reactive-statement-non-reactive-import-statement/input.svelte +++ /dev/null @@ -1,8 +0,0 @@ - - -{prop}
diff --git a/packages/svelte/tests/validator/samples/reactive-statement-non-reactive-import-statement/warnings.json b/packages/svelte/tests/validator/samples/reactive-statement-non-reactive-import-statement/warnings.json deleted file mode 100644 index 4184902753..0000000000 --- a/packages/svelte/tests/validator/samples/reactive-statement-non-reactive-import-statement/warnings.json +++ /dev/null @@ -1,14 +0,0 @@ -[ - { - "code": "reactive_declaration_non_reactive_property", - "message": "Properties of objects and arrays are not reactive unless in runes mode. Changes to this property will not cause the reactive statement to update", - "start": { - "line": 4, - "column": 11 - }, - "end": { - "line": 4, - "column": 19 - } - } -]