Skip to content

Commit

Permalink
fix: ensure deep mutation ownership widening
Browse files Browse the repository at this point in the history
Previously, ownership widening/removal was only done if the immediate object that was encountered was state. This isn't always the case. It also didn't take into account classes with state on it (which turn into getters). This change takes both these cases into account and now always traverses the given object deeply.
fixes #11060
  • Loading branch information
dummdidumm committed Apr 8, 2024
1 parent b1a8038 commit d849558
Show file tree
Hide file tree
Showing 5 changed files with 148 additions and 13 deletions.
5 changes: 5 additions & 0 deletions .changeset/mighty-frogs-obey.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"svelte": patch
---

fix: ensure deep mutation ownership widening
85 changes: 72 additions & 13 deletions packages/svelte/src/internal/client/dev/ownership.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import { STATE_SYMBOL } from '../constants.js';
import { untrack } from '../runtime.js';
import { get_descriptors } from '../utils.js';

/** @type {Record<string, Array<{ start: Location, end: Location, component: Function }>>} */
const boundaries = {};
Expand Down Expand Up @@ -91,49 +92,107 @@ export function mark_module_end() {
}
}

let add_owner_visited = new Set();

/**
*
* @param {any} object
* @param {any} owner
*/
export function add_owner(object, owner) {
untrack(() => {
add_owner_to_object(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;
}
}

/**
* @param {any} object
* @param {Function} owner
* @param {Set<any>} visited
*/
function add_owner_to_object(object, owner) {
function add_owner_to_object(object, owner, visited) {
if (visited.has(object)) return;
visited.add(object);

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) {
untrack(() => {
strip_owner_from_object(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;
}
}

/**
* @param {any} object
* @param {Set<any>} visited
*/
function strip_owner_from_object(object) {
function strip_owner_from_object(object, visited) {
if (visited.has(object)) return;
visited.add(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) {
strip_owner(object[key]);
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
}
}
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<script lang="ts">
import { setContext } from 'svelte';
import Sub from './sub.svelte';
class Person1 {
value = $state({ person: 'John', age: 33 })
}
const class_nested_state = $state(new Person1());
class Person2 {
person = $state('John');
age = $state(33);
}
const state_nested_class = $state({ value: new Person2() });
const nested_state = $state({ person: 'John', age: 33 });
setContext('foo', {
nested_state,
get class_nested_state() { return class_nested_state },
get state_nested_class() { return state_nested_class }
})
</script>

<Sub {class_nested_state} {state_nested_class} {nested_state} />
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<script>
import { getContext } from "svelte";
const foo = getContext('foo')
</script>

<button onclick={() => {
foo.class_nested_state.value.age++;
foo.state_nested_class.value.age++;
foo.nested_state.age++;
}}>mutate</button>

0 comments on commit d849558

Please sign in to comment.