From f0bb9c58d650e480472683089907b1ee02057f77 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Fri, 31 Jan 2025 16:08:10 +0100 Subject: [PATCH 1/2] fix: recurse into `$derived` for ownership validation - `$derived` can contain `$state` declarations so we cannot ignore them, so this reverts #14533 - instead, we add equality checks to not do this expensive work unnecessarily - this also adds a render effect similar to the class ownership addition when it detects a getter on a POJO during ownership addition fixes #15164 --- .changeset/thick-carrots-arrive.md | 5 ++ .../3-transform/client/visitors/ClassBody.js | 29 ++++--- .../client/visitors/shared/component.js | 41 +++------- .../src/internal/client/dev/ownership.js | 42 ++++++++-- packages/svelte/src/internal/client/index.js | 1 + .../CounterBinding.svelte | 7 ++ .../CounterContext.svelte | 13 +++ .../_config.js | 34 ++++++++ .../main.svelte | 82 +++++++++++++++++++ 9 files changed, 204 insertions(+), 50 deletions(-) create mode 100644 .changeset/thick-carrots-arrive.md create mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterBinding.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterContext.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/main.svelte diff --git a/.changeset/thick-carrots-arrive.md b/.changeset/thick-carrots-arrive.md new file mode 100644 index 000000000000..582cf5e6e15b --- /dev/null +++ b/.changeset/thick-carrots-arrive.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: recurse into `$derived` for ownership validation diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/ClassBody.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/ClassBody.js index 7b3a9a4d0e29..ed800e5226ce 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/ClassBody.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/ClassBody.js @@ -190,22 +190,21 @@ export function ClassBody(node, context) { 'method', b.id('$.ADD_OWNER'), [b.id('owner')], - Array.from(public_state) - // Only run ownership addition on $state fields. - // Theoretically someone could create a `$state` while creating `$state.raw` or inside a `$derived.by`, - // but that feels so much of an edge case that it doesn't warrant a perf hit for the common case. - .filter(([_, { kind }]) => kind === 'state') - .map(([name]) => - b.stmt( - b.call( - '$.add_owner', - b.call('$.get', b.member(b.this, b.private_id(name))), - b.id('owner'), - b.literal(false), - is_ignored(node, 'ownership_invalid_binding') && b.true - ) + [ + b.stmt( + b.call( + '$.add_owner_to_class', + b.this, + b.id('owner'), + b.array( + Array.from(public_state).map(([name]) => + b.thunk(b.call('$.get', b.member(b.this, b.private_id(name)))) + ) + ), + is_ignored(node, 'ownership_invalid_binding') && b.true ) - ), + ) + ], true ) ); diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js index 15e4f68e9e49..2bae4486dc58 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js @@ -180,37 +180,18 @@ export function build_component(node, component_name, context, anchor = context. const expression = /** @type {Expression} */ (context.visit(attribute.expression)); if (dev && attribute.name !== 'this') { - let should_add_owner = true; - - if (attribute.expression.type !== 'SequenceExpression') { - const left = object(attribute.expression); - - if (left?.type === 'Identifier') { - const binding = context.state.scope.get(left.name); - - // Only run ownership addition on $state fields. - // Theoretically someone could create a `$state` while creating `$state.raw` or inside a `$derived.by`, - // but that feels so much of an edge case that it doesn't warrant a perf hit for the common case. - if (binding?.kind === 'derived' || binding?.kind === 'raw_state') { - should_add_owner = false; - } - } - } - - if (should_add_owner) { - binding_initializers.push( - b.stmt( - b.call( - b.id('$.add_owner_effect'), - expression.type === 'SequenceExpression' - ? expression.expressions[0] - : b.thunk(expression), - b.id(component_name), - is_ignored(node, 'ownership_invalid_binding') && b.true - ) + binding_initializers.push( + b.stmt( + b.call( + b.id('$.add_owner_effect'), + expression.type === 'SequenceExpression' + ? expression.expressions[0] + : b.thunk(expression), + b.id(component_name), + is_ignored(node, 'ownership_invalid_binding') && b.true ) - ); - } + ) + ); } if (expression.type === 'SequenceExpression') { diff --git a/packages/svelte/src/internal/client/dev/ownership.js b/packages/svelte/src/internal/client/dev/ownership.js index 2a2527803af9..62119b36dbd6 100644 --- a/packages/svelte/src/internal/client/dev/ownership.js +++ b/packages/svelte/src/internal/client/dev/ownership.js @@ -6,7 +6,7 @@ import { render_effect, user_pre_effect } from '../reactivity/effects.js'; import { dev_current_component_function } from '../context.js'; import { get_prototype_of } from '../../shared/utils.js'; import * as w from '../warnings.js'; -import { FILENAME } from '../../../constants.js'; +import { FILENAME, UNINITIALIZED } from '../../../constants.js'; /** @type {Record>} */ const boundaries = {}; @@ -140,6 +140,25 @@ export function add_owner_effect(get_object, Component, skip_warning = false) { }); } +/** + * @param {any} _this + * @param {Function} owner + * @param {Array<() => any>} getters + * @param {boolean} skip_warning + */ +export function add_owner_to_class(_this, owner, getters, skip_warning) { + _this[ADD_OWNER].current ||= getters.map(() => UNINITIALIZED); + + for (let i = 0; i < getters.length; i += 1) { + const current = getters[i](); + // For performance reasons we only re-add the owner if the state has changed + if (current !== _this[ADD_OWNER][i]) { + _this[ADD_OWNER].current[i] = current; + add_owner(current, owner, false, skip_warning); + } + } +} + /** * @param {ProxyMetadata | null} from * @param {ProxyMetadata} to @@ -196,7 +215,19 @@ function add_owner_to_object(object, owner, seen) { if (proto === Object.prototype) { // recurse until we find a state proxy for (const key in object) { - add_owner_to_object(object[key], owner, seen); + if (Object.getOwnPropertyDescriptor(object, key)?.get) { + // Similar to the class case; the getter could update with a new state + let current = UNINITIALIZED; + render_effect(() => { + const next = object[key]; + if (current !== next) { + current = next; + add_owner_to_object(next, owner, seen); + } + }); + } else { + add_owner_to_object(object[key], owner, seen); + } } } else if (proto === Array.prototype) { // recurse until we find a state proxy @@ -221,9 +252,10 @@ function has_owner(metadata, component) { return ( metadata.owners.has(component) || // This helps avoid false positives when using HMR, where the component function is replaced - [...metadata.owners].some( - (owner) => /** @type {any} */ (owner)[FILENAME] === /** @type {any} */ (component)?.[FILENAME] - ) || + (FILENAME in component && + [...metadata.owners].some( + (owner) => /** @type {any} */ (owner)[FILENAME] === component[FILENAME] + )) || (metadata.parent !== null && has_owner(metadata.parent, component)) ); } diff --git a/packages/svelte/src/internal/client/index.js b/packages/svelte/src/internal/client/index.js index 3f3b29b029dd..4c45ee4a8c5d 100644 --- a/packages/svelte/src/internal/client/index.js +++ b/packages/svelte/src/internal/client/index.js @@ -10,6 +10,7 @@ export { mark_module_start, mark_module_end, add_owner_effect, + add_owner_to_class, skip_ownership_validation } from './dev/ownership.js'; export { check_target, legacy_api } from './dev/legacy.js'; diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterBinding.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterBinding.svelte new file mode 100644 index 000000000000..d6da559fb176 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterBinding.svelte @@ -0,0 +1,7 @@ + + +

