diff --git a/.changeset/eight-jeans-compare.md b/.changeset/eight-jeans-compare.md new file mode 100644 index 000000000000..fbd71dd1c273 --- /dev/null +++ b/.changeset/eight-jeans-compare.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: improve reactive Map and Set implementations diff --git a/packages/svelte/src/internal/client/dev/inspect.js b/packages/svelte/src/internal/client/dev/inspect.js index 3e01dc5b4470..fb37f99df277 100644 --- a/packages/svelte/src/internal/client/dev/inspect.js +++ b/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) { diff --git a/packages/svelte/src/reactivity/map.js b/packages/svelte/src/reactivity/map.js index 59fcfc1cf09c..ece3d1845d2b 100644 --- a/packages/svelte/src/reactivity/map.js +++ b/packages/svelte/src/reactivity/map.js @@ -2,7 +2,6 @@ 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 @@ -10,7 +9,7 @@ import { map } from './utils.js'; * @extends {Map} */ export class ReactiveMap extends Map { - /** @type {Map>} */ + /** @type {Map>} */ #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,65 +96,63 @@ 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]() { @@ -154,6 +160,7 @@ export class ReactiveMap extends Map { } get size() { - return get(this.#size); + get(this.#size); + return super.size; } } diff --git a/packages/svelte/src/reactivity/map.test.ts b/packages/svelte/src/reactivity/map.test.ts index 7777392cf997..3cdba4bb38e5 100644 --- a/packages/svelte/src/reactivity/map.test.ts +++ b/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); +}); diff --git a/packages/svelte/src/reactivity/set.js b/packages/svelte/src/reactivity/set.js index 0a4963f83964..809109846a92 100644 --- a/packages/svelte/src/reactivity/set.js +++ b/packages/svelte/src/reactivity/set.js @@ -1,7 +1,6 @@ import { DEV } from 'esm-env'; import { source, set } from '../internal/client/reactivity/sources.js'; import { get } from '../internal/client/runtime.js'; -import { map } from './utils.js'; var read_methods = ['forEach', 'isDisjointFrom', 'isSubsetOf', 'isSupersetOf']; var set_like_methods = ['difference', 'intersection', 'symmetricDifference', 'union']; @@ -28,13 +27,10 @@ export class ReactiveSet extends Set { if (DEV) new Set(value); if (value) { - var sources = this.#sources; - for (var element of value) { - sources.set(element, source(true)); + super.add(element); } - - this.#size.v = sources.size; + this.#size.v = super.size; } if (!inited) this.#init(); @@ -51,11 +47,8 @@ export class ReactiveSet extends Set { // @ts-ignore proto[method] = function (...v) { get(this.#version); - // We don't populate the underlying Set, so we need to create a clone using - // our internal values and then pass that to the method. - var clone = new Set(this.values()); // @ts-ignore - return set_proto[method].apply(clone, v); + return set_proto[method].apply(this, v); }; } @@ -63,11 +56,8 @@ export class ReactiveSet extends Set { // @ts-ignore proto[method] = function (...v) { get(this.#version); - // We don't populate the underlying Set, so we need to create a clone using - // our internal values and then pass that to the method. - var clone = new Set(this.values()); // @ts-ignore - var set = /** @type {Set} */ (set_proto[method].apply(clone, v)); + var set = /** @type {Set} */ (set_proto[method].apply(this, v)); return new ReactiveSet(set); }; } @@ -79,73 +69,86 @@ export class ReactiveSet extends Set { /** @param {T} value */ has(value) { - var s = this.#sources.get(value); + var sources = this.#sources; + var s = sources.get(value); 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.has(value); + if (ret) { + s = source(true); + sources.set(value, s); + } else { + // We should always track the version in case + // the Set ever gets this value in the future. + get(this.#version); + return false; + } } - return get(s); + get(s); + return super.has(value); } /** @param {T} value */ add(value) { var sources = this.#sources; + var res = super.add(value); + var s = sources.get(value); - if (!sources.has(value)) { + if (s === undefined) { sources.set(value, source(true)); - set(this.#size, sources.size); + set(this.#size, super.size); this.#increment_version(); + } else { + set(s, true); } - return this; + return res; } /** @param {T} value */ delete(value) { var sources = this.#sources; var s = sources.get(value); + var res = super.delete(value); if (s !== undefined) { - var removed = sources.delete(value); - set(this.#size, sources.size); + sources.delete(value); + set(this.#size, super.size); set(s, false); 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, false); } this.#increment_version(); + sources.clear(); } - - sources.clear(); + super.clear(); } keys() { get(this.#version); - return map(this.#sources.keys(), (key) => key, 'Set Iterator'); + return super.keys(); } values() { - return this.keys(); + get(this.#version); + return super.values(); } entries() { - return map(this.keys(), (key) => /** @type {[T, T]} */ ([key, key]), 'Set Iterator'); + get(this.#version); + return super.entries(); } [Symbol.iterator]() { diff --git a/packages/svelte/src/reactivity/set.test.ts b/packages/svelte/src/reactivity/set.test.ts index 522601451804..97d0869c9e66 100644 --- a/packages/svelte/src/reactivity/set.test.ts +++ b/packages/svelte/src/reactivity/set.test.ts @@ -106,3 +106,54 @@ test('set.forEach()', () => { cleanup(); }); + +test('not invoking reactivity when value is not in the set after changes', () => { + const set = new ReactiveSet([1, 2]); + + const log: any = []; + + const cleanup = effect_root(() => { + render_effect(() => { + log.push('has 1', set.has(1)); + }); + + render_effect(() => { + log.push('has 2', set.has(2)); + }); + + render_effect(() => { + log.push('has 3', set.has(3)); + }); + }); + + flushSync(() => { + set.delete(2); + }); + + flushSync(() => { + set.add(2); + }); + + assert.deepEqual(log, [ + 'has 1', + true, + 'has 2', + true, + 'has 3', + false, + 'has 2', + false, + 'has 3', + false, + 'has 2', + true, + 'has 3', + false + ]); + + cleanup(); +}); + +test('Set.instanceOf', () => { + assert.equal(new ReactiveSet() instanceof Set, true); +});