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.

diff --git a/History.md b/History.md
index a7c6e0f..1f9e3d2 100644 (file)
--- 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 76c5fc2..aa8cc0b 100644 (file)
--- 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 cbe5e52..6fbf4e6 100644 (file)
--- 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 21a40c6..40d312f 100644 (file)
--- 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 ae06b08..979515d 100644 (file)
--- 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 3c273f9..852ad9f 100644 (file)
--- 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 dd8e310..e147147 100644 (file)
--- 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')
           ]);
         }
       }