Skip to content

Use unique symbol for well-known symbols #21603

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

Closed
falsandtru opened this issue Feb 3, 2018 · 12 comments · Fixed by #42543
Closed

Use unique symbol for well-known symbols #21603

falsandtru opened this issue Feb 3, 2018 · 12 comments · Fixed by #42543
Labels
Revisit An issue worth coming back to Suggestion An idea for TypeScript

Comments

@falsandtru
Copy link
Contributor

Expected behavior:

interface SymbolConstructor {
    readonly iterator: unique symbol;
}

Actual behavior:

interface SymbolConstructor {
    readonly iterator: symbol;
}
@RyanCavanaugh
Copy link
Member

@rbuckton thoughts?

@falsandtru
Copy link
Contributor Author

If we do this update, we need a fix like #20018.

@rbuckton
Copy link
Member

rbuckton commented Feb 5, 2018

We can't fix this in the core library because of NodeJS, Core-JS, ES6-Shim, etc. also defining these as symbol. I hope to get "lib" references in soon so that we can change NodeJS et. al. to reference the definitions in lib.es2016.symbol.wellknown.d.ts and then make this change.

@mhegazy mhegazy added the Suggestion An idea for TypeScript label Feb 6, 2018
@RyanCavanaugh
Copy link
Member

@rbuckton lib references are in, what should we do here?

@rbuckton
Copy link
Member

We unfortunately run into collisions in type definitions for packages like NodeJS that forward-declare some symbols. #26568 would help us to get to the point where the NodeJS type definitions could use /// <reference lib="..." /> without breaking in older versions of the compiler.

@weswigham
Copy link
Member

#24738 fixes this gracefully with older versions of the node.d.ts by just assuming that if you wrote symbol under the global SymbolConstructor you meant to say unique symbol.

@rbuckton
Copy link
Member

#24738 fixes this gracefully with older versions of the node.d.ts by just assuming that if you wrote symbol under the global SymbolConstructor you meant to say unique symbol.

I know we considered this, but it was more of a workaround. We need something like #26568 for other reasons and its more maintainable going forward.

@weswigham
Copy link
Member

weswigham commented Aug 23, 2018

That won't really help the community upgrade as much, though - they'll be forced to try and update their entire type heirarchy to depending on the latest version of all the declaration files. The workaround has the advantage that older declaration files continue to work (and continue to work with older versions of TS) and can be updated as needed.

@rbuckton
Copy link
Member

they'll be forced to try and update their entire type heirarchy to depending on the latest version of all the declaration files.

This is usually the case when types are updated in a dependency. On definitely typed there are only a handful of packages that introduce this kind collision, which we can address easily enough once #26568 has landed in a development build. I'm not saying that the symbol to unique symbol workaround might not still be needed, but its possible it won't be.

@RyanCavanaugh RyanCavanaugh added the Revisit An issue worth coming back to label Aug 23, 2018
@ntninja
Copy link

ntninja commented Sep 13, 2018

Sorry for being a bother, but now that #26568 has been merged, is it possible to revisit this for TS 3.1?

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Jan 3, 2020

#24738 would fix this in a manner that’s compatible with old .d.ts files.

@falsandtru falsandtru changed the title Use unique symbol for wellknown symbols Use unique symbol for well-known symbols Jan 3, 2020
@mmis1000
Copy link

mmis1000 commented Jan 15, 2021

Looks like this is a blocker to define correct Array::concat signature.

The Array::concat decides whether they should spread the argument based on Symbol.isConcatSpreadable

But Symbol.isConcatSpreadable is a symbol currently.

Note:
The incorrect code below passed the type check.

const a: number[][] = [[0]]
const b: number[][] = a.concat([0])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Revisit An issue worth coming back to Suggestion An idea for TypeScript
Projects
None yet
8 participants