From 7810d1fdc7bb3c96c117f2c8306937542e9427be Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 11 Apr 2024 12:17:57 -0400 Subject: [PATCH 1/3] Revert "fix: ensure deep mutation ownership widening (#11094)" This reverts commit 8578857332c27005866661a22734b1bf2d83b69f. --- .changeset/mighty-frogs-obey.md | 5 -- .../src/internal/client/dev/ownership.js | 85 +++---------------- .../_config.js | 35 -------- .../main.svelte | 25 ------ .../sub.svelte | 11 --- 5 files changed, 13 insertions(+), 148 deletions(-) delete mode 100644 .changeset/mighty-frogs-obey.md delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte delete mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/sub.svelte diff --git a/.changeset/mighty-frogs-obey.md b/.changeset/mighty-frogs-obey.md deleted file mode 100644 index 0321c60463ac..000000000000 --- a/.changeset/mighty-frogs-obey.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"svelte": patch ---- - -fix: ensure deep mutation ownership widening diff --git a/packages/svelte/src/internal/client/dev/ownership.js b/packages/svelte/src/internal/client/dev/ownership.js index fde316c8401a..358b56df224b 100644 --- a/packages/svelte/src/internal/client/dev/ownership.js +++ b/packages/svelte/src/internal/client/dev/ownership.js @@ -2,7 +2,6 @@ import { STATE_SYMBOL } from '../constants.js'; import { untrack } from '../runtime.js'; -import { get_descriptors } from '../utils.js'; /** @type {Record>} */ const boundaries = {}; @@ -92,107 +91,49 @@ export function mark_module_end() { } } -let add_owner_visited = new Set(); - /** * * @param {any} object * @param {any} owner */ export function add_owner(object, owner) { - // Needed because ownership addition can invoke getters on a proxy, - // calling add_owner anew, so just keeping the set as part of - // add_owner_to_object would not be enough. - const prev = add_owner_visited; - try { - add_owner_visited = new Set(add_owner_visited); - untrack(() => { - add_owner_to_object(object, owner, add_owner_visited); - }); - } finally { - add_owner_visited = prev; - } + untrack(() => { + add_owner_to_object(object, owner); + }); } /** * @param {any} object * @param {Function} owner - * @param {Set} visited */ -function add_owner_to_object(object, owner, visited) { - if (visited.has(object)) return; - visited.add(object); - +function add_owner_to_object(object, owner) { if (object?.[STATE_SYMBOL]?.o && !object[STATE_SYMBOL].o.has(owner)) { object[STATE_SYMBOL].o.add(owner); + + for (const key in object) { + add_owner_to_object(object[key], owner); + } } - // Not inside previous if-block; there could be normal objects in-between - traverse_for_owners(object, (nested) => add_owner_to_object(nested, owner, visited)); } -let strip_owner_visited = new Set(); - /** * @param {any} object */ export function strip_owner(object) { - // Needed because ownership stripping can invoke getters on a proxy, - // calling strip_owner anew, so just keeping the set as part of - // strip_owner_from_object would not be enough. - const prev = strip_owner_visited; - try { - untrack(() => { - strip_owner_from_object(object, strip_owner_visited); - }); - } finally { - strip_owner_visited = prev; - } + untrack(() => { + strip_owner_from_object(object); + }); } /** * @param {any} object - * @param {Set} visited */ -function strip_owner_from_object(object, visited) { - if (visited.has(object)) return; - visited.add(object); - +function strip_owner_from_object(object) { if (object?.[STATE_SYMBOL]?.o) { object[STATE_SYMBOL].o = null; - } - // Not inside previous if-block; there could be normal objects in-between - traverse_for_owners(object, (nested) => strip_owner_from_object(nested, visited)); -} -/** - * @param {any} object - * @param {(obj: any) => void} cb - */ -function traverse_for_owners(object, cb) { - if (typeof object === 'object' && object !== null && !(object instanceof EventTarget)) { for (const key in object) { - cb(object[key]); - } - // deal with state on classes - const proto = Object.getPrototypeOf(object); - if ( - proto !== Object.prototype && - proto !== Array.prototype && - proto !== Map.prototype && - proto !== Set.prototype && - proto !== Date.prototype - ) { - const descriptors = get_descriptors(proto); - for (let key in descriptors) { - const get = descriptors[key].get; - if (get) { - try { - cb(object[key]); - } catch (e) { - // continue - } - } - } + strip_owner(object[key]); } } } diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js deleted file mode 100644 index e2b31a2e64fb..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js +++ /dev/null @@ -1,35 +0,0 @@ -import { tick } from 'svelte'; -import { test } from '../../test'; - -/** @type {typeof console.warn} */ -let warn; - -/** @type {any[]} */ -let warnings = []; - -export default test({ - compileOptions: { - dev: true - }, - - before_test: () => { - warn = console.warn; - - console.warn = (...args) => { - warnings.push(...args); - }; - }, - - after_test: () => { - console.warn = warn; - warnings = []; - }, - - async test({ assert, target }) { - const btn = target.querySelector('button'); - - await btn?.click(); - await tick(); - assert.deepEqual(warnings.length, 0); - } -}); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte deleted file mode 100644 index ffe9735e21f9..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte +++ /dev/null @@ -1,25 +0,0 @@ - - - diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/sub.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/sub.svelte deleted file mode 100644 index 0f60a4584576..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/sub.svelte +++ /dev/null @@ -1,11 +0,0 @@ - - - From 25d744cb020363e7c593cba0ef9d6444100f57b0 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 11 Apr 2024 12:18:22 -0400 Subject: [PATCH 2/3] reinstate tests --- .../_config.js | 35 +++++++++++++++++++ .../main.svelte | 25 +++++++++++++ .../sub.svelte | 11 ++++++ 3 files changed, 71 insertions(+) create mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/sub.svelte diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js new file mode 100644 index 000000000000..e2b31a2e64fb --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/_config.js @@ -0,0 +1,35 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +/** @type {typeof console.warn} */ +let warn; + +/** @type {any[]} */ +let warnings = []; + +export default test({ + compileOptions: { + dev: true + }, + + before_test: () => { + warn = console.warn; + + console.warn = (...args) => { + warnings.push(...args); + }; + }, + + after_test: () => { + console.warn = warn; + warnings = []; + }, + + async test({ assert, target }) { + const btn = target.querySelector('button'); + + await btn?.click(); + await tick(); + assert.deepEqual(warnings.length, 0); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte new file mode 100644 index 000000000000..ffe9735e21f9 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/main.svelte @@ -0,0 +1,25 @@ + + + diff --git a/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/sub.svelte b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/sub.svelte new file mode 100644 index 000000000000..0f60a4584576 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/non-local-mutation-inherited-owner-6/sub.svelte @@ -0,0 +1,11 @@ + + + From 04f57a35e0720d2108082bece227a186bb9d1062 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 11 Apr 2024 14:20:54 -0400 Subject: [PATCH 3/3] WIP --- .../src/internal/client/dev/ownership.js | 52 ++++++++++++++++--- .../svelte/src/internal/client/runtime.js | 6 ++- packages/svelte/src/internal/client/utils.js | 12 +++++ 3 files changed, 62 insertions(+), 8 deletions(-) diff --git a/packages/svelte/src/internal/client/dev/ownership.js b/packages/svelte/src/internal/client/dev/ownership.js index 358b56df224b..03483d5419ec 100644 --- a/packages/svelte/src/internal/client/dev/ownership.js +++ b/packages/svelte/src/internal/client/dev/ownership.js @@ -1,7 +1,9 @@ /** @typedef {{ file: string, line: number, column: number }} Location */ import { STATE_SYMBOL } from '../constants.js'; +import { unstate } from '../proxy.js'; import { untrack } from '../runtime.js'; +import { get_descriptors } from '../utils.js'; /** @type {Record>} */ const boundaries = {}; @@ -98,20 +100,56 @@ export function mark_module_end() { */ export function add_owner(object, owner) { untrack(() => { - add_owner_to_object(object, owner); + var previous_owner = current_owner; + current_owner = owner; + add_owner_to_object(object); + current_owner = previous_owner; }); } +/** @type {any} */ +export var current_owner = null; + /** * @param {any} object - * @param {Function} owner + * @param {Set} [seen] */ -function add_owner_to_object(object, owner) { - if (object?.[STATE_SYMBOL]?.o && !object[STATE_SYMBOL].o.has(owner)) { - object[STATE_SYMBOL].o.add(owner); +function add_owner_to_object(object, seen = new Set()) { + if (seen.has(object) || typeof object !== 'object' || object === null) { + return; + } - for (const key in object) { - add_owner_to_object(object[key], owner); + if (object?.[STATE_SYMBOL]?.o) { + object[STATE_SYMBOL].o.add(current_owner); + } else { + seen.add(object); + + for (let key in object) { + try { + add_owner_to_object(object[key], seen); + } catch (e) { + // continue + } + } + const proto = Object.getPrototypeOf(object); + if ( + proto !== Object.prototype && + proto !== Array.prototype && + proto !== Map.prototype && + proto !== Set.prototype && + proto !== Date.prototype + ) { + const descriptors = get_descriptors(proto); + for (let key in descriptors) { + const get = descriptors[key].get; + if (get) { + try { + get.call(object); + } catch (e) { + // continue + } + } + } } } } diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index c1b8ee3e441a..4c172953301e 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -25,7 +25,7 @@ import { ROOT_EFFECT } from './constants.js'; import { flush_tasks } from './dom/task.js'; -import { add_owner } from './dev/ownership.js'; +import { add_owner, current_owner } from './dev/ownership.js'; import { mutate, set, source } from './reactivity/sources.js'; import { update_derived } from './reactivity/deriveds.js'; @@ -758,6 +758,10 @@ export function get(signal) { } } + if (current_owner !== null && signal.v?.[STATE_SYMBOL]?.o) { + signal.v[STATE_SYMBOL].o.add(current_owner); + } + return signal.v; } diff --git a/packages/svelte/src/internal/client/utils.js b/packages/svelte/src/internal/client/utils.js index 6c6b9cc5461c..f2df27832b9a 100644 --- a/packages/svelte/src/internal/client/utils.js +++ b/packages/svelte/src/internal/client/utils.js @@ -47,3 +47,15 @@ export function map_get(map, key) { export function is_function(thing) { return typeof thing === 'function'; } + +/** @param {any} object */ +export function is_exotic_object(object) { + const proto = Object.getPrototypeOf(object); + return ( + proto !== Object.prototype && + proto !== Array.prototype && + proto !== Map.prototype && + proto !== Set.prototype && + proto !== Date.prototype + ); +}