Binding

+ + diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterContext.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterContext.svelte new file mode 100644 index 000000000000..b935f0a472dc --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/CounterContext.svelte @@ -0,0 +1,13 @@ + + +

Context

+ + diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/_config.js new file mode 100644 index 000000000000..d6d12d01cd09 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/_config.js @@ -0,0 +1,34 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +// Tests that ownership is widened with $derived (on class or on its own) that contains $state +export default test({ + compileOptions: { + dev: true + }, + + test({ assert, target, warnings }) { + const [root, counter_context1, counter_context2, counter_binding1, counter_binding2] = + target.querySelectorAll('button'); + + counter_context1.click(); + counter_context2.click(); + counter_binding1.click(); + counter_binding2.click(); + flushSync(); + + assert.equal(warnings.length, 0); + + root.click(); + flushSync(); + counter_context1.click(); + counter_context2.click(); + counter_binding1.click(); + counter_binding2.click(); + flushSync(); + + assert.equal(warnings.length, 0); + }, + + warnings: [] +}); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/main.svelte new file mode 100644 index 000000000000..ae2fe3cb3dc7 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/main.svelte @@ -0,0 +1,82 @@ + + +

Parent

+ + + + + + From 67dc57935d9ac734844ee5a9d73b327585141da8 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Mon, 3 Feb 2025 19:54:17 +0100 Subject: [PATCH 2/2] Update main.svelte --- .../main.svelte | 36 ------------------- 1 file changed, 36 deletions(-) diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/main.svelte index ae2fe3cb3dc7..aaade26e162c 100644 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-with-binding-8/main.svelte @@ -44,39 +44,3 @@ - -