update map/set constructor to match TypeScript's... mostly #459
Conversation
🦋 Changeset detectedLatest commit: 52a00ca The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| #valueTriggers = new TriggerCache<K>(); | ||
|
|
||
| constructor(initial?: [K, V][]) { | ||
| constructor(initial?: Iterable<readonly [K, V]> | null) { |
There was a problem hiding this comment.
| #valueTriggers = new TriggerCache<K>(WeakMap); | ||
|
|
||
| constructor(initial?: [K, V][]) { | ||
| constructor(initial?: Iterable<readonly [K, V]> | null) { |
There was a problem hiding this comment.
c.f. https://github.com/microsoft/TypeScript/blob/main/src/lib/es2015.iterable.d.ts#LL147C40-L147C67
Note that TypeScript doesn't support undefined or null; I think this is a bug because the spec says:
- If iterable is either undefined or null, return map.
The TS PR making "Map constructor argument optional and nullable" didn't include WeakMap, which I believe is an oversight. We'll see if/when I get enough nerve to submit a PR to TypeScript 😅
There was a problem hiding this comment.
since we are doing a if (initial) check, we could allow any falsy type.
But we should probably limit to nullable if we decide to change the check
| #triggers = new TriggerCache<T | typeof $KEYS>(); | ||
|
|
||
| constructor(values?: readonly T[] | null) { | ||
| constructor(values?: Iterable<T> | null) { |
There was a problem hiding this comment.
c.f. https://github.com/microsoft/TypeScript/blob/main/src/lib/es2015.iterable.d.ts#LL189C21-L189C42
I'm not sure why this is readonly; looking at the gitblame for the above line in the TS repo seems to indicate it was never readonly. Commit where readonly was added.
There was a problem hiding this comment.
readonly was there only to allow passing readonly arrays, instead of forcing you to copy to satisfy ts.
with iterable it's not an issue.
| #triggers = new TriggerCache<T>(WeakMap); | ||
|
|
||
| constructor(values?: readonly T[] | null) { | ||
| constructor(values?: Iterable<T> | null) { |
There was a problem hiding this comment.
https://github.com/microsoft/TypeScript/blob/main/src/lib/es2015.iterable.d.ts#LL195C47-L195C60
The above comments also apply here - in particular the readonly comment and the fact that TS doesn't have WeakSetConstructor support null or undefined, which (again) doesn't conform to the spec:
- If iterable is either undefined or null, return set.
AlexErrant
changed the title
|
Thank you :) |
2 participants
There are two potential sources of type definitions:
I'm going with
iterablebecause it's more general.