[BUGFIX] Fix trackedMap and trackedWeakMap reactivity for existing keys - #21128
Merged
NullVoxPopuli merged 3 commits intoMar 4, 2026
Merged
Conversation
megothss
force-pushed
the
fix/tracked-map-set-reactivity
branch
from
February 27, 2026 17:35
1514309 to
2ceccbd
Compare
megothss
changed the title
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| return { value: [currentKey, self.get(currentKey!)], done: false }; | ||
| }, | ||
| [Symbol.iterator]() { |
| consumeTag(this.#collection); | ||
|
|
||
| this.#vals.forEach(fn); | ||
| for (let [key, value] of this) { |
Contributor
There was a problem hiding this comment.
does the collection tag not need to be entangled here anymore? I see it's not in entries or the symbol iterator either 🤔
Contributor
Author
There was a problem hiding this comment.
The collection tag is still entangled!
It's consumed transitively:
forEachusesfor...of this[Symbol.iterator]()this.keys()consumeTag(this.#collection)
It's the same for entries() and values().
I've updated the JSDoc on [Symbol.iterator] to make this clearer.
Contributor
|
legend. I'm optimistic we can get CI fixed today, so we can have a bit more confidence in the fixes and actually get them merged <3 |
Contributor
Estimated Asset SizesDiff --- main/out.txt 2026-03-03 17:28:20.000000000 +0000
+++ pr/./pr-22633602623/out.txt 2026-03-03 17:38:00.000000000 +0000
@@ -1,34 +1,34 @@
-╔═══════╤═══════════╤════════╗
-║ │ Min │ Gzip ║
-╟───────┼───────────┼────────╢
-║ Total │ 352.16 KB │ 204 KB ║
-╚═══════╧═══════════╧════════╝
+╔═══════╤═══════════╤═══════════╗
+║ │ Min │ Gzip ║
+╟───────┼───────────┼───────────╢
+║ Total │ 352.09 KB │ 204.08 KB ║
+╚═══════╧═══════════╧═══════════╝
╔══════════════════════╤═══════════╤═══════════╗
║ @ember/* │ Min │ Gzip ║
╟──────────────────────┼───────────┼───────────╢
-║ Total │ 313.39 KB │ 181.94 KB ║
+║ Total │ 313.39 KB │ 181.91 KB ║
╟──────────────────────┼───────────┼───────────╢
-║ -internals │ 36.65 KB │ 26.21 KB ║
-║ application │ 13.23 KB │ 8.09 KB ║
+║ -internals │ 36.65 KB │ 26.22 KB ║
+║ application │ 13.23 KB │ 8.05 KB ║
║ array │ 13.01 KB │ 7.46 KB ║
║ canary-features │ 304 B │ 389 B ║
-║ component │ 2.05 KB │ 1.59 KB ║
+║ component │ 2.05 KB │ 1.64 KB ║
║ controller │ 1.96 KB │ 1.41 KB ║
║ debug │ 11.69 KB │ 8.12 KB ║
║ deprecated-features │ 31 B │ 77 B ║
║ destroyable │ 561 B │ 383 B ║
║ enumerable │ 259 B │ 387 B ║
-║ helper │ 1.08 KB │ 804 B ║
+║ helper │ 1.08 KB │ 811 B ║
║ instrumentation │ 2.43 KB │ 1.79 KB ║
-║ modifier │ 1.22 KB │ 995 B ║
+║ modifier │ 1.22 KB │ 965 B ║
║ object │ 35.94 KB │ 22.16 KB ║
║ owner │ 159 B │ 178 B ║
-║ renderer │ 630 B │ 499 B ║
-║ routing │ 59.3 KB │ 34.13 KB ║
+║ renderer │ 630 B │ 487 B ║
+║ routing │ 59.3 KB │ 34.12 KB ║
║ runloop │ 2.36 KB │ 1.5 KB ║
║ service │ 1 KB │ 845 B ║
-║ template │ 654 B │ 523 B ║
+║ template │ 654 B │ 541 B ║
║ template-compilation │ 429 B │ 366 B ║
║ template-compiler │ 123.08 KB │ 59.45 KB ║
║ template-factory │ 370 B │ 374 B ║
@@ -40,9 +40,9 @@
╔═════════════════╤══════════╤══════════╗
║ @glimmer/* │ Min │ Gzip ║
╟─────────────────┼──────────┼──────────╢
-║ Total │ 38.77 KB │ 22.06 KB ║
+║ Total │ 38.71 KB │ 22.17 KB ║
╟─────────────────┼──────────┼──────────╢
-║ destroyable │ 2.95 KB │ 1.52 KB ║
+║ destroyable │ 2.77 KB │ 1.39 KB ║
║ encoder │ 81 B │ 171 B ║
║ env │ 38 B │ 87 B ║
║ global-context │ 886 B │ 545 B ║
@@ -55,7 +55,7 @@
║ runtime │ 10.32 KB │ 5.32 KB ║
║ tracking │ 1.34 KB │ 1.16 KB ║
║ util │ 1.94 KB │ 1.68 KB ║
-║ validator │ 15.75 KB │ 6.96 KB ║
+║ validator │ 15.86 KB │ 7.19 KB ║
║ vm │ 495 B │ 569 B ║
║ wire-format │ 1.84 KB │ 1.35 KB ║
╚═════════════════╧══════════╧══════════╝Details
|
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 emberjs#21123
Rename `existing` → `hasExisting` in set() and clarify iterator tag entanglement in JSDoc
NullVoxPopuli
force-pushed
the
fix/tracked-map-set-reactivity
branch
from
March 3, 2026 16:54
cf9075d to
ab7ceb8
Compare
Contributor
|
GH is missing a commit 🤔 edit: finally! it showed up |
NullVoxPopuli
approved these changes
Mar 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TrackedMap.set()andTrackedWeakMap.set()use value truthiness (if (existing)) instead ofthis.#vals.has(key)to check for existing keys, causing two bugs:.values(),.entries(),.forEach()) miss value updates0,false,null,""): The equality check is skipped entirely, and the collection tag is dirtied unnecessarilyAdditionally, TrackedMap's
.entries(),.values(), and.forEach()only consume the collection tag, so they miss per-key value updates even with a correctset(). This is in contrast to[Symbol.iterator]()which correctly consumes per-key storage tags viaself.get().Changes
TrackedMap:
set(): Replace value truthiness checks withthis.#vals.has(key)entries(),values(),forEach(): Delegate to[Symbol.iterator]()which already correctly consumes both the collection tag and per-key storage tags[Symbol.iterator](): Add[Symbol.iterator]()to the returned object so it satisfies the iterable protocol (needed byentries()andfor...ofconsumers)values,entries,forEachwith existing key updates, and a test for setting existing falsy valuesTrackedWeakMap:
set(): Same truthiness bug fix — replacethis.#vals.get(key)withthis.#vals.has(key)Fixes #21123