Add 'name' property to Identifier#17329
Conversation
|
This is pretty awkward when so many places that reference identifiers are named myFunc.name.name |
|
Any alternative suggestions? |
|
You could potentially just specialize |
|
If we're trying to get some API compatibility back, why not rename identifier.text to identifier.escapedText, then make identifier.text be the unescaped version? It means our consumers could still have hidden underscore related costs; but we still won't. Should probably also make escapeIdentifier and unescapeIdentifier identity functions if we do this (since old code will be passing |
|
@weswigham Implemented that suggestion. Had to fix a few invalid casts along the way, since previously |
There was a problem hiding this comment.
If we want to prevent our pre 2.5 API consumers from suddenly having bugs relating to over-unescaping things, we should also likely make escapeIdentifier and unescapeIdentifier equal to x => x. But aside from that, I think this keeps us good internally while keeping out API very backcompat. Probably want someone else to glance at this, but I like it.
|
|
||
| get unescapedText(): string { | ||
| get text(): string { | ||
| return unescapeLeadingUnderscores(this.escapedText); |
There was a problem hiding this comment.
Can we cache this since the node is supposed to be immutable? Potentially return (delete this.text, (this as any).text = unescapeLeadingUnderscores(this.escapedText)) or using an internal field. Not sure which is better.
There was a problem hiding this comment.
Could that wait for another PR? I would want to see the performance difference between testing for _ every time we call .text vs adding an extra field to every Identifier.
There was a problem hiding this comment.
delete this.text would be the worst you could do. V8 will deoptimize the whole object and your optimization becomes a deoptimization.
If you really need to optimize here (I doubt that), you can use Object.definePropery to override the getter with the computed value
|
@Andy-MS Re: PR to tsutils - that PR will still function as intended (across all TS versions!) if we make |
id.namewill be more conventent than callingts.unescapeLeadingUnderscores(id.text).