Add source mappings for serialized properties with available declaration #60005
Conversation
typescript-bot
added
the
For Uncommitted Bug
typescript-bot
added
For Backlog Bug
src/testRunner/unittests/tsserver/projectReferencesSourcemap.ts
Outdated
Show resolved
Hide resolved
src/compiler/checker.ts
Outdated
| const propertyName = getPropertyNameNodeForSymbol(propertySymbol, context); | ||
| if (propertyDeclaration && (isPropertyAssignment(propertyDeclaration) || isShorthandPropertyAssignment(propertyDeclaration) || isMethodDeclaration(propertyDeclaration) || isMethodSignature(propertyDeclaration) || isPropertySignature(propertyDeclaration) || isPropertyDeclaration(propertyDeclaration) || isGetOrSetAccessorDeclaration(propertyDeclaration))) { | ||
| setSourceMapRange(propertyName, propertyDeclaration.name); |
There was a problem hiding this comment.
I still feel at least a little uncomfortable with this; why isn't it declaration emit which can do this? The node builder produces lots of nodes, why doesn't this happen elsewhere?
There was a problem hiding this comment.
Also, can this just be done in getPropertyNameNodeForSymbol?
(In any case, this PR is safe, since getPropertyNameNodeForSymbol is returning a new node.)
There was a problem hiding this comment.
I still feel at least a little uncomfortable with this; why isn't it declaration emit which can do this?
the required information is on the symbol, emit works based on the produced nodes and we need to pass somehow the information that it needs from here as this is the place where this information is being lost. This just leaves a breadcrumb that can be picked up by the emitter
Also, can this just be done in getPropertyNameNodeForSymbol?
maybe, im not sure sure ;p I'll try to analyze this
src/compiler/checker.ts
Outdated
| const propertyName = getPropertyNameNodeForSymbol(propertySymbol, context); | ||
| if (propertyDeclaration && (isPropertyAssignment(propertyDeclaration) || isShorthandPropertyAssignment(propertyDeclaration) || isMethodDeclaration(propertyDeclaration) || isMethodSignature(propertyDeclaration) || isPropertySignature(propertyDeclaration) || isPropertyDeclaration(propertyDeclaration) || isGetOrSetAccessorDeclaration(propertyDeclaration))) { | ||
| setSourceMapRange(propertyName, propertyDeclaration.name); |
There was a problem hiding this comment.
For declaration emit nodes, just use setTextRange rather than setSourceMapRange - there's a helper in scope that takes care of a bunch of sanity checking for you that's absent here. But yeah, you should also be able to move this into getPropertyNameNodeForSymbol so it happens for all callers.
There was a problem hiding this comment.
Using setTextRange doesn't cut it and the results are as on the main branch. Am I using it wrong or missing something?
I have moved the fix to getPropertyNameNodeForSymbol though
There was a problem hiding this comment.
Yeah, it doesn't seem to help. I don't understand why this isn't working:
propertyNameNode = setTextRange(context, propertyNameNode, declaration.name);So I would expect the original node stuff to "just work". setTextRange doesn't call setSourceMapRange at all, though, so...
There was a problem hiding this comment.
If you stick setSourceMapRange(range, location); into setTextRange after setOriginalNode, that actually also changes a whole bunch of other stuff kinda for the better? But then breaks some other JSDoc related code (which I think was another recent bugfix that may conflict?) Dunno if that's a good idea or not, I don't have a good mental model of this.
fixes #60004