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
mprobst committed May 25, 2016
1 parent 9a972b1 commit cf0be47
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 20 deletions.
17 changes: 9 additions & 8 deletions lib/reporter.js
View file
@@ -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
Expand All @@ -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
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
41 changes: 29 additions & 12 deletions test/unit/reporter.spec.js
View file
Expand Up @@ -45,18 +45,18 @@ describe('reporter', () => {
})

it('should remove domain from files', () => {
expect(formatError('file http://localhost:8080/base/usr/a.js and http://127.0.0.1:8080/base/home/b.js')).to.be.equal('file /usr/a.js and /home/b.js\n')
expect(formatError('file http://localhost:8080/base/usr/a.js and http://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 http://localhost:8080/_karma_/base/usr/a.js and http://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 http://localhost:8080/_karma_/base/usr/a.js and http://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 http://localhost:8080/base/usr/file.js?6e31cb249ee5b32d91f37ea516ca0f84bddc5aa9 and http://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', () => {
Expand All @@ -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 All @@ -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) {
Expand All @@ -112,6 +113,7 @@ describe('reporter', () => {

beforeEach(() => {
originalPositionForCallCount = 0
sourceMappingPath = '/original/'
})

MockSourceMapConsumer.GREATEST_LOWER_BOUND = 1
Expand All @@ -127,7 +129,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 +144,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 +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()
})
})
Expand All @@ -172,7 +189,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 +204,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 +225,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 c:/original/b.js:4:8 <- C:/a/b/c.js:2:6\n')
done()
})
})
Expand All @@ -218,7 +235,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 c:/original/b.js:4:8 <- C:/a/b/c.js:2:6\n')
done()
})
})
Expand Down

0 comments on commit cf0be47

Please sign in to comment.