feat(reporter): improve source map handling and reporting.

mprobst · mprobst · commit cf0be4778f07 · 2016-05-25T09:06:39.000-07:00
- Avoid prepending basepath to every file path.
  The original path is usually relative to the project, and much more meaningful
  than basepath + path, which is usually a long, absolute path.
- Format as [mapped location] &lt;- [original location]
  The mapped location is the more important part for users, it's the source
  file they are working on.
- Resolve source map file paths relative to the original file path.
  It's common for tools that generate a source map to not have a complete
  understanding of what paths they should generate, in which case they just use
  the local path. An example tool is TypeScript.
  Resolving the path against the original file path makes these paths
  unambiguous in the project.
diff --git a/lib/reporter.js b/lib/reporter.js
@@ -1,4 +1,5 @@
 var util = require('util')
+var resolve = require('url').resolve
 var log = require('./logger').create('reporter')
 var MultiReporter = require('./reporters/multi')
 var baseReporterDecoratorFactory = require('./reporters/base').decoratorFactory
@@ -23,7 +24,7 @@ var createErrorFormatter = function (basePath, emitter, SourceMapConsumer) {
   }
 
   var URL_REGEXP = new RegExp('(?:https?:\\/\\/[^\\/]*)?\\/?' +
-    '(base|absolute)' + // prefix
+    '(base/|absolute)' + // prefix, including slash for base/ to create relative paths.
     '((?:[A-z]\\:)?[^\\?\\s\\:]*)' + // path
     '(\\?\\w*)?' + // sha
     '(\\:(\\d+))?' + // line
@@ -53,11 +54,8 @@ var createErrorFormatter = function (basePath, emitter, SourceMapConsumer) {
     // remove domain and timestamp from source files
     // and resolve base path / absolute path urls into absolute path
     var msg = input.replace(URL_REGEXP, function (_, prefix, path, __, ___, line, ____, column) {
-      if (prefix === 'base') {
-        path = basePath + path
-      }
-
-      var file = findFile(path)
+      // Find the file using basePath + path, but use the more readable path down below.
+      var file = findFile(prefix === 'base/' ? basePath + '/' + path : path)
 
       if (file && file.sourceMap && line) {
         line = parseInt(line || '0', 10)
@@ -72,9 +70,12 @@ var createErrorFormatter = function (basePath, emitter, SourceMapConsumer) {
           var original = getSourceMapConsumer(file.sourceMap)
             .originalPositionFor({line: line, column: (column || 0), bias: bias})
 
+          // Source maps often only have a local file name, resolve to turn into a full path if
+          // the path is not absolute yet.
+          var sourcePath = resolve(path, original.source)
           var formattedColumn = column ? util.format(':%s', column) : ''
-          return util.format('%s:%d%s <- %s:%d:%d', path, line, formattedColumn, original.source,
-            original.line, original.column)
+          return util.format('%s:%d:%d <- %s:%d%s', sourcePath, original.line, original.column,
+              path, line, formattedColumn)
         } catch (e) {
           log.warn('SourceMap position not found for trace: %s', msg)
           // Fall back to non-source-mapped formatting.
diff --git a/test/unit/reporter.spec.js b/test/unit/reporter.spec.js
@@ -45,18 +45,18 @@ describe('reporter', () => {
     })
 
     it('should remove domain from files', () => {
-      expect(formatError('file https://localhost:8080/base/usr/a.js and https://127.0.0.1:8080/base/home/b.js')).to.be.equal('file /usr/a.js and /home/b.js\n')
+      expect(formatError('file https://localhost:8080/base/usr/a.js and https://127.0.0.1:8080/absolute/home/b.js')).to.be.equal('file usr/a.js and /home/b.js\n')
     })
 
     // TODO(vojta): enable once we serve source under urlRoot
     it.skip('should handle non default karma service folders', () => {
       formatError = m.createErrorFormatter('', '/_karma_/')
-      expect(formatError('file https://localhost:8080/_karma_/base/usr/a.js and https://127.0.0.1:8080/_karma_/base/home/b.js')).to.be.equal('file /usr/a.js and /home/b.js\n')
+      expect(formatError('file https://localhost:8080/_karma_/base/usr/a.js and https://127.0.0.1:8080/_karma_/base/home/b.js')).to.be.equal('file usr/a.js and home/b.js\n')
     })
 
     it('should remove shas', () => {
       var ERROR = 'file https://localhost:8080/base/usr/file.js?6e31cb249ee5b32d91f37ea516ca0f84bddc5aa9 and https://127.0.0.1:8080/absolute/home/file.js?6e31cb249ee5b32d91f37ea516ca0f84bddc5aa9'
-      expect(formatError(ERROR)).to.be.equal('file /usr/file.js and /home/file.js\n')
+      expect(formatError(ERROR)).to.be.equal('file usr/file.js and /home/file.js\n')
     })
 
     it('should indent all lines', () => {
@@ -65,7 +65,7 @@ describe('reporter', () => {
 
     it('should restore base paths', () => {
       formatError = m.createErrorFormatter('/some/base', emitter)
-      expect(formatError('at https://localhost:123/base/a.js?123')).to.equal('at /some/base/a.js\n')
+      expect(formatError('at https://localhost:123/base/a.js?123')).to.equal('at a.js\n')
     })
 
     it('should restore absolute paths', () => {
@@ -90,10 +90,11 @@ describe('reporter', () => {
 
     describe('source maps', () => {
       var originalPositionForCallCount = 0
+      var sourceMappingPath = null
 
       class MockSourceMapConsumer {
         constructor (sourceMap) {
-          this.source = sourceMap.content.replace('SOURCE MAP ', '/original/')
+          this.source = sourceMap.content.replace('SOURCE MAP ', sourceMappingPath)
         }
 
         originalPositionFor (position) {
@@ -112,6 +113,7 @@ describe('reporter', () => {
 
       beforeEach(() => {
         originalPositionForCallCount = 0
+        sourceMappingPath = '/original/'
       })
 
       MockSourceMapConsumer.GREATEST_LOWER_BOUND = 1
@@ -127,7 +129,7 @@ describe('reporter', () => {
 
         _.defer(() => {
           var ERROR = 'at https://localhost:123/base/b.js:2:6'
-          expect(formatError(ERROR)).to.equal('at /some/base/b.js:2:6 <- /original/b.js:4:8\n')
+          expect(formatError(ERROR)).to.equal('at /original/b.js:4:8 <- b.js:2:6\n')
           done()
         })
       })
@@ -142,7 +144,7 @@ describe('reporter', () => {
 
         _.defer(() => {
           var ERROR = 'at https://localhost:123/base/b.js:2'
-          expect(formatError(ERROR)).to.equal('at /some/base/b.js:2 <- /original/b.js:4:2\n')
+          expect(formatError(ERROR)).to.equal('at /original/b.js:4:2 <- b.js:2\n')
           done()
         })
       })
@@ -157,7 +159,22 @@ describe('reporter', () => {
 
         _.defer(() => {
           var ERROR = 'at /base/b.js:2:6'
-          expect(formatError(ERROR)).to.equal('at /some/base/b.js:2:6 <- /original/b.js:4:8\n')
+          expect(formatError(ERROR)).to.equal('at /original/b.js:4:8 <- b.js:2:6\n')
+          done()
+        })
+      })
+
+      it('should resolve relative urls from source maps', (done) => {
+        sourceMappingPath = 'original/' // Note: relative path.
+        formatError = m.createErrorFormatter('/some/base', emitter, MockSourceMapConsumer)
+        var servedFiles = [new File('/some/base/path/a.js')]
+        servedFiles[0].sourceMap = {content: 'SOURCE MAP a.fancyjs'}
+
+        emitter.emit('file_list_modified', {served: servedFiles})
+
+        _.defer(() => {
+          var ERROR = 'at /base/path/a.js:2:6'
+          expect(formatError(ERROR)).to.equal('at path/original/a.fancyjs:4:8 <- path/a.js:2:6\n')
           done()
         })
       })
@@ -172,7 +189,7 @@ describe('reporter', () => {
 
         _.defer(() => {
           var ERROR = 'at https://localhost:123/base/b.js:0:0'
-          expect(formatError(ERROR)).to.equal('at /some/base/b.js\n')
+          expect(formatError(ERROR)).to.equal('at b.js\n')
           done()
         })
       })
@@ -187,7 +204,7 @@ describe('reporter', () => {
 
         _.defer(() => {
           var ERROR = 'at https://localhost:123/base/b.js'
-          expect(formatError(ERROR)).to.equal('at /some/base/b.js\n')
+          expect(formatError(ERROR)).to.equal('at b.js\n')
           expect(originalPositionForCallCount).to.equal(0)
           done()
         })
@@ -208,7 +225,7 @@ describe('reporter', () => {
 
           _.defer(() => {
             var ERROR = 'at https://localhost:123/absoluteC:/a/b/c.js:2:6'
-            expect(formatError(ERROR)).to.equal('at C:/a/b/c.js:2:6 <- /original/b.js:4:8\n')
+            expect(formatError(ERROR)).to.equal('at c:/original/b.js:4:8 <- C:/a/b/c.js:2:6\n')
             done()
           })
         })
@@ -218,7 +235,7 @@ describe('reporter', () => {
 
           _.defer(() => {
             var ERROR = 'at https://localhost:123/absoluteC:/a/b/c.js?da39a3ee5e6:2:6'
-            expect(formatError(ERROR)).to.equal('at C:/a/b/c.js:2:6 <- /original/b.js:4:8\n')
+            expect(formatError(ERROR)).to.equal('at c:/original/b.js:4:8 <- C:/a/b/c.js:2:6\n')
             done()
           })
         })
