-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Expunge the concept of well-known symbols from the checker #24738
Conversation
…ymbol tests work as expected
…hin the global symbol constructor
src/compiler/checker.ts
Outdated
const name = hasLateBindableName(decl) && resolveEntityName(decl.name.expression, SymbolFlags.Value); | ||
if (name && context.tracker.trackSymbol) { | ||
context.tracker.trackSymbol(name, saveEnclosingDeclaration, SymbolFlags.Value); | ||
if (hasLateBindableName(decl)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a copy of the primary change from #24668 - it's required to make the symbols get looked up correctly.
Huh. These changes seem pretty innocuous, but the build of |
e05c759
to
67cee76
Compare
@weswigham Ron's sourcemap rewrite should be going in soon so that will hopefully fix the OOM issues you're getting here |
A merge with master from two months ago shows that we'd already resolved the issue. CI on this PR is green, though it's probably out of date again. |
Also fixes #31253 |
This will also fix #36468 |
any update on this? |
@rbuckton do we have an appetite for looking into this again? It looks like a number of things are getting dupe'd to it. IIRC we held off on it long ago so |
FWIW, this PR would also probably fix immerjs/immer#710. |
I'd like to bring this back to our focus. I recently spoke to @benlesh about RxJS and how the library leans on So just to start, imagine an augmentation like interface SymbolConstructor {
observable: symbol;
} This is inspired by the existing declarations; however, the const symbolObservable: typeof Symbol.observable;
export default symbolObservable; that appears to any other code as just Trying to switch over to a unique symbol like so interface SymbolConstructor {
observable: unique symbol;
// ^^^^^^
} won't necessarily work because other libraries have also defined it as interface SymbolConstructor {
observable: symbol;
}
interface SymbolConstructor {
observable: unique symbol;
// ~~~~~~~~~~
// Subsequent property declarations must have the same type. Property 'observable' must be of type 'unique symbol', but here has type 'symbol'.
} |
I would like to note that:
Another point is that we support IE, which doesn't have |
#42543 Now supersedes this as an updated version for 2021 - large parts are the same (and the result certainly is), but some of the implementation is different. |
Interestingly, there is some fallout here in some odd places:
any
produces anumber
index signature. With this change, we make astring
index signature instead (it's more broad and closer to correct).Symbol.iterator
when it appeared as a property name (and assume that the expression had no sideffects), now we retain and emit it (and potentially cache it), as we generally do not assume property access expressions are safe to elide or copy.Fixes #24622
An important implementation note: As we discussed in person, Inside the checker, if we see members of the global
SymbolConstructor
of typesymbol
, we also assume you meant to sayunique symbol
. In this way, we retain compataibility with older libs or definition files (even as they update), which is how this can build (at all), given node is shimmingSymbol.iterator
as asymbol
.