From cf0be4778f0719c94766927e21875b8d33500515 Mon Sep 17 00:00:00 2001 From: Martin Probst Date: Tue, 24 May 2016 20:52:06 -0700 Subject: [PATCH] feat(reporter): improve source map handling and reporting. - 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. --- lib/reporter.js | 17 ++++++++-------- test/unit/reporter.spec.js | 41 +++++++++++++++++++++++++++----------- 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/lib/reporter.js b/lib/reporter.js index 9ea87a1c6..f7dad1039 100644 --- 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 index a60692d6c..5b5b88507 100644 --- 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 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', () => { @@ -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', () => { @@ -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 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() }) }) @@ -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() }) }) @@ -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 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() }) }) @@ -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() }) @@ -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() }) }) @@ -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() }) })