Skip to content

Commit ea8c64d

Browse files
megothssNullVoxPopuli
authored andcommitted
[BUGFIX] Fix trackedMap reactivity for existing keys
Fix set() truthiness bug and iteration methods missing value updates on existing keys. set() was using value truthiness (`if (existing)`) instead of `this.#vals.has(key)` to decide whether to run the equality check and whether to dirty the collection tag. This caused: - Existing truthy values: collection tag never dirtied, so iteration consumers missed the update - Existing falsy values (0, false, null, ""): equality check skipped, collection tag dirtied unnecessarily Additionally, entries(), values(), and forEach() only consumed the collection tag, missing per-key value updates even with a correct set(). These now delegate to [Symbol.iterator]() which already consumes per-key tags via self.get(). Fixes #21123
1 parent e9c61cc commit ea8c64d

4 files changed

Lines changed: 116 additions & 13 deletions

File tree

packages/@glimmer-workspace/integration-tests/test/collections/map-test.ts

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -268,6 +268,82 @@ class TrackedMapTest extends RenderTest {
268268
);
269269
}
270270

271+
@test
272+
'values: set existing value triggers reactivity'() {
273+
this.assertReactivity(
274+
class extends Component {
275+
map = trackedMap([['foo', 123]]);
276+
277+
get value() {
278+
let sum = 0;
279+
for (const v of this.map.values()) sum += v;
280+
return sum;
281+
}
282+
283+
update() {
284+
this.map.set('foo', 456);
285+
}
286+
}
287+
);
288+
}
289+
290+
@test
291+
'entries: set existing value triggers reactivity'() {
292+
this.assertReactivity(
293+
class extends Component {
294+
map = trackedMap([['foo', 123]]);
295+
296+
get value() {
297+
let result = '';
298+
for (const [k, v] of this.map.entries()) result += `${k}:${v}`;
299+
return result;
300+
}
301+
302+
update() {
303+
this.map.set('foo', 456);
304+
}
305+
}
306+
);
307+
}
308+
309+
@test
310+
'forEach: set existing value triggers reactivity'() {
311+
this.assertReactivity(
312+
class extends Component {
313+
map = trackedMap([['foo', 123]]);
314+
315+
get value() {
316+
let sum = 0;
317+
this.map.forEach((v) => {
318+
sum += v;
319+
});
320+
return sum;
321+
}
322+
323+
update() {
324+
this.map.set('foo', 456);
325+
}
326+
}
327+
);
328+
}
329+
330+
@test
331+
'set existing falsy value triggers reactivity'() {
332+
this.assertReactivity(
333+
class extends Component {
334+
map = trackedMap<string, number>([['foo', 0]]);
335+
336+
get value() {
337+
return this.map.get('foo');
338+
}
339+
340+
update() {
341+
this.map.set('foo', 123);
342+
}
343+
}
344+
);
345+
}
346+
271347
@test
272348
'each: set'() {
273349
this.assertEachReactivity(

packages/@glimmer-workspace/integration-tests/test/collections/weak-map-test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,24 @@ class TrackedWeakMapTest extends RenderTest {
187187
);
188188
}
189189

190+
@test
191+
'set existing falsy value triggers reactivity'() {
192+
this.assertReactivity(
193+
class extends Component {
194+
obj = {};
195+
map = trackedWeakMap<object, number>([[this.obj, 0]]);
196+
197+
get value() {
198+
return this.map.get(this.obj);
199+
}
200+
201+
update() {
202+
this.map.set(this.obj, 123);
203+
}
204+
}
205+
);
206+
}
207+
190208
@test 'delete unrelated value'() {
191209
this.assertReactivity(
192210
class extends Component {

packages/@glimmer/validator/lib/collections/map.ts

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,8 @@ class TrackedMap<K = unknown, V = unknown> implements Map<K, V> {
5151
}
5252

5353
// **** ALL GETTERS ****
54-
entries() {
55-
consumeTag(this.#collection);
56-
57-
return this.#vals.entries();
54+
entries(): MapIterator<[K, V]> {
55+
return this[Symbol.iterator]();
5856
}
5957

6058
getOrInsert(key: K, defaultValue: V): V {
@@ -82,15 +80,23 @@ class TrackedMap<K = unknown, V = unknown> implements Map<K, V> {
8280
}
8381

8482
values() {
85-
consumeTag(this.#collection);
83+
let iterator = this[Symbol.iterator]();
8684

87-
return this.#vals.values();
85+
return {
86+
next() {
87+
let { value, done } = iterator.next();
88+
return { value: done ? (undefined as V) : value![1], done };
89+
},
90+
[Symbol.iterator]() {
91+
return this;
92+
},
93+
} as MapIterator<V>;
8894
}
8995

9096
forEach(fn: (value: V, key: K, map: Map<K, V>) => void): void {
91-
consumeTag(this.#collection);
92-
93-
this.#vals.forEach(fn);
97+
for (let [key, value] of this) {
98+
fn(value, key, this);
99+
}
94100
}
95101

96102
get size(): number {
@@ -121,6 +127,9 @@ class TrackedMap<K = unknown, V = unknown> implements Map<K, V> {
121127
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
122128
return { value: [currentKey, self.get(currentKey!)], done: false };
123129
},
130+
[Symbol.iterator]() {
131+
return this;
132+
},
124133
} as MapIterator<[K, V]>;
125134
}
126135

@@ -129,10 +138,10 @@ class TrackedMap<K = unknown, V = unknown> implements Map<K, V> {
129138
}
130139

131140
set(key: K, value: V): this {
132-
let existing = this.#vals.get(key);
141+
let existing = this.#vals.has(key);
133142

134143
if (existing) {
135-
let isUnchanged = this.#options.equals(existing, value);
144+
let isUnchanged = this.#options.equals(this.#vals.get(key) as V, value);
136145

137146
if (isUnchanged) {
138147
return this;

packages/@glimmer/validator/lib/collections/weak-map.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,10 @@ class TrackedWeakMap<K extends WeakKey = object, V = unknown> implements WeakMap
5151
}
5252

5353
set(key: K, value: V): this {
54-
let existing = this.#vals.get(key);
54+
let existing = this.#vals.has(key);
5555

5656
if (existing) {
57-
let isUnchanged = this.#options.equals(existing, value);
57+
let isUnchanged = this.#options.equals(this.#vals.get(key) as V, value);
5858

5959
if (isUnchanged) {
6060
return this;

0 commit comments

Comments
 (0)