Skip to content

Type definitions for 'Symbols as WeakMap keys' #52534

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
acutmore opened this issue Jan 31, 2023 · 8 comments · Fixed by #54195
Closed

Type definitions for 'Symbols as WeakMap keys' #52534

acutmore opened this issue Jan 31, 2023 · 8 comments · Fixed by #54195
Assignees
Labels
Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". Fix Available A PR has been opened for this issue Help Wanted You can do this

Comments

@acutmore
Copy link
Contributor

lib Update Request for 'Symbols as WeakMap keys'

Configuration Check

My compilation target is ESNext and my lib is the default.

Missing / Incorrect Definition

Symbols as WeakMap keys went to stage 4 at TC39 yesterday.
The types for WeakMap, WeakSet, WeakRef and FinalizationRegistry can be updated to reflect that symbol is now also allowed alongside the existing object constraint.

Sample Code

new WeakMap<symbol, any>();
new WeakSet<symbol>();
new WeakRef(Symbol());
new FinalizationRegistry(() => {}).register(Symbol());

Documentation Link

https://github.com/tc39/proposal-symbols-as-weakmap-keys

@acutmore
Copy link
Contributor Author

acutmore commented Feb 1, 2023

If 'contributions are welcome', there are developers at Bloomberg ready and standing by to contribute a PR.

@DanielRosenwasser DanielRosenwasser added Help Wanted You can do this Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". labels Feb 1, 2023
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 5.0.1 milestone Feb 1, 2023
@acutmore
Copy link
Contributor Author

acutmore commented Feb 9, 2023

@leoelm from @bloomberg has begun looking into the changes required for this.

@rbuckton
Copy link
Member

rbuckton commented Feb 9, 2023

We discussed this in a design meeting recently. Even though the proposal makes an exception for registered symbols from Symbol.for, we don't want to take on the complexity to model that exception within the TypeScript type system. That means that our expectation is that we just expand the key constraint for WeakMap et al. to object | symbol, even if that means you can potentially write failing code like new WeakMap().set(Symbol.for("foo"), {}).

@leoelm
Copy link
Contributor

leoelm commented Feb 13, 2023

Hello all,

We have identified a complexity in regards to adding type support for the primitive symbol to WeakRef, WeakSet, and WeakMap. We also have a potential solution but wanted to get the TypeScript team’s opinion.

The problem originates from the change being to the constraint of a generic, which is not something interface merging supports. Illustrated at the example of WeakSet:

ES2015 :

interface WeakSet<T extends object> {
    ...
}

ES2023:

interface WeakSet<T extends object | symbol> {
    ...
}
// Error! All declarations of 'WeakSet' must have identical type parameters.(2428)

The above addition in ES2023 would now lead to an invalid redefinition of the interface WeakSet as it does not use the same template parameter type for T.

Proposed Solution

Introduce a custom defined type to ES2015 based on an interface that can be augmented to include symbol alongside object. e.g.

ES2015:

/**
 * This type is used to 'store' the types that can be used with Weak{Map,Set,Ref}
 * es2022
 **/
interface AllowableWeakTypesStore {
  object: object;
}

type AllowableWeakTypes = AllowableWeakTypesStore[keyof AllowableWeakTypesStore];

interface WeakSet<T extends (AllowableWeakTypes & {})> {
  ...
}

ES2023:

interface AllowableWeakTypesStore {
  symbol: symbol;
}

Alternative Solution (not favoured)

lib.es2023.d.ts would not directly reference lib.es2022.d.ts. Instead it would individually select the definitions from ES2015 up to ES2022 but avoid the files which define WeakSet, WeakMap, and WeakRef. This would allow ES2023 to define the type with a new constraint.

/// <reference no-default-lib="true"/>
/// <reference lib="es2022.exceptweak" />

interface WeakRef<T extends object | symbol> {
...
}

We discarded this since it requires duplicating the definitions for WeakSet, WeakMap, and WeakRef, and forks the library inheritance tree adding complexity and risk.

Any thoughts or alternative approaches are greatly appreciated! Thank you in advance!

@rbuckton
Copy link
Member

The solution we use for ArrayBuffer | SharedArrayBuffer in typed array constructors is the same as your proposed solution with AllowableWeakTypesStore, so I would recommend that approach. I'd recommend a simpler name like WeakKeyTypes, though.

@graphemecluster
Copy link
Contributor

@rbuckton Somehow off topic, but I would like to see ArrayLike<T> | Iterable<T> using the same approach too. Currently many definitions in es5.d.ts are repeated in es2015.iterable.d.ts.

@rbuckton
Copy link
Member

rbuckton commented May 3, 2023

@acutmore, @leoelm: Is bloomberg#76 ready for a PR? It looks pretty much like what I would have written given my earlier suggestion.

@acutmore
Copy link
Contributor Author

acutmore commented May 4, 2023

Hi @rbuckton, thanks for taking a look at bloomberg#76. We've merged that now. I'll sync up with @leoelm and hopefully we'll get the PR to Microsoft open tomorrow.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Effort: Casual Good issue if you're already used to contributing to the codebase. Harder than "good first issue". Fix Available A PR has been opened for this issue Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants