fix: improve reactive Map and Set implementations (#11827)

Closes #11727.

This PR aims to tackle issues around our reactive Map/Set implementations. Notably:

- We now store the values on the backing Map/Set, allowing for much better introspection in console/dev tools
- We no longer store the values inside the source signals, instead we use Symbols and booleans only as markers

There's one limitation around `.has(x)` when `x` is not in the Map/Set yet - it's not fine-grained. Making it so could create too much memory pressure when e.g. iterating a big list of items with few of them being in the set (`has` returns `false` most of the time).

Author: trueadm

.changeset/eight-jeans-compare.md

@@ -0,0 +1,5 @@
+---
+"svelte": patch
+---
+
+fix: improve reactive Map and Set implementations

packages/svelte/src/internal/client/dev/inspect.js

@@ -62,16 +62,6 @@ export function inspect(get_value, inspector = console.log) {
  */
 function deep_snapshot(value, visited = new Map()) {
 	if (typeof value === 'object' && value !== null && !visited.has(value)) {
-		if (DEV) {
-			// When dealing with ReactiveMap or ReactiveSet, return normal versions
-			// so that console.log provides better output versions
-			if (value instanceof Map && value.constructor !== Map) {
-				return new Map(value);
-			}
-			if (value instanceof Set && value.constructor !== Set) {
-				return new Set(value);
-			}
-		}
 		const unstated = snapshot(value);
 
 		if (unstated !== value) {

packages/svelte/src/reactivity/map.js

@@ -2,15 +2,14 @@ import { DEV } from 'esm-env';
 import { source, set } from '../internal/client/reactivity/sources.js';
 import { get } from '../internal/client/runtime.js';
 import { UNINITIALIZED } from '../constants.js';
-import { map } from './utils.js';
 
 /**
  * @template K
  * @template V
  * @extends {Map<K, V>}
  */
 export class ReactiveMap extends Map {
-	/** @type {Map<K, import('#client').Source<V>>} */
+	/** @type {Map<K, import('#client').Source<symbol>>} */
 	#sources = new Map();
 	#version = source(0);
 	#size = source(0);
@@ -25,13 +24,10 @@ export class ReactiveMap extends Map {
 		if (DEV) new Map(value);
 
 		if (value) {
-			var sources = this.#sources;
-
 			for (var [key, v] of value) {
-				sources.set(key, source(v));
+				super.set(key, v);
 			}
-
-			this.#size.v = sources.size;
+			this.#size.v = super.size;
 		}
 	}
 
@@ -41,14 +37,20 @@ export class ReactiveMap extends Map {
 
 	/** @param {K} key */
 	has(key) {
-		var s = this.#sources.get(key);
+		var sources = this.#sources;
+		var s = sources.get(key);
 
 		if (s === undefined) {
-			// We should always track the version in case
-			// the Set ever gets this value in the future.
-			get(this.#version);
-
-			return false;
+			var ret = super.get(key);
+			if (ret !== undefined) {
+				s = source(Symbol());
+				sources.set(key, s);
+			} else {
+				// We should always track the version in case
+				// the Set ever gets this value in the future.
+				get(this.#version);
+				return false;
+			}
 		}
 
 		get(s);
@@ -62,23 +64,29 @@ export class ReactiveMap extends Map {
 	forEach(callbackfn, this_arg) {
 		get(this.#version);
 
-		var bound_callbackfn = callbackfn.bind(this_arg);
-		this.#sources.forEach((s, key) => bound_callbackfn(s.v, key, this));
+		return super.forEach(callbackfn, this_arg);
 	}
 
 	/** @param {K} key */
 	get(key) {
-		var s = this.#sources.get(key);
+		var sources = this.#sources;
+		var s = sources.get(key);
 
 		if (s === undefined) {
-			// We should always track the version in case
-			// the Set ever gets this value in the future.
-			get(this.#version);
-
-			return undefined;
+			var ret = super.get(key);
+			if (ret !== undefined) {
+				s = source(Symbol());
+				sources.set(key, s);
+			} else {
+				// We should always track the version in case
+				// the Set ever gets this value in the future.
+				get(this.#version);
+				return undefined;
+			}
 		}
 
-		return get(s);
+		get(s);
+		return super.get(key);
 	}
 
 	/**
@@ -88,72 +96,71 @@ export class ReactiveMap extends Map {
 	set(key, value) {
 		var sources = this.#sources;
 		var s = sources.get(key);
+		var prev_res = super.get(key);
+		var res = super.set(key, value);
 
 		if (s === undefined) {
-			sources.set(key, source(value));
-			set(this.#size, sources.size);
+			sources.set(key, source(Symbol()));
+			set(this.#size, super.size);
 			this.#increment_version();
-		} else {
-			set(s, value);
+		} else if (prev_res !== value) {
+			set(s, Symbol());
 		}
 
-		return this;
+		return res;
 	}
 
 	/** @param {K} key */
 	delete(key) {
 		var sources = this.#sources;
 		var s = sources.get(key);
+		var res = super.delete(key);
 
 		if (s !== undefined) {
-			var removed = sources.delete(key);
-			set(this.#size, sources.size);
-			set(s, /** @type {V} */ (UNINITIALIZED));
+			sources.delete(key);
+			set(this.#size, super.size);
+			set(s, UNINITIALIZED);
 			this.#increment_version();
-			return removed;
 		}
 
-		return false;
+		return res;
 	}
 
 	clear() {
 		var sources = this.#sources;
 
-		if (sources.size !== 0) {
+		if (super.size !== 0) {
 			set(this.#size, 0);
 			for (var s of sources.values()) {
-				set(s, /** @type {V} */ (UNINITIALIZED));
+				set(s, UNINITIALIZED);
 			}
 			this.#increment_version();
+			sources.clear();
 		}
-
-		sources.clear();
+		super.clear();
 	}
 
 	keys() {
 		get(this.#version);
-		return this.#sources.keys();
+		return super.keys();
 	}
 
 	values() {
 		get(this.#version);
-		return map(this.#sources.values(), get, 'Map Iterator');
+		return super.values();
 	}
 
 	entries() {
 		get(this.#version);
-		return map(
-			this.#sources.entries(),
-			([key, source]) => /** @type {[K, V]} */ ([key, get(source)]),
-			'Map Iterator'
-		);
+		return super.entries();
 	}
 
 	[Symbol.iterator]() {
 		return this.entries();
 	}
 
 	get size() {
-		return get(this.#size);
+		get(this.#size);
+		return super.size;
 	}
 }

packages/svelte/src/reactivity/map.test.ts

@@ -183,3 +183,35 @@ test('map handling of undefined values', () => {
 
 	cleanup();
 });
+
+test('not invoking reactivity when value is not in the map after changes', () => {
+	const map = new ReactiveMap([[1, 1]]);
+
+	const log: any = [];
+
+	const cleanup = effect_root(() => {
+		render_effect(() => {
+			log.push(map.get(1));
+		});
+
+		render_effect(() => {
+			log.push(map.get(2));
+		});
+
+		flushSync(() => {
+			map.delete(1);
+		});
+
+		flushSync(() => {
+			map.set(1, 1);
+		});
+	});
+
+	assert.deepEqual(log, [1, undefined, undefined, undefined, 1, undefined]);
+
+	cleanup();
+});
+
+test('Map.instanceOf', () => {
+	assert.equal(new ReactiveMap() instanceof Map, true);
+});