-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Remove originalKeywordKind
from Identifier
s
#51497
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
Changes from all commits
045ca9f
6ba90be
b9a4758
716e136
ac172de
952715c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1677,7 +1677,6 @@ export interface Identifier extends PrimaryExpression, Declaration, JSDocContain | |
* Text of identifier, but if the identifier begins with two underscores, this will begin with three. | ||
*/ | ||
readonly escapedText: __String; | ||
readonly originalKeywordKind?: SyntaxKind; // Original syntaxKind which get set so that we can report an error later | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My only concern with this is that it is a public API breaking change. Do we expose any other way to get this information? If we find that there are consumers in the field that need this information, we could potentially expose it via a getter in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could try to keep it as a public but deprecated property. I do see some usage in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a best practice to add deprecated accessors to prototypes right now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also https://sourcegraph.com/search?q=context:global+originalKeywordKind+-repo:microsoft/TypeScript+-file:%5C.d%5C.ts+lang:typescript+-file:src/compiler+-file:src/services&patternType=standard&sm=1 which sorts a little better; it is being used by eslint, angular, a few other big projects. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no prototype to add it to, since everything inherits from The current patching mechanism was primarily to handle patching the factory API itself, and only the one that is reachable during transformations. As a result, the current patching mechanism doesn't touch the factory used internally by the parser, which is the one you'd want most if you want to patch node instances returned by the API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does seem like a win in terms of memory usage. If we really want to avoid the break, #51498 gives us both. We should still deprecate it either way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I didn't notice the memory improvement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I misspoke. we could potentially add it to the prototype of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, honestly, maybe it won't be that slow and sticking it on Identifier is enough. (I was hoping caching it somehow would help but that might be slower in general...) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'd have to test whether defining an accessor on |
||
/** @internal */ readonly autoGenerateFlags?: GeneratedIdentifierFlags; // Specifies whether to auto-generate the text for an identifier. | ||
/** @internal */ readonly autoGenerateId?: number; // Ensures unique generated identifiers get unique names, but clones get the same name. | ||
/** @internal */ readonly autoGeneratePrefix?: string | GeneratedNamePart; | ||
|
@@ -1686,6 +1685,9 @@ export interface Identifier extends PrimaryExpression, Declaration, JSDocContain | |
isInJSDocNamespace?: boolean; // if the node is a member in a JSDoc namespace | ||
/** @internal */ typeArguments?: NodeArray<TypeNode | TypeParameterDeclaration>; // Only defined on synthesized nodes. Though not syntactically valid, used in emitting diagnostics, quickinfo, and signature help. | ||
/** @internal */ jsdocDotPos?: number; // Identifier occurs in JSDoc-style generic: Id.<T> | ||
/** | ||
* @deprecated `originalKeywordKind` will be removed in the future. | ||
*/ readonly originalKeywordKind?: SyntaxKind; | ||
/**@internal*/ hasExtendedUnicodeEscape?: boolean; | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.