From 546a82922c948ac8eef9c1455a821426782db052 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 18 Nov 2024 14:10:12 +0000 Subject: [PATCH 1/5] fix: ensure internal cloning can work circular values --- .changeset/cool-trains-yawn.md | 5 +++++ packages/svelte/src/internal/shared/clone.js | 17 ++++++++++++++--- .../samples/inspect-recursive-2/_config.js | 15 +++++++++++++++ .../samples/inspect-recursive-2/main.svelte | 11 +++++++++++ 4 files changed, 45 insertions(+), 3 deletions(-) create mode 100644 .changeset/cool-trains-yawn.md create mode 100644 packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/main.svelte diff --git a/.changeset/cool-trains-yawn.md b/.changeset/cool-trains-yawn.md new file mode 100644 index 000000000000..d5198d118c52 --- /dev/null +++ b/.changeset/cool-trains-yawn.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: ensure internal cloning can work circular values diff --git a/packages/svelte/src/internal/shared/clone.js b/packages/svelte/src/internal/shared/clone.js index bfdc9af26390..40a9f0724c86 100644 --- a/packages/svelte/src/internal/shared/clone.js +++ b/packages/svelte/src/internal/shared/clone.js @@ -64,7 +64,11 @@ function clone(value, cloned, path, paths) { cloned.set(value, copy); for (let i = 0; i < value.length; i += 1) { - copy.push(clone(value[i], cloned, DEV ? `${path}[${i}]` : path, paths)); + var element = value[i]; + if (cloned.get(element) === null) { + cloned.set(element, copy); + } + copy.push(clone(element, cloned, DEV ? `${path}[${i}]` : path, paths)); } return copy; @@ -76,8 +80,11 @@ function clone(value, cloned, path, paths) { cloned.set(value, copy); for (var key in value) { - // @ts-expect-error - copy[key] = clone(value[key], cloned, DEV ? `${path}.${key}` : path, paths); + var prop = /** @type {any} */ (value[key]); + if (cloned.get(prop) === null) { + cloned.set(prop, copy); + } + copy[key] = clone(prop, cloned, DEV ? `${path}.${key}` : path, paths); } return copy; @@ -88,6 +95,10 @@ function clone(value, cloned, path, paths) { } if (typeof (/** @type {T & { toJSON?: any } } */ (value).toJSON) === 'function') { + // To avoid cycles set the clone to null, so if we encounter it again later we can + // slot in the currently copied object instead + // @ts-ignore + cloned.set(value, null); return clone( /** @type {T & { toJSON(): any } } */ (value).toJSON(), cloned, diff --git a/packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/_config.js b/packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/_config.js new file mode 100644 index 000000000000..bfa47ecb3fff --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/_config.js @@ -0,0 +1,15 @@ +import { test } from '../../test'; + +export default test({ + compileOptions: { + dev: true + }, + + async test({ assert, logs }) { + var a = { + a: null + }; + a.a = a; + assert.deepEqual(logs, ['init', { a }]); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/main.svelte b/packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/main.svelte new file mode 100644 index 000000000000..cd7eb8afdd0b --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/main.svelte @@ -0,0 +1,11 @@ + From 1d9ebd5bdf4d5b6d499e350ec3248a60073af62e Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 18 Nov 2024 14:23:55 +0000 Subject: [PATCH 2/5] better fixc --- packages/svelte/src/internal/shared/clone.js | 33 +++++++++---------- .../samples/inspect-recursive-2/_config.js | 12 +++++-- .../samples/inspect-recursive-2/main.svelte | 12 +++++++ 3 files changed, 38 insertions(+), 19 deletions(-) diff --git a/packages/svelte/src/internal/shared/clone.js b/packages/svelte/src/internal/shared/clone.js index 40a9f0724c86..87d5968ee0ab 100644 --- a/packages/svelte/src/internal/shared/clone.js +++ b/packages/svelte/src/internal/shared/clone.js @@ -22,7 +22,7 @@ export function snapshot(value, skip_warning = false) { /** @type {string[]} */ const paths = []; - const copy = clone(value, new Map(), '', paths); + const copy = clone(value, new Map(), null, '', paths); if (paths.length === 1 && paths[0] === '') { // value could not be cloned w.state_snapshot_uncloneable(); @@ -40,18 +40,19 @@ export function snapshot(value, skip_warning = false) { return copy; } - return clone(value, new Map(), '', empty); + return clone(value, new Map(), null, '', empty); } /** * @template T * @param {T} value * @param {Map>} cloned + * @param {null | T} json_instance * @param {string} path * @param {string[]} paths * @returns {Snapshot} */ -function clone(value, cloned, path, paths) { +function clone(value, cloned, json_instance, path, paths) { if (typeof value === 'object' && value !== null) { const unwrapped = cloned.get(value); if (unwrapped !== undefined) return unwrapped; @@ -62,13 +63,12 @@ function clone(value, cloned, path, paths) { if (is_array(value)) { const copy = /** @type {Snapshot} */ ([]); cloned.set(value, copy); + if (json_instance !== null) { + cloned.set(json_instance, copy); + } for (let i = 0; i < value.length; i += 1) { - var element = value[i]; - if (cloned.get(element) === null) { - cloned.set(element, copy); - } - copy.push(clone(element, cloned, DEV ? `${path}[${i}]` : path, paths)); + copy.push(clone(value[i], cloned, null, DEV ? `${path}[${i}]` : path, paths)); } return copy; @@ -78,13 +78,13 @@ function clone(value, cloned, path, paths) { /** @type {Snapshot} */ const copy = {}; cloned.set(value, copy); + if (json_instance !== null) { + cloned.set(json_instance, copy); + } for (var key in value) { - var prop = /** @type {any} */ (value[key]); - if (cloned.get(prop) === null) { - cloned.set(prop, copy); - } - copy[key] = clone(prop, cloned, DEV ? `${path}.${key}` : path, paths); + // @ts-ignore + copy[key] = clone(value[key], cloned, null, DEV ? `${path}.${key}` : path, paths); } return copy; @@ -95,13 +95,12 @@ function clone(value, cloned, path, paths) { } if (typeof (/** @type {T & { toJSON?: any } } */ (value).toJSON) === 'function') { - // To avoid cycles set the clone to null, so if we encounter it again later we can - // slot in the currently copied object instead - // @ts-ignore - cloned.set(value, null); + // Associate the instance with the toJSON clone + var ref_json_instance = value; return clone( /** @type {T & { toJSON(): any } } */ (value).toJSON(), cloned, + ref_json_instance, DEV ? `${path}.toJSON()` : path, paths ); diff --git a/packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/_config.js b/packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/_config.js index bfa47ecb3fff..ab496971955f 100644 --- a/packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/_config.js @@ -7,9 +7,17 @@ export default test({ async test({ assert, logs }) { var a = { - a: null + a: {} }; a.a = a; - assert.deepEqual(logs, ['init', { a }]); + + var b = { + a: { + b: {} + } + }; + b.a.b = b; + + assert.deepEqual(logs, ['init', a, 'init', b]); } }); diff --git a/packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/main.svelte b/packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/main.svelte index cd7eb8afdd0b..f7874d2192ce 100644 --- a/packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/inspect-recursive-2/main.svelte @@ -8,4 +8,16 @@ } const state = $state(new A()); $inspect(state); + + class B { + toJSON(){ + return { + a: { + b: this + } + } + } + } + const state2 = $state(new B()); + $inspect(state2); From dfded6c7f08dc05220c21de5e0e9fcdae77b117d Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 19 Nov 2024 12:21:50 -0500 Subject: [PATCH 3/5] 'original' feels slightly clearer than 'json_instance' --- packages/svelte/src/internal/shared/clone.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/svelte/src/internal/shared/clone.js b/packages/svelte/src/internal/shared/clone.js index 87d5968ee0ab..27470ebb84b5 100644 --- a/packages/svelte/src/internal/shared/clone.js +++ b/packages/svelte/src/internal/shared/clone.js @@ -47,12 +47,12 @@ export function snapshot(value, skip_warning = false) { * @template T * @param {T} value * @param {Map>} cloned - * @param {null | T} json_instance + * @param {null | T} original The original value, if `value` was produced from a `toJSON` call * @param {string} path * @param {string[]} paths * @returns {Snapshot} */ -function clone(value, cloned, json_instance, path, paths) { +function clone(value, cloned, original, path, paths) { if (typeof value === 'object' && value !== null) { const unwrapped = cloned.get(value); if (unwrapped !== undefined) return unwrapped; @@ -63,8 +63,8 @@ function clone(value, cloned, json_instance, path, paths) { if (is_array(value)) { const copy = /** @type {Snapshot} */ ([]); cloned.set(value, copy); - if (json_instance !== null) { - cloned.set(json_instance, copy); + if (original !== null) { + cloned.set(original, copy); } for (let i = 0; i < value.length; i += 1) { @@ -78,8 +78,8 @@ function clone(value, cloned, json_instance, path, paths) { /** @type {Snapshot} */ const copy = {}; cloned.set(value, copy); - if (json_instance !== null) { - cloned.set(json_instance, copy); + if (original !== null) { + cloned.set(original, copy); } for (var key in value) { From 75a1f58327f90da7df31d1baa8ab239df0e0398c Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 19 Nov 2024 12:24:13 -0500 Subject: [PATCH 4/5] use an optional parameter, so we can omit it in most cases --- packages/svelte/src/internal/shared/clone.js | 21 ++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/packages/svelte/src/internal/shared/clone.js b/packages/svelte/src/internal/shared/clone.js index 27470ebb84b5..d4ba2f84ea9a 100644 --- a/packages/svelte/src/internal/shared/clone.js +++ b/packages/svelte/src/internal/shared/clone.js @@ -22,7 +22,7 @@ export function snapshot(value, skip_warning = false) { /** @type {string[]} */ const paths = []; - const copy = clone(value, new Map(), null, '', paths); + const copy = clone(value, new Map(), '', paths); if (paths.length === 1 && paths[0] === '') { // value could not be cloned w.state_snapshot_uncloneable(); @@ -40,19 +40,19 @@ export function snapshot(value, skip_warning = false) { return copy; } - return clone(value, new Map(), null, '', empty); + return clone(value, new Map(), '', empty); } /** * @template T * @param {T} value * @param {Map>} cloned - * @param {null | T} original The original value, if `value` was produced from a `toJSON` call * @param {string} path * @param {string[]} paths + * @param {null | T} original The original value, if `value` was produced from a `toJSON` call * @returns {Snapshot} */ -function clone(value, cloned, original, path, paths) { +function clone(value, cloned, path, paths, original = null) { if (typeof value === 'object' && value !== null) { const unwrapped = cloned.get(value); if (unwrapped !== undefined) return unwrapped; @@ -63,12 +63,13 @@ function clone(value, cloned, original, path, paths) { if (is_array(value)) { const copy = /** @type {Snapshot} */ ([]); cloned.set(value, copy); + if (original !== null) { cloned.set(original, copy); } for (let i = 0; i < value.length; i += 1) { - copy.push(clone(value[i], cloned, null, DEV ? `${path}[${i}]` : path, paths)); + copy.push(clone(value[i], cloned, DEV ? `${path}[${i}]` : path, paths)); } return copy; @@ -78,13 +79,14 @@ function clone(value, cloned, original, path, paths) { /** @type {Snapshot} */ const copy = {}; cloned.set(value, copy); + if (original !== null) { cloned.set(original, copy); } for (var key in value) { // @ts-ignore - copy[key] = clone(value[key], cloned, null, DEV ? `${path}.${key}` : path, paths); + copy[key] = clone(value[key], cloned, DEV ? `${path}.${key}` : path, paths); } return copy; @@ -95,14 +97,13 @@ function clone(value, cloned, original, path, paths) { } if (typeof (/** @type {T & { toJSON?: any } } */ (value).toJSON) === 'function') { - // Associate the instance with the toJSON clone - var ref_json_instance = value; return clone( /** @type {T & { toJSON(): any } } */ (value).toJSON(), cloned, - ref_json_instance, DEV ? `${path}.toJSON()` : path, - paths + paths, + // Associate the instance with the toJSON clone + value ); } } From 4224888c827d59534c939da3993f28392dbd646e Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 19 Nov 2024 18:03:13 +0000 Subject: [PATCH 5/5] Update packages/svelte/src/internal/shared/clone.js Co-authored-by: Rich Harris --- packages/svelte/src/internal/shared/clone.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/svelte/src/internal/shared/clone.js b/packages/svelte/src/internal/shared/clone.js index d4ba2f84ea9a..cef14bc14f9e 100644 --- a/packages/svelte/src/internal/shared/clone.js +++ b/packages/svelte/src/internal/shared/clone.js @@ -85,7 +85,7 @@ function clone(value, cloned, path, paths, original = null) { } for (var key in value) { - // @ts-ignore + // @ts-expect-error copy[key] = clone(value[key], cloned, DEV ? `${path}.${key}` : path, paths); }