From 1bf840040c5d2f745c6e8ea12dbc743413700864 Mon Sep 17 00:00:00 2001 From: Jakub Pawlowicz Date: Sun, 15 Mar 2015 21:08:10 +0000 Subject: [PATCH] Improves path resolution inside source maps. Source files referenced inside source maps are no properly rebased in many scenarios: * Full resolution for source maps referenced by a path. * Full resolution for source maps passed to `#minify` inside a hash. * Full resolution for remote source maps. * No resolution if path to source map not given. --- History.md | 1 + lib/images/url-rebase.js | 1 - lib/selectors/tokenizer.js | 9 ++--- lib/utils/input-source-map-tracker.js | 45 ++++++++++++++++------ lib/utils/source-maps.js | 14 +++---- lib/utils/source-reader.js | 3 +- test/source-map-test.js | 54 +++++++++++++++++++++------ 7 files changed, 87 insertions(+), 40 deletions(-) diff --git a/History.md b/History.md index a7c6e0fa..1f9e3d27 100644 --- a/History.md +++ b/History.md @@ -1,6 +1,7 @@ [3.2.0 / 2015-xx-xx](https://github.com/jakubpawlowicz/clean-css/compare/v3.1.1...HEAD) ================== +* Improves path resolution inside source maps. * Makes `root` option implicitely default to `process.cwd()`. * Fixed issue [#376](https://github.com/jakubpawlowicz/clean-css/issues/376) - option to disable `0[unit]` -> `0`. * Fixed issue [#396](https://github.com/jakubpawlowicz/clean-css/issues/396) - better input source maps tracking. diff --git a/lib/images/url-rebase.js b/lib/images/url-rebase.js index 76c5fc2b..aa8cc0b9 100644 --- a/lib/images/url-rebase.js +++ b/lib/images/url-rebase.js @@ -1,4 +1,3 @@ -var fs = require('fs'); var path = require('path'); var UrlRewriter = require('./url-rewriter'); diff --git a/lib/selectors/tokenizer.js b/lib/selectors/tokenizer.js index cbe5e526..6fbf4e64 100644 --- a/lib/selectors/tokenizer.js +++ b/lib/selectors/tokenizer.js @@ -3,7 +3,6 @@ var Extract = require('../utils/extractors'); var SourceMaps = require('../utils/source-maps'); var path = require('path'); -var fs = require('fs'); var flatBlock = /(^@(font\-face|page|\-ms\-viewport|\-o\-viewport|viewport|counter\-style)|\\@.+?)/; @@ -34,20 +33,18 @@ Tokenizer.prototype.toTokens = function (data) { source: undefined }; - if (this.minifyContext.options.target) + if (this.minifyContext.options.explicitTarget) context.resolvePath = relativePathResolver(context); return tokenize(context); }; function relativePathResolver(context) { - var rebaseTo = path.resolve(context.outer.options.root, context.outer.options.target); - if (!fs.existsSync(rebaseTo) || fs.statSync(rebaseTo).isFile()) - rebaseTo = path.dirname(rebaseTo); + var rebaseTo = path.relative(context.outer.options.root, context.outer.options.target); return function (relativeTo, sourcePath) { return relativeTo != sourcePath ? - path.normalize(path.join(path.dirname(relativeTo), sourcePath)) : + path.normalize(path.join(path.relative(rebaseTo, path.dirname(relativeTo)), sourcePath)) : sourcePath; }; } diff --git a/lib/utils/input-source-map-tracker.js b/lib/utils/input-source-map-tracker.js index 21a40c62..40d312f6 100644 --- a/lib/utils/input-source-map-tracker.js +++ b/lib/utils/input-source-map-tracker.js @@ -9,6 +9,7 @@ var url = require('url'); var override = require('../utils/object.js').override; var MAP_MARKER = /\/\*# sourceMappingURL=(\S+) \*\//; +var REMOTE_RESOURCE = /^(https?:)?\/\//; function InputSourceMapStore(outerContext) { this.options = outerContext.options; @@ -21,7 +22,7 @@ function InputSourceMapStore(outerContext) { } function fromString(self, _, whenDone) { - self.maps[undefined] = new SourceMapConsumer(self.options.sourceMap); + self.trackLoaded(undefined, undefined, self.options.sourceMap); return whenDone(); } @@ -63,10 +64,10 @@ function fromSource(self, data, whenDone, context) { } else { var sourceFile = context.files[context.files.length - 1]; var sourceDir = sourceFile ? path.dirname(sourceFile) : self.options.relativeTo; - var fullPathToSourceFile = path.join(self.options.root, sourceDir || '', sourceMapFile); + var sourceMapPath = path.resolve(self.options.root, path.join(sourceDir || '', sourceMapFile)); - var inputMapData = fs.readFileSync(fullPathToSourceFile, 'utf-8'); - self.maps[sourceFile || undefined] = new SourceMapConsumer(inputMapData); + var sourceMapData = fs.readFileSync(sourceMapPath, 'utf-8'); + self.trackLoaded(sourceFile || undefined, sourceMapPath, sourceMapData); } } @@ -76,14 +77,14 @@ function fromSource(self, data, whenDone, context) { return whenDone(); } -function fetchMapFile(self, mapSource, context, done) { +function fetchMapFile(self, sourceUrl, context, done) { function handleError(status) { - context.errors.push('Broken source map at "' + mapSource + '" - ' + status); + context.errors.push('Broken source map at "' + sourceUrl + '" - ' + status); return done(); } - var method = mapSource.indexOf('https') === 0 ? https : http; - var requestOptions = override(url.parse(mapSource), self.requestOptions); + var method = sourceUrl.indexOf('https') === 0 ? https : http; + var requestOptions = override(url.parse(sourceUrl), self.requestOptions); method .get(requestOptions, function (res) { @@ -95,7 +96,7 @@ function fetchMapFile(self, mapSource, context, done) { chunks.push(chunk.toString()); }); res.on('end', function () { - self.maps[context.files[context.files.length - 1] || undefined] = new SourceMapConsumer(chunks.join('')); + self.trackLoaded(context.files[context.files.length - 1] || undefined, sourceUrl, chunks.join('')); done(); }); }) @@ -118,7 +119,7 @@ function originalPositionIn(trackedSource, sourceInfo, token, allowNFallbacks) { while (maxRange-- > 0) { position.column--; - originalPosition = trackedSource.originalPositionFor(position); + originalPosition = trackedSource.data.originalPositionFor(position); if (originalPosition) break; @@ -127,6 +128,14 @@ function originalPositionIn(trackedSource, sourceInfo, token, allowNFallbacks) { if (originalPosition.line === null && sourceInfo.line > 1 && allowNFallbacks > 0) return originalPositionIn(trackedSource, { line: sourceInfo.line - 1, column: sourceInfo.column }, token, allowNFallbacks - 1); + if (trackedSource.path) { + originalPosition.source = REMOTE_RESOURCE.test(trackedSource.path) ? + url.resolve(trackedSource.path, originalPosition.source) : + path.join(trackedSource.path, originalPosition.source); + + originalPosition.sourceResolved = true; + } + return originalPosition; } @@ -136,8 +145,20 @@ InputSourceMapStore.prototype.track = function (data, whenDone) { fromSource(this, data, whenDone, { files: [], cursor: 0, errors: this.errors }); }; -InputSourceMapStore.prototype.trackLoaded = function (sourceFile, sourceMap) { - this.maps[sourceFile] = new SourceMapConsumer(sourceMap); +InputSourceMapStore.prototype.trackLoaded = function (sourcePath, mapPath, mapData) { + var relativeTo = this.options.explicitTarget ? this.options.target : this.options.root; + var isRemote = REMOTE_RESOURCE.test(sourcePath); + + if (mapPath) { + mapPath = isRemote ? + path.dirname(mapPath) : + path.dirname(path.relative(relativeTo, mapPath)); + } + + this.maps[sourcePath] = { + path: mapPath, + data: new SourceMapConsumer(mapData) + }; }; InputSourceMapStore.prototype.isTracking = function (source) { diff --git a/lib/utils/source-maps.js b/lib/utils/source-maps.js index ae06b08e..979515d6 100644 --- a/lib/utils/source-maps.js +++ b/lib/utils/source-maps.js @@ -1,5 +1,3 @@ -var url = require('url'); - function trimLeft(value, context) { var withoutContent; var total; @@ -23,12 +21,10 @@ function trimLeft(value, context) { function sourceFor(originalMetadata, contextMetadata, context) { var source = originalMetadata.source || contextMetadata.source; - if (source && contextMetadata.source && (/^https?:\/\//.test(contextMetadata.source) || /^\/\//.test(contextMetadata.source)) && source != contextMetadata.source) - return url.resolve(contextMetadata.source, source); - else if (source && context.resolvePath) + if (source && context.resolvePath) return context.resolvePath(contextMetadata.source, source); - else - return source; + + return source; } var SourceMaps = { @@ -50,7 +46,9 @@ var SourceMaps = { contextMetadata.original.line = sourceMetadata.line || contextMetadata.original.line; contextMetadata.original.column = sourceMetadata.column || contextMetadata.original.column; - contextMetadata.source = sourceFor(sourceMetadata, contextMetadata, context); + contextMetadata.source = sourceMetadata.sourceResolved ? + sourceMetadata.source : + sourceFor(sourceMetadata, contextMetadata, context); this.track(trimmedValue, context); diff --git a/lib/utils/source-reader.js b/lib/utils/source-reader.js index 3c273f94..852ad9fa 100644 --- a/lib/utils/source-reader.js +++ b/lib/utils/source-reader.js @@ -53,7 +53,8 @@ function fromHash(outerContext, sources) { if (outerContext.options.sourceMap && inputSourceMap) { styles = outerContext.sourceTracker.store(source, styles); - outerContext.inputSourceMapTracker.trackLoaded(source, inputSourceMap); + // here we assume source map lies in the same directory as `source` does + outerContext.inputSourceMapTracker.trackLoaded(source, source, inputSourceMap); } data.push(styles); diff --git a/test/source-map-test.js b/test/source-map-test.js index dd8e3100..e1471477 100644 --- a/test/source-map-test.js +++ b/test/source-map-test.js @@ -370,7 +370,7 @@ vows.describe('source-map') generatedColumn: 0, originalLine: 1, originalColumn: 4, - source: 'styles.less', + source: path.join('test', 'fixtures', 'source-maps', 'styles.less'), name: null }; assert.deepEqual(minified.sourceMap._mappings._array[0], mapping); @@ -381,7 +381,7 @@ vows.describe('source-map') generatedColumn: 6, originalLine: 2, originalColumn: 2, - source: 'styles.less', + source: path.join('test', 'fixtures', 'source-maps', 'styles.less'), name: null }; assert.deepEqual(minified.sourceMap._mappings._array[1], mapping); @@ -389,7 +389,7 @@ vows.describe('source-map') }, 'input map from source with root': { 'topic': function () { - return new CleanCSS({ sourceMap: true, relativeTo: path.dirname(inputMapPath) }).minify('div > a {\n color: red;\n}/*# sourceMappingURL=styles.css.map */'); + return new CleanCSS({ sourceMap: true, root: './test/fixtures' }).minify('div > a {\n color: red;\n}/*# sourceMappingURL=source-maps/styles.css.map */'); }, 'should have 2 mappings': function (minified) { assert.lengthOf(minified.sourceMap._mappings._array, 2); @@ -400,7 +400,7 @@ vows.describe('source-map') generatedColumn: 0, originalLine: 1, originalColumn: 4, - source: 'styles.less', + source: path.join('source-maps', 'styles.less'), name: null }; assert.deepEqual(minified.sourceMap._mappings._array[0], mapping); @@ -411,7 +411,37 @@ vows.describe('source-map') generatedColumn: 6, originalLine: 2, originalColumn: 2, - source: 'styles.less', + source: path.join('source-maps', 'styles.less'), + name: null + }; + assert.deepEqual(minified.sourceMap._mappings._array[1], mapping); + } + }, + 'input map from source with target': { + 'topic': function () { + return new CleanCSS({ sourceMap: true, target: './test' }).minify('div > a {\n color: red;\n}/*# sourceMappingURL=' + inputMapPath + ' */'); + }, + 'should have 2 mappings': function (minified) { + assert.lengthOf(minified.sourceMap._mappings._array, 2); + }, + 'should have selector mapping': function (minified) { + var mapping = { + generatedLine: 1, + generatedColumn: 0, + originalLine: 1, + originalColumn: 4, + source: path.join('fixtures', 'source-maps', 'styles.less'), + name: null + }; + assert.deepEqual(minified.sourceMap._mappings._array[0], mapping); + }, + 'should have _color:red_ mapping': function (minified) { + var mapping = { + generatedLine: 1, + generatedColumn: 6, + originalLine: 2, + originalColumn: 2, + source: path.join('fixtures', 'source-maps', 'styles.less'), name: null }; assert.deepEqual(minified.sourceMap._mappings._array[1], mapping); @@ -479,7 +509,7 @@ vows.describe('source-map') }, 'complex but partial input map referenced by path': { 'topic': function () { - return new CleanCSS({ sourceMap: true, target: process.cwd() }).minify('@import url(test/fixtures/source-maps/no-map-import.css);'); + return new CleanCSS({ sourceMap: true }).minify('@import url(test/fixtures/source-maps/no-map-import.css);'); }, 'should have 4 mappings': function (minified) { assert.lengthOf(minified.sourceMap._mappings._array, 4); @@ -499,14 +529,14 @@ vows.describe('source-map') }, 'complex input map with an existing file as target': { 'topic': function () { - return new CleanCSS({ sourceMap: true, target: path.join(process.cwd(), 'test', 'fixtures', 'source-maps', 'styles.css') }).minify('@import url(test/fixtures/source-maps/styles.css);'); + return new CleanCSS({ sourceMap: true, target: path.join('test', 'fixtures', 'source-maps', 'styles.css') }).minify('@import url(test/fixtures/source-maps/styles.css);'); }, 'should have 2 mappings': function (minified) { assert.lengthOf(minified.sourceMap._mappings._array, 2); }, 'should have 2 mappings to styles.less file': function (minified) { var stylesSource = minified.sourceMap._mappings._array.filter(function (mapping) { - return mapping.source == path.join('test', 'fixtures', 'source-maps', 'styles.less'); + return mapping.source == 'styles.less'; }); assert.lengthOf(stylesSource, 2); } @@ -923,7 +953,7 @@ vows.describe('source-map') 'relative to path': { 'complex but partial input map referenced by path': { 'topic': function () { - return new CleanCSS({ sourceMap: true, target: process.cwd() }).minify({ + return new CleanCSS({ sourceMap: true, target: './test' }).minify({ 'test/fixtures/source-maps/some.css': { styles: 'div {\n color: red;\n}', sourceMap: '{"version":3,"sources":["some.less"],"names":[],"mappings":"AAAA;EACE,UAAA","file":"some.css"}' @@ -949,9 +979,9 @@ vows.describe('source-map') }); assert.deepEqual(sources, [ - path.join('test', 'fixtures', 'source-maps', 'some.less'), - path.join('test', 'fixtures', 'source-maps', 'nested', 'once.less'), - path.join('test', 'fixtures', 'source-maps', 'styles.less') + path.join('fixtures', 'source-maps', 'some.less'), + path.join('fixtures', 'source-maps', 'nested', 'once.less'), + path.join('fixtures', 'source-maps', 'styles.less') ]); } } -- 2.34.1