fix: better handle $inspect on array mutations (#16389)

* fix: better handle $inspect on array mutations

* increase stack trace limit, revert test change

* second changeset

---------

Co-authored-by: Rich Harris <[email protected]>

Author: 7nik

.changeset/curvy-houses-jog.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: better handle $inspect on array mutations

.changeset/metal-coats-thank.md

@@ -0,0 +1,5 @@
+---
+"svelte": patch
+---
+
+fix: leave proxied array `length` untouched when deleting properties

packages/svelte/src/internal/client/proxy.js

@@ -15,7 +15,13 @@ import {
 	is_array,
 	object_prototype
 } from '../shared/utils.js';
-import { state as source, set, increment } from './reactivity/sources.js';
+import {
+	state as source,
+	set,
+	increment,
+	flush_inspect_effects,
+	set_inspect_effects_deferred
+} from './reactivity/sources.js';
 import { PROXY_PATH_SYMBOL, STATE_SYMBOL } from '#client/constants';
 import { UNINITIALIZED } from '../../constants.js';
 import * as e from './errors.js';
@@ -80,6 +86,9 @@ export function proxy(value) {
 		// We need to create the length source eagerly to ensure that
 		// mutations to the array are properly synced with our proxy
 		sources.set('length', source(/** @type {any[]} */ (value).length, stack));
+		if (DEV) {
+			value = /** @type {any} */ (inspectable_array(/** @type {any[]} */ (value)));
+		}
 	}
 
 	/** Used in dev for $inspect.trace() */
@@ -142,16 +151,6 @@ export function proxy(value) {
 					}
 				}
 			} else {
-				// When working with arrays, we need to also ensure we update the length when removing
-				// an indexed property
-				if (is_proxied_array && typeof prop === 'string') {
-					var ls = /** @type {Source<number>} */ (sources.get('length'));
-					var n = Number(prop);
-
-					if (Number.isInteger(n) && n < ls.v) {
-						set(ls, n);
-					}
-				}
 				set(s, UNINITIALIZED);
 				increment(version);
 			}
@@ -388,3 +387,42 @@ export function get_proxied_value(value) {
 export function is(a, b) {
 	return Object.is(get_proxied_value(a), get_proxied_value(b));
 }
+
+const ARRAY_MUTATING_METHODS = new Set([
+	'copyWithin',
+	'fill',
+	'pop',
+	'push',
+	'reverse',
+	'shift',
+	'sort',
+	'splice',
+	'unshift'
+]);
+
+/**
+ * Wrap array mutating methods so $inspect is triggered only once and
+ * to prevent logging an array in intermediate state (e.g. with an empty slot)
+ * @param {any[]} array
+ */
+function inspectable_array(array) {
+	return new Proxy(array, {
+		get(target, prop, receiver) {
+			var value = Reflect.get(target, prop, receiver);
+			if (!ARRAY_MUTATING_METHODS.has(/** @type {string} */ (prop))) {
+				return value;
+			}
+
+			/**
+			 * @this {any[]}
+			 * @param {any[]} args
+			 */
+			return function (...args) {
+				set_inspect_effects_deferred();
+				var result = value.apply(this, args);
+				flush_inspect_effects();
+				return result;
+			};
+		}
+	});
+}

packages/svelte/src/internal/client/reactivity/sources.js

@@ -49,6 +49,12 @@ export function set_inspect_effects(v) {
 	inspect_effects = v;
 }
 
+let inspect_effects_deferred = false;
+
+export function set_inspect_effects_deferred() {
+	inspect_effects_deferred = true;
+}
+
 /**
  * @template V
  * @param {V} v
@@ -213,26 +219,32 @@ export function internal_set(source, value) {
 			}
 		}
 
-		if (DEV && inspect_effects.size > 0) {
-			const inspects = Array.from(inspect_effects);
+		if (DEV && inspect_effects.size > 0 && !inspect_effects_deferred) {
+			flush_inspect_effects();
+		}
+	}
+
+	return value;
+}
 
-			for (const effect of inspects) {
-				// Mark clean inspect-effects as maybe dirty and then check their dirtiness
-				// instead of just updating the effects - this way we avoid overfiring.
-				if ((effect.f & CLEAN) !== 0) {
-					set_signal_status(effect, MAYBE_DIRTY);
-				}
+export function flush_inspect_effects() {
+	inspect_effects_deferred = false;
 
-				if (is_dirty(effect)) {
-					update_effect(effect);
-				}
-			}
+	const inspects = Array.from(inspect_effects);
 
-			inspect_effects.clear();
+	for (const effect of inspects) {
+		// Mark clean inspect-effects as maybe dirty and then check their dirtiness
+		// instead of just updating the effects - this way we avoid overfiring.
+		if ((effect.f & CLEAN) !== 0) {
+			set_signal_status(effect, MAYBE_DIRTY);
+		}
+
+		if (is_dirty(effect)) {
+			update_effect(effect);
 		}
 	}
 
-	return value;
+	inspect_effects.clear();
 }
 
 /**

packages/svelte/tests/runtime-runes/samples/array-delete-item/_config.js

@@ -0,0 +1,13 @@
+import { ok, test } from '../../test';
+import { flushSync } from 'svelte';
+
+export default test({
+	mode: ['client'],
+	async test({ target, assert, logs }) {
+		const btn = target.querySelector('button');
+
+		flushSync(() => btn?.click());
+
+		assert.deepEqual(logs[0], [0, , 2]);
+	}
+});

packages/svelte/tests/runtime-runes/samples/array-delete-item/main.svelte

@@ -0,0 +1,8 @@
+<script>
+	const arr = [0, 1, 2];
+</script>
+
+<button onclick={() => {
+	delete arr[1];
+	console.log(arr);
+}}>del</button>

packages/svelte/tests/runtime-runes/samples/inspect-deep-array/_config.js

@@ -13,17 +13,6 @@ export default test({
 			button?.click();
 		});
 
-		assert.deepEqual(logs, [
-			'init',
-			[1, 2, 3, 7],
-			'update',
-			[2, 2, 3, 7],
-			'update',
-			[2, 3, 3, 7],
-			'update',
-			[2, 3, 7, 7],
-			'update',
-			[2, 3, 7]
-		]);
+		assert.deepEqual(logs, ['init', [1, 2, 3, 7], 'update', [2, 3, 7]]);
 	}
 });

packages/svelte/tests/suite.ts

@@ -20,6 +20,9 @@ const filter = process.env.FILTER
 		)
 	: /./;
 
+// this defaults to 10, which is too low for some of our tests
+Error.stackTraceLimit = 100;
+
 export function suite<Test extends BaseTest>(fn: (config: Test, test_dir: string) => void) {
 	return {
 		test: (config: Test) => config,