Skip to content

Commit 37f0616

Browse files
thetarnavthetarnav
authored
Fix and optimize ReactiveMap and ReactiveWeakMap (#704)
2 parents 1daf96b + 46e4379 commit 37f0616

File tree

4 files changed

+120
-106
lines changed

4 files changed

+120
-106
lines changed

‎.changeset/perfect-jars-visit.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@solid-primitives/map": minor
3+
---
4+
5+
Fix and optimize ReactiveMap and ReactiveWeakMap (https://github.com/solidjs-community/solid-primitives/pull/704)

‎packages/map/src/index.ts‎

Lines changed: 94 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { Accessor, batch } from "solid-js";
22
import { TriggerCache } from "@solid-primitives/trigger";
33

4-
const $KEYS = Symbol("track-keys");
4+
const $OBJECT = Symbol("track-object");
55

66
/**
77
* A reactive version of `Map` data structure. All the reads (like `get` or `has`) are signals, and all the writes (`delete` or `set`) will cause updates to appropriate signals.
@@ -21,100 +21,114 @@ const $KEYS = Symbol("track-keys");
2121
* userPoints.set(user1, { foo: "bar" });
2222
*/
2323
export class ReactiveMap<K, V> extends Map<K, V> {
24-
#keyTriggers = new TriggerCache<K | typeof $KEYS>();
25-
#valueTriggers = new TriggerCache<K>();
24+
#keyTriggers = new TriggerCache<K | typeof $OBJECT>();
25+
#valueTriggers = new TriggerCache<K | typeof $OBJECT>();
2626

27-
constructor(initial?: Iterable<readonly [K, V]> | null) {
28-
super();
29-
if (initial) for (const v of initial) super.set(v[0], v[1]);
27+
[Symbol.iterator](): MapIterator<[K, V]> {
28+
return this.entries();
3029
}
3130

32-
// reads
33-
has(key: K): boolean {
34-
this.#keyTriggers.track(key);
35-
return super.has(key);
36-
}
37-
get(key: K): V | undefined {
38-
this.#valueTriggers.track(key);
39-
return super.get(key);
31+
constructor(entries?: Iterable<readonly [K, V]> | null) {
32+
super();
33+
if (entries) for (const entry of entries) super.set(...entry);
4034
}
35+
4136
get size(): number {
42-
this.#keyTriggers.track($KEYS);
37+
this.#keyTriggers.track($OBJECT);
4338
return super.size;
4439
}
4540

4641
*keys(): MapIterator<K> {
42+
this.#keyTriggers.track($OBJECT);
43+
4744
for (const key of super.keys()) {
48-
this.#keyTriggers.track(key);
4945
yield key;
5046
}
51-
this.#keyTriggers.track($KEYS);
5247
}
48+
5349
*values(): MapIterator<V> {
54-
for (const [key, v] of super.entries()) {
55-
this.#valueTriggers.track(key);
56-
yield v;
50+
this.#valueTriggers.track($OBJECT);
51+
52+
for (const value of super.values()) {
53+
yield value;
5754
}
58-
this.#keyTriggers.track($KEYS);
5955
}
56+
6057
*entries(): MapIterator<[K, V]> {
58+
this.#keyTriggers.track($OBJECT);
59+
this.#valueTriggers.track($OBJECT);
60+
6161
for (const entry of super.entries()) {
62-
this.#valueTriggers.track(entry[0]);
6362
yield entry;
6463
}
65-
this.#keyTriggers.track($KEYS);
6664
}
6765

68-
// writes
66+
forEach(callbackfn: (value: V, key: K, map: Map<K, V>) => void, thisArg?: any): void {
67+
this.#keyTriggers.track($OBJECT);
68+
this.#valueTriggers.track($OBJECT);
69+
super.forEach(callbackfn, thisArg);
70+
}
71+
72+
has(key: K): boolean {
73+
this.#keyTriggers.track(key);
74+
return super.has(key);
75+
}
76+
77+
get(key: K): V | undefined {
78+
this.#valueTriggers.track(key);
79+
return super.get(key);
80+
}
81+
6982
set(key: K, value: V): this {
70-
batch(() => {
71-
if (super.has(key)) {
72-
if (super.get(key)! === value) return;
73-
} else {
74-
this.#keyTriggers.dirty(key);
75-
this.#keyTriggers.dirty($KEYS);
76-
}
77-
this.#valueTriggers.dirty(key);
78-
super.set(key, value);
79-
});
80-
return this;
83+
const hadNoKey = !super.has(key);
84+
const hasChanged = super.get(key) !== value;
85+
const result = super.set(key, value);
86+
87+
if (hasChanged || hadNoKey) {
88+
batch(() => {
89+
if (hadNoKey) {
90+
this.#keyTriggers.dirty($OBJECT);
91+
this.#keyTriggers.dirty(key);
92+
}
93+
if (hasChanged) {
94+
this.#valueTriggers.dirty($OBJECT);
95+
this.#valueTriggers.dirty(key);
96+
}
97+
});
98+
}
99+
100+
return result;
81101
}
102+
82103
delete(key: K): boolean {
83-
const r = super.delete(key);
84-
if (r) {
104+
const isDefined = super.get(key) !== undefined;
105+
const result = super.delete(key);
106+
107+
if (result) {
85108
batch(() => {
109+
this.#keyTriggers.dirty($OBJECT);
110+
this.#valueTriggers.dirty($OBJECT);
86111
this.#keyTriggers.dirty(key);
87-
this.#keyTriggers.dirty($KEYS);
88-
this.#valueTriggers.dirty(key);
112+
113+
if (isDefined) {
114+
this.#valueTriggers.dirty(key);
115+
}
89116
});
90117
}
91-
return r;
118+
119+
return result;
92120
}
121+
93122
clear(): void {
94123
if (super.size) {
124+
super.clear();
125+
95126
batch(() => {
96-
for (const v of super.keys()) {
97-
this.#keyTriggers.dirty(v);
98-
this.#valueTriggers.dirty(v);
99-
}
100-
super.clear();
101-
this.#keyTriggers.dirty($KEYS);
127+
this.#keyTriggers.dirtyAll();
128+
this.#valueTriggers.dirtyAll();
102129
});
103130
}
104131
}
105-
106-
// callback
107-
forEach(callbackfn: (value: V, key: K, map: this) => void) {
108-
this.#keyTriggers.track($KEYS);
109-
for (const [key, v] of super.entries()) {
110-
this.#valueTriggers.track(key);
111-
callbackfn(v, key, this);
112-
}
113-
}
114-
115-
[Symbol.iterator](): MapIterator<[K, V]> {
116-
return this.entries();
117-
}
118132
}
119133

120134
/**
@@ -137,9 +151,9 @@ export class ReactiveWeakMap<K extends object, V> extends WeakMap<K, V> {
137151
#keyTriggers = new TriggerCache<K>(WeakMap);
138152
#valueTriggers = new TriggerCache<K>(WeakMap);
139153

140-
constructor(initial?: Iterable<readonly [K, V]> | null) {
154+
constructor(entries?: Iterable<readonly [K, V]> | null) {
141155
super();
142-
if (initial) for (const v of initial) super.set(v[0], v[1]);
156+
if (entries) for (const entry of entries) super.set(...entry);
143157
}
144158

145159
has(key: K): boolean {
@@ -151,24 +165,31 @@ export class ReactiveWeakMap<K extends object, V> extends WeakMap<K, V> {
151165
return super.get(key);
152166
}
153167
set(key: K, value: V): this {
154-
batch(() => {
155-
if (super.has(key)) {
156-
if (super.get(key)! === value) return;
157-
} else this.#keyTriggers.dirty(key);
158-
this.#valueTriggers.dirty(key);
159-
super.set(key, value);
160-
});
161-
return this;
168+
const hadNoKey = !super.has(key);
169+
const hasChanged = super.get(key) !== value;
170+
const result = super.set(key, value);
171+
172+
if (hasChanged || hadNoKey) {
173+
batch(() => {
174+
if (hadNoKey) this.#keyTriggers.dirty(key);
175+
if (hasChanged) this.#valueTriggers.dirty(key);
176+
});
177+
}
178+
179+
return result;
162180
}
163181
delete(key: K): boolean {
164-
const r = super.delete(key);
165-
if (r) {
182+
const isDefined = super.get(key) !== undefined;
183+
const result = super.delete(key);
184+
185+
if (result) {
166186
batch(() => {
167187
this.#keyTriggers.dirty(key);
168-
this.#valueTriggers.dirty(key);
188+
if (isDefined) this.#valueTriggers.dirty(key);
169189
});
170190
}
171-
return r;
191+
192+
return result;
172193
}
173194
}
174195

‎packages/map/test/index.test.ts‎

Lines changed: 19 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,6 @@ describe("ReactiveMap", () => {
210210
[1, "a"],
211211
[2, "b"],
212212
[3, "c"],
213-
[4, "d"],
214213
]);
215214

216215
const captured: unknown[][] = [];
@@ -220,7 +219,6 @@ describe("ReactiveMap", () => {
220219
const run: unknown[] = [];
221220
for (const key of map.keys()) {
222221
run.push(key);
223-
if (key === 3) break; // don't iterate over all keys
224222
}
225223
captured.push(run);
226224
});
@@ -233,12 +231,12 @@ describe("ReactiveMap", () => {
233231
map.set(1, "e");
234232
expect(captured, "value change").toHaveLength(1);
235233

236-
map.set(5, "f");
237-
expect(captured, "not seen key change").toHaveLength(1);
234+
map.set(4, "f");
235+
expect(captured, "new key added").toHaveLength(2);
238236

239237
map.delete(1);
240-
expect(captured, "seen key change").toHaveLength(2);
241-
expect(captured[1]).toEqual([2, 3]);
238+
expect(captured, "seen key change").toHaveLength(3);
239+
expect(captured[2]).toEqual([2, 3, 4]);
242240

243241
dispose();
244242
});
@@ -248,19 +246,15 @@ describe("ReactiveMap", () => {
248246
[1, "a"],
249247
[2, "b"],
250248
[3, "c"],
251-
[4, "d"],
252249
]);
253250

254251
const captured: unknown[][] = [];
255252

256253
const dispose = createRoot(dispose => {
257254
createEffect(() => {
258255
const run: unknown[] = [];
259-
let i = 0;
260256
for (const v of map.values()) {
261257
run.push(v);
262-
if (i === 2) break; // don't iterate over all keys
263-
i += 1;
264258
}
265259
captured.push(run);
266260
});
@@ -275,14 +269,12 @@ describe("ReactiveMap", () => {
275269
expect(captured[1]).toEqual(["e", "b", "c"]);
276270

277271
map.set(4, "f");
278-
expect(captured, "not seen value change").toHaveLength(2);
272+
expect(captured, "new key added").toHaveLength(3);
273+
expect(captured[2]).toEqual(["e", "b", "c", "f"]);
279274

280275
map.delete(4);
281-
expect(captured, "not seen key change").toHaveLength(2);
282-
283-
map.delete(1);
284-
expect(captured, "seen key change").toHaveLength(3);
285-
expect(captured[2]).toEqual(["b", "c"]);
276+
expect(captured, "key removed").toHaveLength(4);
277+
expect(captured[3]).toEqual(["e", "b", "c"]);
286278

287279
dispose();
288280
});
@@ -292,19 +284,15 @@ describe("ReactiveMap", () => {
292284
[1, "a"],
293285
[2, "b"],
294286
[3, "c"],
295-
[4, "d"],
296287
]);
297288

298289
const captured: unknown[][] = [];
299290

300291
const dispose = createRoot(dispose => {
301292
createEffect(() => {
302293
const run: unknown[] = [];
303-
let i = 0;
304294
for (const e of map.entries()) {
305295
run.push(e);
306-
if (i === 2) break; // don't iterate over all keys
307-
i += 1;
308296
}
309297
captured.push(run);
310298
});
@@ -327,14 +315,18 @@ describe("ReactiveMap", () => {
327315
]);
328316

329317
map.set(4, "f");
330-
expect(captured, "not seen value change").toHaveLength(2);
318+
expect(captured, "new key added").toHaveLength(3);
319+
expect(captured[2]).toEqual([
320+
[1, "e"],
321+
[2, "b"],
322+
[3, "c"],
323+
[4, "f"],
324+
]);
331325

332326
map.delete(4);
333-
expect(captured, "not seen key change").toHaveLength(2);
334-
335-
map.delete(1);
336-
expect(captured, "seen key change").toHaveLength(3);
337-
expect(captured[2]).toEqual([
327+
expect(captured, "key removed").toHaveLength(4);
328+
expect(captured[3]).toEqual([
329+
[1, "e"],
338330
[2, "b"],
339331
[3, "c"],
340332
]);
@@ -347,7 +339,6 @@ describe("ReactiveMap", () => {
347339
[1, "a"],
348340
[2, "b"],
349341
[3, "c"],
350-
[4, "d"],
351342
]);
352343

353344
const captured: unknown[][] = [];
@@ -368,7 +359,6 @@ describe("ReactiveMap", () => {
368359
[1, "a"],
369360
[2, "b"],
370361
[3, "c"],
371-
[4, "d"],
372362
]);
373363

374364
map.set(1, "e");
@@ -377,15 +367,13 @@ describe("ReactiveMap", () => {
377367
[1, "e"],
378368
[2, "b"],
379369
[3, "c"],
380-
[4, "d"],
381370
]);
382371

383-
map.delete(4);
372+
map.delete(3);
384373
expect(captured).toHaveLength(3);
385374
expect(captured[2]).toEqual([
386375
[1, "e"],
387376
[2, "b"],
388-
[3, "c"],
389377
]);
390378

391379
dispose();

0 commit comments

Comments
 (0)