Skip to content

Commit

Permalink
feat(reporter): improve source map handling and reporting.
Browse files Browse the repository at this point in the history
- 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] <- [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.
  • Loading branch information
mprobst committed May 25, 2016
1 parent 9a972b1 commit 8fed424
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 15 deletions.
15 changes: 8 additions & 7 deletions lib/reporter.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
var util = require('util')
var resolve = require('path').resolve
var log = require('./logger').create('reporter')
var MultiReporter = require('./reporters/multi')
var baseReporterDecoratorFactory = require('./reporters/base').decoratorFactory
Expand Down Expand Up @@ -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)
Expand All @@ -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.
Expand Down
16 changes: 8 additions & 8 deletions test/unit/reporter.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ describe('reporter', () => {

it('should restore base paths', () => {
formatError = m.createErrorFormatter('/some/base', emitter)
expect(formatError('at http://localhost:123/base/a.js?123')).to.equal('at /some/base/a.js\n')
expect(formatError('at http://localhost:123/base/a.js?123')).to.equal('at /a.js\n')
})

it('should restore absolute paths', () => {
Expand Down Expand Up @@ -127,7 +127,7 @@ describe('reporter', () => {

_.defer(() => {
var ERROR = 'at http://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()
})
})
Expand All @@ -142,7 +142,7 @@ describe('reporter', () => {

_.defer(() => {
var ERROR = 'at http://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()
})
})
Expand All @@ -157,7 +157,7 @@ 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()
})
})
Expand All @@ -172,7 +172,7 @@ describe('reporter', () => {

_.defer(() => {
var ERROR = 'at http://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()
})
})
Expand All @@ -187,7 +187,7 @@ describe('reporter', () => {

_.defer(() => {
var ERROR = 'at http://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()
})
Expand All @@ -208,7 +208,7 @@ describe('reporter', () => {

_.defer(() => {
var ERROR = 'at http://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 /original/b.js:4:8 <- C:/a/b/c.js:2:6\n')
done()
})
})
Expand All @@ -218,7 +218,7 @@ describe('reporter', () => {

_.defer(() => {
var ERROR = 'at http://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 /original/b.js:4:8 <- C:/a/b/c.js:2:6\n')
done()
})
})
Expand Down

0 comments on commit 8fed424

Please sign in to comment.