Skip to content

Call toPath on sourceRoot to ensure it is always absolute #25838

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

Merged
merged 2 commits into from
Jul 24, 2018

Conversation

weswigham
Copy link
Member

Fixes #25322

@weswigham weswigham requested review from mhegazy and a user July 20, 2018 20:41
//// member: string;
//// /*2*/methodName(propName: SomeType): void {}
//// otherMethod() {
//// if (Math.random() > 0.5) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you try to cut the repro down to size?

Copy link
Member Author

@weswigham weswigham Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, I didn't want to since it is just a copy of the other test with a relative sourceroot patched in. :3

@@ -61,7 +61,7 @@ namespace ts.sourcemaps {

export function decode(host: SourceMapDecodeHost, mapPath: string, map: SourceMapData, program?: Program, fallbackCache = createSourceFileLikeCache(host)): SourceMapper {
const currentDirectory = getDirectoryPath(mapPath);
const sourceRoot = map.sourceRoot || currentDirectory;
const sourceRoot = map.sourceRoot ? toPath(map.sourceRoot, currentDirectory, p => host.getCanonicalFileName(p)) : currentDirectory;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to use getNormalizedAbsolutePath instead to avoid messing with the casing when not needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Within this file, sourceRoot is passed into comparePaths, and as a base for toPath, so in all places it would just be canonicalized later anyway. By canonicalizing it early, hopefully work is repeated less.

@@ -61,7 +61,7 @@ namespace ts.sourcemaps {

export function decode(host: SourceMapDecodeHost, mapPath: string, map: SourceMapData, program?: Program, fallbackCache = createSourceFileLikeCache(host)): SourceMapper {
const currentDirectory = getDirectoryPath(mapPath);
const sourceRoot = map.sourceRoot || currentDirectory;
const sourceRoot = map.sourceRoot ? getNormalizedAbsolutePath(map.sourceRoot, currentDirectory) : currentDirectory;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to handle if the sourceRoot specified is URL ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getNormalizedAbsolutePath already passes along URLs just fine (it preserves the input root if it is rooted), as will the rest of the path handling in the sourcemap code. The language service will likely summarily reject the mapped location, however, since https://github.com/Microsoft/TypeScript/tree/master/src/compiler/checker.ts (for example) won't exist on disk (though I imagine a host implementation could exist that returns results for such paths). UNC paths might in theory work fine, though.... those return normal results via fileExists and similar APIs on windows, right?

In any case: No, I don't think so - all the path normalizing/building stuff already is.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 24, 2018

@weswigham can you please port to release-3.0 as well.

@weswigham weswigham merged commit 797d8bd into microsoft:master Jul 24, 2018
@weswigham weswigham deleted the handle-relative-sourceRoot branch July 24, 2018 21:14
weswigham added a commit to weswigham/TypeScript that referenced this pull request Jul 24, 2018
…25838)

* Call toPath on sourceRoot to ensure it is always absolute

* Leave canonicalization to avoid a Path/string union
mhegazy added a commit that referenced this pull request Jul 24, 2018
Call toPath on sourceRoot to ensure it is always absolute (#25838)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants