Skip to content
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

Allow comments to contain absolute paths. #41

Merged

Conversation

jamestalmage
Copy link
Collaborator

Using path.resolve instead of path.join allows for absolute paths on Windows. The Node.js documentation does not provide a clear reason why, but I have verified this solves my issue.

You can prove this to yourself with the following command:

$ node -e "console.log(path.win32.join('C:\\foo\\bar', 'D:\\baz\\quz.js'));"
$ node -e "console.log(path.win32.resolve('C:\\foo\\bar', 'D:\\baz\\quz.js'));"

Using `path.resolve` instead of `path.join` allows for absolute paths on Windows. The Node.js documentation does not provide a clear reason why, but I have verified this solves my issue.

You can prove this to yourself with the following command:

```
$ node -e "console.log(path.win32.join('C:\\foo\\bar', 'D:\\baz\\quz.js'));"
$ node -e "console.log(path.win32.resolve('C:\\foo\\bar', 'D:\\baz\\quz.js'));"
```
@rundef
Copy link

rundef commented Jul 21, 2016

This is an issue for me as well ; the typescript ts-node transpiler generates source maps links such as:

//# sourceMappingURL=/tmp/ts-node/72c27f/751238.js.map

But, when using the nyc code coverage tool, nyc calls the fromMapFileSource function from convert-source-map, and it fails at line 30 in the readFromFileMap function, because of the absolute path not being handled correctly.

My solution was to replace this line (in the readFromFileMap function) :

var filepath = path.join(dir, filename);

by

var filepath = path.isAbsolute(filename) ? filename : path.join(dir, filename);

But the code in this PR works as well.

Is it possible to get this merged @thlorenz ?

Thank you

@thlorenz
Copy link
Owner

LGTM and if all tests pass we can merge.
However we'd need to publish as a minor upgrade as it'd change behavior in cases when the dir is provided as a relative path as well.

@thlorenz
Copy link
Owner

@jamestalmage made you collaborator.

Please merge to master following this guide.
I will then version and publish to npm.

Thanks.

@jamestalmage jamestalmage merged commit 81d7d2d into thlorenz:master Jul 22, 2016
@jamestalmage jamestalmage deleted the allow-filename-to-be-absolute branch July 22, 2016 16:37
@jamestalmage
Copy link
Collaborator Author

Merged.

FYI: The merge guide seems a bit outdated. Using the "Squash and Merge" option from the GH UI works the same as a fast forward merge. You should be able to adjust the default and/or disable merge commits in the settings for the repo.

@thlorenz
Copy link
Owner

Thanks, published as v1.3.0.
Good point WRT the merge guide I'll update when I get some time.

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.

None yet

3 participants