Editorial: Fix another IterableWeakMap leak
#216
Conversation
| constructor(iterable = null) { | ||
| if (iterable !== null) { | ||
| for (const { 0: key, 1: value } of iterable) { | ||
| this.set(key, value); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Using object destructuring approximates the AddEntriesFromIterable abstract operation more closely.
| *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; | ||
| } | ||
| } |
There was a problem hiding this comment.
Using object destructuring doesn’t require an inner iteration of the key/value pair.
| this.set(key, value); | ||
| constructor(iterable = null) { | ||
| if (iterable !== null) { | ||
| for (const { 0: key, 1: value } of iterable) { |
There was a problem hiding this comment.
if leaks are a concern, for..of is not an available option. Perhaps Array.from()?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
On arraylikes, Array.from doesn’t invoke the iterator protocol, so it’s actually robust for the common case.
There was a problem hiding this comment.
@ljharb
Array.from prefers using the iterator protocol: https://tc39.es/ecma262/#sec-array.from
There was a problem hiding this comment.
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.
Fixes: #213
This would cause the
#refSetto possibly contain dead references and lead to a key/value pair being yielded multiple times.It also applies some fixes so that
new IterableWeakMap()doesn’t throw aTypeError, becauseundefinedis not iterable and makesIterableWeakMap.length === WeakMap.length.