diff --git a/.changeset/modern-apricots-promise.md b/.changeset/modern-apricots-promise.md new file mode 100644 index 0000000000..5efd5e8e2a --- /dev/null +++ b/.changeset/modern-apricots-promise.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: prevent buggy ownership warning when reassigning state diff --git a/packages/svelte/src/compiler/phases/3-transform/client/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/utils.js index 29af706aa1..2c343480af 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js @@ -203,7 +203,7 @@ export function serialize_set_binding(node, context, fallback, options) { assignment.right = private_state.kind === 'frozen_state' ? b.call('$.freeze', value) - : b.call('$.proxy', value); + : serialize_proxy_reassignment(value, private_state.id, state); return assignment; } } @@ -216,7 +216,7 @@ export function serialize_set_binding(node, context, fallback, options) { should_proxy_or_freeze(value, context.state.scope) ? private_state.kind === 'frozen_state' ? b.call('$.freeze', value) - : b.call('$.proxy', value) + : serialize_proxy_reassignment(value, private_state.id, state) : value ); } @@ -240,7 +240,7 @@ export function serialize_set_binding(node, context, fallback, options) { assignment.right = public_state.kind === 'frozen_state' ? b.call('$.freeze', value) - : b.call('$.proxy', value); + : serialize_proxy_reassignment(value, public_state.id, state); return assignment; } } @@ -305,7 +305,7 @@ export function serialize_set_binding(node, context, fallback, options) { context.state.analysis.runes && !options?.skip_proxy_and_freeze && should_proxy_or_freeze(value, context.state.scope) - ? b.call('$.proxy', value) + ? serialize_proxy_reassignment(value, left_name, state) : value ); } else if (binding.kind === 'frozen_state') { @@ -410,6 +410,25 @@ export function serialize_set_binding(node, context, fallback, options) { return serialize(); } +/** + * @param {import('estree').Expression} value + * @param {import('estree').PrivateIdentifier | string} proxy_reference + * @param {import('./types').ClientTransformState} state + */ +export function serialize_proxy_reassignment(value, proxy_reference, state) { + return state.options.dev + ? b.call( + '$.proxy', + value, + b.true, + b.null, + typeof proxy_reference === 'string' + ? b.id(proxy_reference) + : b.member(b.this, proxy_reference) + ) + : b.call('$.proxy', value); +} + /** * @param {import('estree').ArrowFunctionExpression | import('estree').FunctionExpression} node * @param {import('./types').ComponentContext} context diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js index 71bb8c6588..e08bb6c8ac 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js @@ -2,7 +2,12 @@ import { get_rune } from '../../../scope.js'; import { is_hoistable_function, transform_inspect_rune } from '../../utils.js'; import * as b from '../../../../utils/builders.js'; import * as assert from '../../../../utils/assert.js'; -import { get_prop_source, is_state_source, should_proxy_or_freeze } from '../utils.js'; +import { + get_prop_source, + is_state_source, + serialize_proxy_reassignment, + should_proxy_or_freeze +} from '../utils.js'; import { extract_paths } from '../../../../utils/ast.js'; import { regex_invalid_identifier_chars } from '../../../patterns.js'; @@ -139,7 +144,11 @@ export const javascript_visitors_runes = { 'set', definition.key, [value], - [b.stmt(b.call('$.set', member, b.call('$.proxy', value)))] + [ + b.stmt( + b.call('$.set', member, serialize_proxy_reassignment(value, field.id, state)) + ) + ] ) ); } diff --git a/packages/svelte/src/compiler/utils/builders.js b/packages/svelte/src/compiler/utils/builders.js index ec85e41a41..6070a96702 100644 --- a/packages/svelte/src/compiler/utils/builders.js +++ b/packages/svelte/src/compiler/utils/builders.js @@ -469,6 +469,7 @@ export function do_while(test, body) { const true_instance = literal(true); const false_instance = literal(false); +const null_instane = literal(null); /** @type {import('estree').DebuggerStatement} */ const debugger_builder = { @@ -630,6 +631,7 @@ export { return_builder as return, if_builder as if, this_instance as this, + null_instane as null, debugger_builder as debugger }; diff --git a/packages/svelte/src/internal/client/proxy.js b/packages/svelte/src/internal/client/proxy.js index 33e5edbe51..31a22600b6 100644 --- a/packages/svelte/src/internal/client/proxy.js +++ b/packages/svelte/src/internal/client/proxy.js @@ -27,9 +27,10 @@ import * as e from './errors.js'; * @param {T} value * @param {boolean} [immutable] * @param {import('#client').ProxyMetadata | null} [parent] + * @param {import('#client').Source} [prev] dev mode only * @returns {import('#client').ProxyStateObject | T} */ -export function proxy(value, immutable = true, parent = null) { +export function proxy(value, immutable = true, parent = null, prev) { if (typeof value === 'object' && value != null && !is_frozen(value)) { // If we have an existing proxy, return it... if (STATE_SYMBOL in value) { @@ -71,13 +72,22 @@ export function proxy(value, immutable = true, parent = null) { // @ts-expect-error value[STATE_SYMBOL].parent = parent; - // @ts-expect-error - value[STATE_SYMBOL].owners = - parent === null - ? current_component_context !== null - ? new Set([current_component_context.function]) - : null - : new Set(); + if (prev) { + // Reuse owners from previous state; necessary because reassignment is not guaranteed to have correct component context. + // If no previous proxy exists we play it safe and assume ownerless state + // @ts-expect-error + const prev_owners = prev?.v?.[STATE_SYMBOL]?.owners; + // @ts-expect-error + value[STATE_SYMBOL].owners = prev_owners ? new Set(prev_owners) : null; + } else { + // @ts-expect-error + value[STATE_SYMBOL].owners = + parent === null + ? current_component_context !== null + ? new Set([current_component_context.function]) + : null + : new Set(); + } } return proxy; diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/Counter.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/Counter.svelte index 57cbebde12..4af73f0dad 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/Counter.svelte +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/Counter.svelte @@ -1,8 +1,8 @@ + diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js index 97f088fb4d..62c6961242 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/_config.js @@ -1,36 +1,30 @@ import { flushSync } from 'svelte'; import { test } from '../../test'; -/** @type {typeof console.trace} */ -let trace; - export default test({ - html: ``, + html: ` `, compileOptions: { dev: true }, - before_test: () => { - trace = console.trace; - console.trace = () => {}; - }, - - after_test: () => { - console.trace = trace; - }, - test({ assert, target, warnings }) { - const btn = target.querySelector('button'); - - flushSync(() => { - btn?.click(); - }); - - assert.htmlEqual(target.innerHTML, ``); - - assert.deepEqual(warnings, [ - 'Counter.svelte mutated a value owned by main.svelte. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead' - ]); + const warning = + 'Counter.svelte mutated a value owned by main.svelte. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead'; + const [btn1, btn2] = target.querySelectorAll('button'); + + btn1.click(); + flushSync(); + assert.htmlEqual(target.innerHTML, ` `); + assert.deepEqual(warnings, [warning]); + + btn2.click(); + flushSync(); + assert.htmlEqual(target.innerHTML, ` `); + + btn1.click(); + flushSync(); + assert.htmlEqual(target.innerHTML, ` `); + assert.deepEqual(warnings, [warning, warning]); } }); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/main.svelte index e9617af17d..45c13c5d7a 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-discouraged/main.svelte @@ -1,7 +1,7 @@ - + object = {count: 0}} /> diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/_config.js new file mode 100644 index 0000000000..b4864154c3 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/_config.js @@ -0,0 +1,11 @@ +import { test } from '../../test'; + +export default test({ + compileOptions: { + dev: true + }, + + async test({ assert, warnings }) { + assert.deepEqual(warnings, []); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/child.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/child.svelte new file mode 100644 index 0000000000..5efb0d11ab --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/child.svelte @@ -0,0 +1,18 @@ + diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/main.svelte new file mode 100644 index 0000000000..8a6922e9e2 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-global-2/main.svelte @@ -0,0 +1,5 @@ +