Improves relative path handling for urls #126

rbuckton · 2016-02-20T23:07:36Z

Fixes #84.

Adds two utility functions to better manage path handling: isURI and toPath.

isURI - Determines whether a file path is a URI using the following rules:

A path is a URI if it starts with an alpha character followed by one or more of either an alpha character, digit, +, -, or -, followed by a colon. For example:

http://tempuri.org/path - Web URI
file://host/path        - File URI for UNC path
file:///c:/path         - File URI for DOS path
urn:custom              - Other URI

A path is NOT a URI if it starts with a single alpha character and a colon is, instead it is treated as a DOS path. For example:
```
c:/path                 - DOS path
c:\path                 - DOS path
```
Any other sequence of characters is not considered a URI.

toPath - Tries to convert a URI to a local path. If the URI is a file URI (file://), it is converted into a local path format that NodeJS understands using the following rules:

A file URI with a host name is treated as a UNC path:
```
file://server/share         -> //host/path
```
A file URI without a host name, whose first path segment is a single alpha character followed by either a colon (:) or pipe (|) is treated as a rooted DOS path:
```
file:///c:/path         -> c:\path
file:///c|/path         -> c:\path
```
A file URI without a hostname that is not treated as a DOS path is treated as a rooted POSIX path:
```
file:///etc/path        -> /etc/path
```
Querystring (?) and fragments (#) are removed.

URI handling is designed to conform to RFC 3986, with exceptions for DOS path handling.

Also fixes the tests so that they also run on Windows.

LinusU · 2016-02-21T17:58:36Z

browser-test/script.map

@@ -3,7 +3,7 @@
  "file": "script.js",
  "sourceRoot": "..",
  "sources": [
-    "browser-test/script.coffee"
+    "browser-test\\script.coffee"


Does this work on unix?

I didn't notice that. script.map is a build output, but I can revert the incidental change

Actually, it looks like script.map is not excluded via the .gitignore as is the case for the other tests.

ncalexan · 2016-04-27T23:21:42Z

Is there more work to be done here? This looks like a more comprehensive approach to #133; can we move this forward and close that attempt?

ncalexan · 2016-04-28T00:06:19Z

I need a single small commit to get better printing locally: ncalexan@bbfcde4

…ces. The source-map-support package doesn't appreciate absolute file:// URLs. I tried to address this in evanw/node-source-map-support#133 and made some progress before discovering evanw/node-source-map-support#126. I pushed that work, and a small follow-up, to my local version used here. With this, I get stack traces that referring to app/.

The source-map-support package doesn't appreciate absolute file:// URLs. I tried to address this in evanw/node-source-map-support#133 and made some progress before discovering evanw/node-source-map-support#126. I pushed that work, and a small follow-up, to my local version used here. With this, I get stack traces that referring to app/.

…es. r=Mossop,rnewman. (#289) The source-map-support package doesn't appreciate absolute file:// URLs. I tried to address this in evanw/node-source-map-support#133 and made some progress before discovering evanw/node-source-map-support#126. I pushed that work, and a small follow-up, to my local version used here. With this, I get stack traces that referring to app/.

felixfbecker · 2016-06-02T11:43:21Z

I don't know why isUri is needed. Source map paths are always URIs. They can be relative, but they cannot be paths (for example no backslashes). This should be enough:

        let path = url.parse(uri).pathname;
        // strip the trailing slash from Windows paths (indicated by a drive letter with a colon)
        if (/^\/[a-zA-Z]:\//.test(path)) {
            path = path.substr(1);
        }

rbuckton · 2016-06-07T18:45:26Z

@felixfbecker path.resolve and uri.resolve work very differently. Being able to more accurately gauge which branch to take helps to make this library better able to handle a larger number of scenarios. Just because source map paths should always be a URI doesn't mean that tools and developers will always make sure it is a valid URI. This makes the library more resilient to various inputs and hopefully reduces breaking changes.

felixfbecker · 2016-06-10T13:18:13Z

Can't wait to get this merged...

felixfbecker · 2016-07-15T07:40:08Z

What's the progress on this?

LinusU · 2016-07-15T08:00:06Z

@rbuckton Do you feel that this is ready to merge?

@ncalexan I saw that you are using a fork, would merging this be enough for you to go back, or is there something more that we should add?

felixfbecker · 2016-07-15T08:59:50Z

What I noticed is that when using an absolute file uri as sourceRoot, the file path in the stack trace is also reported as a URL, while it should be converted to a path.

felixfbecker · 2016-07-15T13:55:40Z

Example:

Error: Cannot find module './routes/investment/fundings'
    at Function.Module._resolveFilename (module.js:438:15)
    at Function.Module._load (module.js:386:25)
    at Module.require (module.js:466:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (file:///c:/Users/felix/git/Projekte/asdasdsad/src/api/app.ts:19:1)
    at Module._compile (module.js:541:32)
    at Object.Module._extensions..js (module.js:550:10)
    at Module.load (module.js:456:32)
    at tryModuleLoad (module.js:415:12)
    at Function.Module._load (module.js:407:3)
    at Module.require (module.js:466:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (file:///c:/Users/felix/git/Projekte/afsdfdsaf/src/test/api/routes/index.ts:4:1)
    at Module._compile (module.js:541:32)
    at Object.Module._extensions..js (module.js:550:10)
    at Module.load (module.js:456:32)
    at tryModuleLoad (module.js:415:12)
    at Function.Module._load (module.js:407:3)
    at Module.require (module.js:466:17)

but this could be solved in a different PR, my team is currently depending on this from your fork and it works fine so far. Would really like to see this get merged.

ncalexan · 2016-07-15T16:59:43Z

@rbuckton Do you feel that this is ready to merge?

@ncalexan I saw that you are using a fork, would merging this be enough for you to go back, or is there something more that we should add?

Hi folks, sorry I've been out of this loop. The project consuming this (https://github.com/mozilla/tofino) may not really need the fork anymore; I'd need to evaluate. My guess is we'd revert to mainline immediately after merge, and re-open if necessary. We're not interested in maintaining forks long term.

felixfbecker · 2016-07-18T08:53:41Z

So can we merge this?

rbuckton · 2016-07-18T13:38:55Z

I have no further changes at this time, so I feel it is ready to merge.

LinusU · 2016-07-20T22:06:15Z

test.js

+    file = path.resolve(file);
+    file = file.replace(/\\/g, '/');
+    file = file[0] !== '/' ? '/' + file : file;
+    return encodeURI('file://' + file);


Can you update this (and the other new functions) to follow the 2 space indentation that the rest of the project uses?

LinusU · 2016-07-20T22:08:46Z

test.js

@@ -110,7 +147,7 @@ function compareStdout(done, sourceMap, source, expected) {
      compareLines(
        (stdout + stderr)
          .trim()
-          .split('\n')
+          .split(/\r\n|\r|\n/g)


Standalone \r is only used on Mac OS classic which no one uses today. How about /\r?\n/g?

felixfbecker · 2016-08-09T13:56:09Z

ping @rbuckton
can you adress the comments? Otherwise I can do it and open a new PR.

felixfbecker · 2018-08-22T18:56:39Z

Why close this?

rbuckton · 2018-08-22T19:33:01Z

It's been over two years since I opened it and I haven't had the time to update the PR or determine if it is still needed.

rbuckton added 5 commits February 18, 2016 11:45

Update tests to run on Windows

dedaf84

Added support for file urls

c3fbc76

Cleaned up url handling

5836cc6

Removed NTFS long paths

2bd6b19

Restricted DOS paths to alpha drive letters.

264f50c

LinusU reviewed Feb 21, 2016
View reviewed changes

Updated .gitignore

945109a

ncalexan mentioned this pull request Apr 28, 2016

User a custom source-map-support in order to get meaningful stack tra… mozilla/tofino#289

Merged

rbuckton added 2 commits June 7, 2016 16:09

Fix case sensitivity for DOS paths.

0305903

Rename toPath to toLocalPath and export

9cf6863

LinusU reviewed Jul 20, 2016
View reviewed changes

rbuckton closed this Aug 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improves relative path handling for urls #126

Improves relative path handling for urls #126

rbuckton commented Feb 20, 2016

LinusU Feb 21, 2016

rbuckton Feb 21, 2016

rbuckton Feb 21, 2016

ncalexan commented Apr 27, 2016

ncalexan commented Apr 28, 2016

felixfbecker commented Jun 2, 2016 •

edited

Loading

rbuckton commented Jun 7, 2016

felixfbecker commented Jun 10, 2016

felixfbecker commented Jul 15, 2016

LinusU commented Jul 15, 2016

felixfbecker commented Jul 15, 2016

felixfbecker commented Jul 15, 2016 •

edited

Loading

ncalexan commented Jul 15, 2016

felixfbecker commented Jul 18, 2016

rbuckton commented Jul 18, 2016

LinusU Jul 20, 2016

LinusU Jul 20, 2016

felixfbecker commented Aug 9, 2016

felixfbecker commented Aug 22, 2018

rbuckton commented Aug 22, 2018

Improves relative path handling for urls #126

Improves relative path handling for urls #126

Conversation

rbuckton commented Feb 20, 2016

LinusU Feb 21, 2016

Choose a reason for hiding this comment

rbuckton Feb 21, 2016

Choose a reason for hiding this comment

rbuckton Feb 21, 2016

Choose a reason for hiding this comment

ncalexan commented Apr 27, 2016

ncalexan commented Apr 28, 2016

felixfbecker commented Jun 2, 2016 • edited Loading

rbuckton commented Jun 7, 2016

felixfbecker commented Jun 10, 2016

felixfbecker commented Jul 15, 2016

LinusU commented Jul 15, 2016

felixfbecker commented Jul 15, 2016

felixfbecker commented Jul 15, 2016 • edited Loading

ncalexan commented Jul 15, 2016

felixfbecker commented Jul 18, 2016

rbuckton commented Jul 18, 2016

LinusU Jul 20, 2016

Choose a reason for hiding this comment

LinusU Jul 20, 2016

Choose a reason for hiding this comment

felixfbecker commented Aug 9, 2016

felixfbecker commented Aug 22, 2018

rbuckton commented Aug 22, 2018

felixfbecker commented Jun 2, 2016 •

edited

Loading

felixfbecker commented Jul 15, 2016 •

edited

Loading