Declaration maps and transparent goto definition using them #22658
Conversation
|
Oh, and @mjbvz do you have some concrete desires for this from the vscode perspective? I'd also like to use these maps to enable going to the JS associated with a .d.ts declaration, too, but that'll require a new LS command, I believe (and so I think will be left off of this PR). |
|
Notes from offline discussion
Yes - keep going until we hit a non-mappable intermediate
No - rename can just
No - same as above
Report an error to the ts server log so we can diagnose by hand
No
Eventually
Support emit+read sections in a future PR |
src/services/services.ts
Outdated
| if (file.sourceMapper) { | ||
| return file.sourceMapper; | ||
| } | ||
| // TODO (wewigham): Read sourcemappingurl from last line of .d.ts if present |
|
On the vscode side, most of the requests have been to make |
|
@mjbvz Actually, it makes "Go to Definition" go to the original TS instead of the declaration file. |
|
Ok, I think that makes sense. Is the idea that |
|
@mjbvz As is, this only affects |
|
@mjbvz I've enabled mappings for |
src/compiler/emitter.ts
Outdated
| None, | ||
| File, | ||
| Inline, | ||
| DeclarationFile |
There was a problem hiding this comment.
Do we want to consider allowing inline declaration maps? The current behavior seems inconsistent because --inlineSourceMap seems to affect declaration maps in a different way than it affects --sourceMap in that we end up emitting both the inline comment and a separate map file. I'd prefer that we chose one of the two following behaviors:
--inlineSourceMapshould have no impact on--declarationMaps, as we can introduce an--inlineDeclarationMapin the future if necessary.--inlineSourceMapdoes affect--declarationMapsand we do not write a separate map file when set.
Barring feedback from other reviewers, I'd lean towards the former than the latter.
src/compiler/emitter.ts
Outdated
| const bundle = sourceFileOrBundle.kind === SyntaxKind.Bundle ? sourc 8000 eFileOrBundle : undefined; | ||
| const sourceFile = sourceFileOrBundle.kind === SyntaxKind.SourceFile ? sourceFileOrBundle : undefined; | ||
| const sourceFiles = bundle ? bundle.sourceFiles : [sourceFile]; | ||
| sourceMap.initialize(jsFilePath, sourceMapFilePath, sourceFileOrBundle); | ||
| if (sourcemapKind !== SourceMapEmitKind.None) { |
There was a problem hiding this comment.
In sourcemap.ts we still set the initial state of disabled to !(compilerOptions.sourceMap || compilerOptions.inlineSourceMap). This seems unnecessary if we are conditionally enabling/disabling the state here.
src/compiler/utilities.ts
Outdated
| @@ -2819,7 +2819,8 @@ namespace ts { | |||
| export interface EmitFileNames { | |||
| jsFilePath: string; | |||
| sourceMapFilePath: string; | |||
There was a problem hiding this comment.
Should this also be string | undefined?
src/compiler/commandLineParser.ts
Outdated
| @@ -194,6 +194,13 @@ namespace ts { | |||
| category: Diagnostics.Basic_Options, | |||
| description: Diagnostics.Generates_corresponding_d_ts_file, | |||
| }, | |||
| { | |||
| name: "declarationMaps", | |||
There was a problem hiding this comment.
Why is --declarationMaps plural when --sourceMap is singular?
src/compiler/program.ts
Outdated
| @@ -2111,7 +2111,7 @@ namespace ts { | |||
| createDiagnosticForOptionName(Diagnostics.Option_0_cannot_be_specified_with_option_1, "out", "outFile"); | |||
| } | |||
|
|
|||
| if (options.mapRoot && !options.sourceMap) { | |||
| if (options.mapRoot && !(options.sourceMap || options.declarationMaps)) { | |||
There was a problem hiding this comment.
Though the condition has changed, the related diagnostic still only says "sourceMap".
src/compiler/program.ts
Outdated
| @@ -2125,6 +2125,12 @@ namespace ts { | |||
| } | |||
| } | |||
|
|
|||
| if (options.declarationMaps) { | |||
There was a problem hiding this comment.
Unnecessary nested if, I would just merge the conditions.
src/services/services.ts
Outdated
| } | ||
|
|
||
| /* @internal */ | ||
| export function getSourceFileLikeCache(host: { readFile?: (path: string) => string, fileExists?: (path: string) => boolean }): SourceFileLikeCache { |
There was a problem hiding this comment.
nit: createSourceFileLikeCache seems more appropriate.
src/services/services.ts
Outdated
| if (file.sourceMapper) { | ||
| return file.sourceMapper; | ||
| } | ||
| // TODO (weswigham): Read sourcemappingurl from last line of .d.ts if present |
There was a problem hiding this comment.
I would not recommend leaving this part unfinished. On the upside, if implemented its fairly easy to support inline declaration maps.
I would recommend you scan a file backwards until you encounter a newline. If that substring is not a sourceMapURL comment, continue with the preceding lines until you encounter a sourceMapURL comment. If you encounter a non-comment, non-whitespace line you break. If no match, you can attempt to look for a ".map" file in the same folder.
src/services/sourcemaps.ts
Outdated
|
|
||
| function decodeSingleSpan<T>(state: DecoderState<T>): void { | ||
| while (state.decodingIndex < state.encodedText.length) { | ||
| const char = state.encodedText.charAt(state.decodingIndex); |
There was a problem hiding this comment.
I would recommend using .charCodeAt and CharacterCodes
src/services/sourcemaps.ts
Outdated
| return decodedMappings || (decodedMappings = calculateDecodedMappings()); | ||
| } | ||
|
|
||
| function getReverseSortedMappings() { |
There was a problem hiding this comment.
The names reverseSortedMappings and forwardSortedMappings aren't very clear. I'd prefer something more like sourceOrderedMappings and generatedOrderedMappings.
src/services/sourcemaps.ts
Outdated
| }; | ||
|
|
||
| function getGeneratedPosition(loc: SourceMappableLocation): SourceMappableLocation { | ||
| const maps = filter(getForwardSortedMappings(), m => comparePaths(loc.fileName, m.sourcePath, sourceRoot) === 0); |
There was a problem hiding this comment.
Couldn't we binarySearch without the filter and then check whether the result points to a mapping in a different file? filter requires a full scan of the array prior to using the more efficient binarySearch.
src/services/sourcemaps.ts
Outdated
| return { fileName: toPath(map.file, sourceRoot, host.getCanonicalFileName), position: maps[targetIndex].emittedPosition }; // Closest span | ||
| } | ||
|
|
||
| function getOriginalPosition(loc: SourceMappableLocation): SourceMappableLocation { |
There was a problem hiding this comment.
Couldn't we binarySearch without the filter and then check whether the result points to a mapping in a different file? filter requires a full scan of the array prior to using the more efficient binarySearch.
There was a problem hiding this comment.
Is this comment in error? The method it is on does not filter anything.
src/services/sourcemaps.ts
Outdated
| return forwardSortedMappings || (forwardSortedMappings = getDecodedMappings().slice().sort(compareProcessedSpanEmittedPositions)); | ||
| } | ||
|
|
||
| function calculateDecodedMappings(): ProcessedSourceMapSpan[] { |
There was a problem hiding this comment.
This could probably be done without so many fields on state if you inline hasCompletedDecoding and decodeSingleSpan. It doesn't need to be done in this PR but it seems unnecessarily complex.
src/services/sourcemaps.ts
Outdated
| name?: string; | ||
| } | ||
|
|
||
| interface RawSourceMapSpan { |
There was a problem hiding this comment.
To avoid confusion, this should not be called "-Span" but "-Position".
src/services/sourcemaps.ts
Outdated
| return state.decodingIndex === state.encodedText.length; | ||
| } | ||
|
|
||
| function decodeSingleSpan<T>(state: DecoderState<T>): void { |
There was a problem hiding this comment.
To avoid confusion, this should not be called "-Span" but "-Position".
src/services/sourcemaps.ts
Outdated
| emittedPosition: getPositionOfLineAndCharacterUsingName(map.file, currentDirectory, span.emittedLine - 1, span.emittedColumn - 1), | ||
| sourcePosition: getPositionOfLineAndCharacterUsingName(sourcePath, sourceRoot, span.sourceLine - 1, span.sourceColumn - 1), | ||
| sourcePath, | ||
| name: span.nameIndex ? map.names[span.nameIndex] : undefined |
There was a problem hiding this comment.
We don't actually use names anywhere in the results, so this is unnecessary work.
src/services/sourcemaps.ts
Outdated
|
|
||
| function isSourceMappingSegmentEnd(encodedText: string, pos: number) { | ||
| return (pos === encodedText.length || | ||
| encodedText.charAt(pos) === "," || |
There was a problem hiding this comment.
It would be better to use .charCodeAt and CharacterCodes here.
| } | ||
| // 5. Check if there is name: | ||
| if (!isSourceMappingSegmentEnd(state.encodedText, state.decodingIndex)) { | ||
| if (state.currentNameIndex === undefined) { |
There was a problem hiding this comment.
We don't use names, so should we care about the name index or whether it's invalid? We could just as easily advance the position until we hit the segment end. I'd rather air on the side of leniency for a better user experience.
src/services/services.ts
Outdated
| return; | ||
| } | ||
| const starts = getLineStarts(mappedFile); | ||
| for (let index = starts.length - 1; index--; index >= 0) { |
There was a problem hiding this comment.
You have your condition and incrementer swapped.
src/services/services.ts
Outdated
| return file.sourceMapper; | ||
| } | ||
| let mapFileName = scanForSourcemapURL(fileName); | ||
| if (mapFileName && dataURLRE.exec(mapFileName)) { |
There was a problem hiding this comment.
dataURLRE seems unnecessary. The same thing could be accomplished with a single RegExp.
There was a problem hiding this comment.
I'm doing this to avoid bothering to look for a file at a data: URL we couldn't understand. It's likely the rest of the path machinery may handle arbitrary data URLs OK-ish... but do we want it to?
There was a problem hiding this comment.
What I meant was that your regexp could be written like this /^data:(?:application\/json;charset=utf-8;base64,(.+)$)?/. Then you can exec the regexp once. If the result is non-null, its at least a data URL, but if it has a matches[1] then it's a valid base64 data URL.
There was a problem hiding this comment.
Also, it might be better to be more forgiving in the regexp with respect to case sensitivity to the value of charset, since UTF-8 is just as acceptable as utf-8 (and is entirely optional). I'd recommend something only slightly more lenient like this:
/^data:(?:application\/json(?:;charset=[uU][tT][fF]-8)?;base64,(.+)$)?/
There was a problem hiding this comment.
Also, if we wanted to be picky we could restrict the capturing group to ([A-Za-z0-9+\/=]+) (rather than (.+)).
There was a problem hiding this comment.
Do we care if a different charset is supplied (like "US-ASCII" or "UTF-16"), or do we want to only allow UTF8?
There was a problem hiding this comment.
I don't think we want to be in the game of supporting multiple encodings, and our builtin impl (ignoring what the platform can provide) only handles utf8.
There was a problem hiding this comment.
Fair enough, though I do believe leveraging a single RegExp and possibly restricting the allowed values for the capturing group would be worthwhile.
src/services/services.ts
Outdated
|
|
||
| const sourceMapCommentRE = /^\/\/[@#] sourceMappingURL=(.+)$/gm; | ||
| const dataURLRE = /^data:/; | ||
| const base64URLRE = /^data:application\/json;charset=utf-8;base64,(.+)$/; |
There was a problem hiding this comment.
"URLRE" is a lot of sequential uppercase letters. I'd prefer a name like base64UrlRegExp.
|
@rbuckton 👍 👎? |
src/services/services.ts
Outdated
| if (b64EncodedMatch) { | ||
| const base64Object = b64EncodedMatch[1]; | ||
| let match: RegExpExecArray; | ||
| if (mapFileName && (match = base64UrlRegExp.exec(mapFileName))) { |
There was a problem hiding this comment.
nit: I'd just use a nested if to avoid having match declared outside of the block since it isn't used elsewhere.
|
Are there any editors that currently support this? I tried in VS2017 (with Typescript 2.9.1 tooling installed) and VS Code (1.24.0) and both still go to the d.ts file on "go to definition". |
|
were the .d.ts files built with |
|
Yes. At the bottom of the d.ts file there's a sourceMappingURL pointing at the emitted d.ts.map file. That said, I am generating these with the latest build of gulp-typescript. |
|
mind filing a new ticket with some repro steps? |
|
just as a reference: michael filed #25322 for that. |
Adds one command line option:
--declarationMap. When enabled alongside--declaration, it causes us to emit.d.ts.mapfiles alongside the output.d.tsfiles. Services can also now understand these map files, and uses them to map declaration-file based definition locations to their original source, if possible.Fixes #14479
There's a handful of TODOs that I want to talk about:
getTargetOfMappedDeclarationFileneed to be mapped somehow?getDefinitionAndBoundSpanneed to be mapped in some way?sourceMappingURLcomment? (probably, but right now we do not)sectionsproperty (which would likely be beneficial for @RyanCavanaugh 's work)? We currently neither emit nor recognize them.Additionally, this could still use some more tests exercising more of the many sourcemap options we have with
declarationMaps. Do we have a better harness for testing things like this (ie, language service features that depend on compilation output)? Fourslash works (seedeclarationMapGoToDefinition.ts), but it's really tedious (and fragile), since I need to include the sourcemap within the test itself (rather than build the map for the test). Should I write a new harness/update an existing one?