-
Notifications
You must be signed in to change notification settings - Fork 439
fix(engine-core): dedupe additions to WeakMultimap @W-21387024
#5738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,50 +7,17 @@ | |
|
|
||
| import { ArrayPush, ArraySplice, isUndefined } from '@lwc/shared'; | ||
|
|
||
| const supportsWeakRefs = | ||
| typeof WeakRef === 'function' && typeof FinalizationRegistry === 'function'; | ||
|
|
||
| /** | ||
| * A map where the keys are weakly held and the values are a Set that are also each weakly held. | ||
| * The goal is to avoid leaking the values, which is what would happen with a WeakMap<K, Set<V>>. | ||
| * | ||
| * Note that this is currently only intended to be used in dev/PRODDEBUG environments. | ||
| * It leaks in legacy browsers, which may be undesired. | ||
| * | ||
| * This implementation relies on the WeakRef/FinalizationRegistry proposal. | ||
| * For some background, see: https://github.com/tc39/proposal-weakrefs | ||
| */ | ||
| export interface WeakMultiMap<T extends object, V extends object> { | ||
| get(key: T): ReadonlySet<V>; | ||
| add(key: T, vm: V): void; | ||
| delete(key: T): void; | ||
| } | ||
|
|
||
| // In browsers that doesn't support WeakRefs, the values will still leak, but at least the keys won't | ||
| class LegacyWeakMultiMap<K extends object, V extends object> implements WeakMultiMap<K, V> { | ||
| private _map: WeakMap<K, Set<V>> = new WeakMap(); | ||
|
|
||
| private _getValues(key: K): Set<V> { | ||
| let values = this._map.get(key); | ||
| if (isUndefined(values)) { | ||
| values = new Set(); | ||
| this._map.set(key, values); | ||
| } | ||
| return values; | ||
| } | ||
|
|
||
| get(key: K): ReadonlySet<V> { | ||
| return this._getValues(key); | ||
| } | ||
| add(key: K, vm: V) { | ||
| const set = this._getValues(key); | ||
| set.add(vm); | ||
| } | ||
| delete(key: K) { | ||
| this._map.delete(key); | ||
| } | ||
| } | ||
|
|
||
| // This implementation relies on the WeakRef/FinalizationRegistry proposal. | ||
| // For some background, see: https://github.com/tc39/proposal-weakrefs | ||
| class ModernWeakMultiMap<K extends object, V extends object> implements WeakMultiMap<K, V> { | ||
| export class WeakMultimap<K extends object, V extends object> { | ||
| private _map = new WeakMap<K, WeakRef<V>[]>(); | ||
|
|
||
| private _registry = new FinalizationRegistry((weakRefs: WeakRef<V>[]) => { | ||
|
|
@@ -88,8 +55,12 @@ class ModernWeakMultiMap<K extends object, V extends object> implements WeakMult | |
| } | ||
| add(key: K, value: V) { | ||
| const weakRefs = this._getWeakRefs(key); | ||
| // We could check for duplicate values here, but it doesn't seem worth it. | ||
| // We transform the output into a Set anyway | ||
| // Skip adding if already present | ||
| for (const weakRef of weakRefs) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we update the contract to:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's kind of confusing because and the According to this article, addition and iteration are comparable for sets and arrays, but sets have faster deletion. I made the switch. |
||
| if (weakRef.deref() === value) { | ||
| return; | ||
| } | ||
| } | ||
| ArrayPush.call(weakRefs, new WeakRef<V>(value)); | ||
|
|
||
| // It's important here not to leak the second argument, which is the "held value." The FinalizationRegistry | ||
|
|
@@ -105,5 +76,3 @@ class ModernWeakMultiMap<K extends object, V extends object> implements WeakMult | |
| this._map.delete(key); | ||
| } | ||
| } | ||
|
|
||
| export const WeakMultiMap = supportsWeakRefs ? ModernWeakMultiMap : LegacyWeakMultiMap; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a stage 4, should be safe to point to mdn ?