Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Editorial: Fix another IterableWeakMap leak #216

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 25 additions & 15 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -243,26 +243,36 @@ class IterableWeakMap {
set.delete(ref);
}

constructor(iterable) {
for (const [key, value] of iterable) {
this.set(key, value);
constructor(iterable = null) {
if (iterable !== null) {
for (const { 0: key, 1: value } of iterable) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if leaks are a concern, for..of is not an available option. Perhaps Array.from()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the constructor, Array.from is no different when it comes to iterables.


Also, if it’s invoked as:

new IterableWeakMap([[a, 1], [b, 2]])

then that’s no different from:

new WeakMap([[a, 1], [b, 2]])

which also performs for...of‑like iteration

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On arraylikes, Array.from doesn’t invoke the iterator protocol, so it’s actually robust for the common case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb
Array.from prefers using the iterator protocol: https://tc39.es/ecma262/#sec-array.from

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, you’re right, i was thinking only of a missing Symbol.iterator i suppose.

in that case there’s no pragmatic way to use a robust iteration mechanism in the readme (altho https://npmjs.com/iterate-value does exist) so this can be resolved.

this.set(key, value);
}
}
}
Comment on lines +246 to 252
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using object destructuring approximates the AddEntriesFromIterable abstract operation more closely.


set(key, value) {
const ref = new WeakRef(key);

this.#weakMap.set(key, { value, ref });
this.#refSet.add(ref);
this.#finalizationGroup.register(key, {
set: this.#refSet,
ref
}, ref);
const entry = this.#weakMap.get(key);
if (entry) {
entry.value = value;
} else {
const ref = new WeakRef(key);

this.#weakMap.set(key, { value, ref });
this.#refSet.add(ref);
this.#finalizationGroup.register(key, {
set: this.#refSet,
ref
}, ref);
}
}

get(key) {
const entry = this.#weakMap.get(key);
return entry && entry.value;
return this.#weakMap.get(key)?.value;
}

has(key) {
return this.#weakMap.has(key);
}

delete(key) {
Expand Down Expand Up @@ -291,13 +301,13 @@ class IterableWeakMap {
}

*keys() {
for (const [key, value] of this) {
for (const { 0: key } of this) {
yield key;
}
}

*values() {
for (const [key, value] of this) {
for (const { 1: value } of this) {
yield value;
}
}
Comment on lines 303 to 313
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using object destructuring doesn’t require an inner iteration of the key/value pair.

Expand Down