Return mapped locations in alternate fields#24074
Conversation
weswigham
left a comment
There was a problem hiding this comment.
Small comments, maybe one change to the original that probably needs to happen, given that maps can be chained into one another.
| if (def.originalFileName) { | ||
| Debug.assert(def.originalTextSpan !== undefined, "originalTextSpan should be present if originalFileName is"); | ||
| return { | ||
| ...<any>def, |
There was a problem hiding this comment.
Lemme guess: excess property warning?
There was a problem hiding this comment.
"Spread types may only be created from object types" (even if T is constrained to DocumentSpan & object!)
There was a problem hiding this comment.
This also requires any rather than a different cast because we don't believe that the returned object literal actually conforms to T
There was a problem hiding this comment.
So it's a symptom of how we don't have higher order spread types. :3
|
|
||
| /** | ||
| * If the span represents a location that was remapped (e.g. via a .d.ts.map file), | ||
| * then the original filename and span will be specified here |
There was a problem hiding this comment.
Is it worth noting that it may have been mapped multiple times and that (from what I can tell) this isnt currently the original-original, but rather just one step back from the final mapping?
There was a problem hiding this comment.
I'll update the code to return the true original
| function makeGetTargetOfMappedPosition<TIn>( | ||
| extract: (original: TIn) => sourcemaps.SourceMappableLocation, | ||
| create: (result: sourcemaps.SourceMappableLocation, original: TIn) => TIn | ||
| create: (result: sourcemaps.SourceMappableLocation, original: TIn, firstOriginal: TIn) => TIn |
There was a problem hiding this comment.
original: TIn, firstOriginal: TIn [](start = 64, length = 33)
As I suggested offline, I think unmapped and original would be clearer.
| return definitions.map(def => this.toFileSpan(def.fileName, def.textSpan, project)); | ||
| } | ||
|
|
||
| private static mapToOriginalLocation<T extends DocumentSpan>(def: T): T { |
There was a problem hiding this comment.
mapToOriginalLocation [](start = 23, length = 21)
This name isn't very descriptive but I can't think of one that would be. I'd suggest putting a simplified version of your commit message in the doc comment for this function.
|
Travis Node 6 failure seems to be unrelated |
When we map a
.d.tslocation to.ts, Visual Studio gets confused because there's no associated RoslynDocumentin the same project which corresponds to the file. VS Code has no problem with this, and luckily we have two protocols.This retains the existing behavior for the "simplified" (VS Code) protocol but stores the
.d.tslocation in a set of additional fields, and does the reverse for VS (store the.d.tslocation where it used to be and stores the.tslocation in the additional fields).This gives both editors maximum flexibility in terms of how they treat go-to-def requests which land in sourcemapped .d.ts files.
I have a separate PR on the managed side to consume these fields when they are present.