Skip to content

Patch for multi disk support on Windows(TS Sourcemap) #493

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

Conversation

itoys
Copy link
Contributor

@itoys itoys commented Jul 27, 2017

@msftclas
Copy link

@itoys,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@codecov-io
Copy link

codecov-io commented Jul 27, 2017

Codecov Report

Merging #493 into master will increase coverage by 0.81%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #493      +/-   ##
==========================================
+ Coverage   33.87%   34.68%   +0.81%     
==========================================
  Files          65       65              
  Lines        3312     3321       +9     
  Branches      382      385       +3     
==========================================
+ Hits         1122     1152      +30     
+ Misses       2188     2167      -21     
  Partials        2        2
Impacted Files Coverage Δ
src/debugger/sourceMapsCombinator.ts 91.83% <81.81%> (+54.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4254046...cbed0b4. Read the comment docs.

// Hack for source-map-resolve and cutted disk letter
// https://github.com/lydell/source-map-resolve/issues/9
function readFileSync(diskLetter: string, filePath: string) {
return fs.readFileSync(`${diskLetter}${filePath}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to check if the filePath is an ablolute path - in this case you'd broke it

Copy link
Contributor Author

@itoys itoys Jul 27, 2017

Choose a reason for hiding this comment

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

This is always looks like absolute and trimmed
c:/project/tests/index.js -> /project/tests/index.js
On unix like systems won't diskLetter

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, now it works that way, but I can imagine that when urix transitive dependency (which has the bug we're trying to workaround) gets updated we'd run into trouble which we could avoid by adding just one additional check

@vladimir-kotikov
Copy link
Contributor

As a general request - please also add a couple of tests to cover this bug

@itoys itoys changed the title Patch for multi disk support on Windows(TS Sourcemap) [WIP] Patch for multi disk support on Windows(TS Sourcemap) Jul 28, 2017
@itoys itoys changed the title [WIP] Patch for multi disk support on Windows(TS Sourcemap) Patch for multi disk support on Windows(TS Sourcemap) Jul 30, 2017

const stub = sandbox.stub(fs, "readFileSync");

stub.withArgs(pathToJS).returns(code);
Copy link
Contributor

Choose a reason for hiding this comment

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

Pls rename stub -> fsReadFileStub

@@ -6,6 +6,8 @@ import * as path from "path";
import { SourceMapConsumer, RawSourceMap, SourceMapGenerator, MappingItem, Mapping, Position, MappedPosition } from "source-map";
import sourceMapResolve = require("source-map-resolve");

const DISK: RegExp = /^[a-z]:/i;
Copy link
Contributor

Choose a reason for hiding this comment

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

DISK -> DISK_LETTER_RE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants