Fix and improve ReactiveSet, ReactiveMap and Trigger #593
Conversation
|
| @@ -50,6 +50,11 @@ export class TriggerCache<T> { | |||
| this.#map.get(key)?.$$(); | |||
| } | |||
|
|
|||
| dirtyAll() { | |||
There was a problem hiding this comment.
dirtyAll is needed to be able to clean all tracked keys. Otherwise the user has no access to keys and not able to clear them without own keys tracking.
This is was an issue for Set and Map implementation where we have iterated over currently defined keys not all tracked keys.
| for (const v of super.keys()) this.#triggers.dirty(v); | ||
| super.clear(); | ||
| this.#triggers.dirty($KEYS); | ||
| this.#triggers.dirtyAll(); |
There was a problem hiding this comment.
This is an important fix, to clear all tracked keys not only the ones currently defined.
| clear(): void { | ||
| if (super.size) { | ||
| super.clear(); |
There was a problem hiding this comment.
clearing, and all super operations should always happen before dirtying, otherwise the computations can have access to an old state.
| super.forEach(callbackfn as any); | ||
|
|
||
| for (const value of super.values()) { | ||
| this.#triggers.track(value); |
There was a problem hiding this comment.
While using forEach on Set we could track the used values to trigger a recomputation when a value gets updated.
| return super.has(v); | ||
|
|
||
| keys(): IterableIterator<T> { | ||
| return this.values(); |
There was a problem hiding this comment.
In case of Set for correctness keys are equal values, not the other way around.
| clear(): void { | ||
| if (super.size) { | ||
| super.clear(); |
There was a problem hiding this comment.
clear should happen before dirting in order to avoid old state in computations
| *entries(): IterableIterator<[K, V]> { | ||
| for (const entry of super.entries()) { | ||
| this.#keyTriggers.track(entry[0]); |
There was a problem hiding this comment.
entries should track keys as well
|
Thank you Alex, this is a still a draft PR though, I need to dig into it more to understand better potential issues with the new changes. |
| const hasKey = super.has(key); | ||
| const currentValue = super.get(key); | ||
| const result = super.set(key, value); |
There was a problem hiding this comment.
Why bring them here, whats wrong with them being inlined? Is this important or just a preference?
|
I will try to work on it this week, review, and cleanup all polishing parts for now. |
|
I will split this PR into smaller ones for easier review. Set PR just landed: #688 I will work on Map fixes, when times allow |
|
Map PR: #704 |
No description provided.