From f77ed6ec6495637e6b46f4d2bf03682113e1e040 Mon Sep 17 00:00:00 2001 From: GoalSmashers Date: Mon, 4 Nov 2013 10:30:07 +0100 Subject: [PATCH] Fixes #139 - adds consistent error and warning handling for CLI and library. * Does not throw errors anymore, instead prints them out to STDERR and exits with status 1. * Adds two new fields to CleanCSS objects - warnings and errors. * Updates imports/inliner.js to not act as a singleton. * Adds 'both root and output file given' warning to inliner. --- History.md | 1 + bin/cleancss | 14 +++++++++++++ lib/clean.js | 12 ++++++++--- lib/images/url-rebase.js | 13 +++++++++--- lib/imports/inliner.js | 8 ++++--- test/binary-test.js | 3 ++- test/custom-test.js | 45 +++++++++++++++++++++++++++++++++++++++- test/unit-test.js | 20 +++++++----------- 8 files changed, 92 insertions(+), 24 deletions(-) diff --git a/History.md b/History.md index 2ecc4cc3..5bddb55a 100644 --- a/History.md +++ b/History.md @@ -5,6 +5,7 @@ * Adds simplified and much faster empty elements removal. * Adds missing `@import` processing to our benchmark (run via `npm run bench`). * Fixed issue [#138](https://github.com/GoalSmashers/clean-css/issues/138) - makes CleanCSS interface OO. +* Fixed issue [#139](https://github.com/GoalSmashers/clean-css/issues/138) - consistent error & warning handling. * Fixed issue [#145](https://github.com/GoalSmashers/clean-css/issues/145) - debug mode in library too. * Fixed issue [#157](https://github.com/GoalSmashers/clean-css/issues/157) - gets rid of `removeEmpty` option. * Fixed issue [#159](https://github.com/GoalSmashers/clean-css/issues/159) - escaped quotes inside content. diff --git a/bin/cleancss b/bin/cleancss index c136bc43..e73ebd56 100755 --- a/bin/cleancss +++ b/bin/cleancss @@ -113,6 +113,12 @@ function minify(data) { console.error('Time spent: %dms', minifier.stats.timeSpent); } + outputFeedback(minifier.errors, true); + outputFeedback(minifier.warnings); + + if (minifier.errors.length > 0) + process.exit(1); + return minified; } @@ -122,3 +128,11 @@ function output(minified) { else process.stdout.write(minified); }; + +function outputFeedback(messages, isError) { + var prefix = isError ? '\x1B[31mERROR\x1B[39m:' : 'WARNING:'; + + messages.forEach(function(message) { + console.error('%s %s', prefix, message); + }); +}; diff --git a/lib/clean.js b/lib/clean.js index 74981c90..af9c7e04 100644 --- a/lib/clean.js +++ b/lib/clean.js @@ -25,6 +25,10 @@ var SelectorsOptimizer = require('./selectors/optimizer'); module.exports = function(options) { var lineBreak = process.platform == 'win32' ? '\r\n' : '\n'; var stats = {}; + var context = { + errors: [], + warnings: [] + }; options = options || {}; options.keepBreaks = options.keepBreaks || false; @@ -71,7 +75,7 @@ module.exports = function(options) { var expressionsProcessor = new ExpressionsProcessor(); var freeTextProcessor = new FreeTextProcessor(); var urlsProcessor = new UrlsProcessor(); - var importInliner = new ImportInliner(); + var importInliner = new ImportInliner(context); if (options.processImport) { // inline all imports @@ -265,7 +269,7 @@ module.exports = function(options) { data = urlsProcessor.restore(data); }); replace(function rebaseUrls() { - data = options.noRebase ? data : UrlRebase.process(data, options); + data = options.noRebase ? data : new UrlRebase(options, context).process(data); }); replace(function restoreFreeText() { data = freeTextProcessor.restore(data); @@ -309,9 +313,11 @@ module.exports = function(options) { }; return { + errors: context.errors, lineBreak: lineBreak, options: options, minify: minify, - stats: stats + stats: stats, + warnings: context.warnings }; }; diff --git a/lib/images/url-rebase.js b/lib/images/url-rebase.js index 78792dc6..5b249544 100644 --- a/lib/images/url-rebase.js +++ b/lib/images/url-rebase.js @@ -2,8 +2,8 @@ var path = require('path'); var UrlRewriter = require('./url-rewriter'); -module.exports = { - process: function(data, options) { +module.exports = function UrlRebase(options, context) { + var process = function(data) { var rebaseOpts = { absolute: !!options.root, relative: !options.root && !!options.target, @@ -13,6 +13,9 @@ module.exports = { if (!rebaseOpts.absolute && !rebaseOpts.relative) return data; + if (rebaseOpts.absolute && !!options.target) + context.warnings.push('Both \'root\' and output file given so rebasing URLs as absolute paths'); + if (rebaseOpts.absolute) rebaseOpts.toBase = path.resolve(options.root); @@ -23,5 +26,9 @@ module.exports = { return data; return UrlRewriter.process(data, rebaseOpts); - } + }; + + return { + process: process + }; }; diff --git a/lib/imports/inliner.js b/lib/imports/inliner.js index e1e694d0..f0c793ae 100644 --- a/lib/imports/inliner.js +++ b/lib/imports/inliner.js @@ -3,7 +3,7 @@ var path = require('path'); var UrlRewriter = require('../images/url-rewriter'); -module.exports = function Inliner() { +module.exports = function Inliner(context) { var process = function(data, options) { var tempData = []; var nextStart = 0; @@ -108,8 +108,10 @@ module.exports = function Inliner() { var fullPath = path.resolve(path.join(relativeTo, importedFile)); - if (!fs.existsSync(fullPath) || !fs.statSync(fullPath).isFile()) - throw new Error('Broken @import declaration of "' + importedFile + '"'); + if (!fs.existsSync(fullPath) || !fs.statSync(fullPath).isFile()) { + context.errors.push('Broken @import declaration of "' + importedFile + '"'); + return ''; + } if (options.visited.indexOf(fullPath) != -1) return ''; diff --git a/test/binary-test.js b/test/binary-test.js index 0e4461c0..d4b4a7ab 100644 --- a/test/binary-test.js +++ b/test/binary-test.js @@ -112,9 +112,10 @@ exports.commandsSuite = vows.describe('binary commands').addBatch({ } }), 'no relative to path': binaryContext('./test/data/partials-absolute/base.css', { - 'should not be able to resolve it fully': function(error, stdout) { + 'should not be able to resolve it fully': function(error, stdout, stderr) { assert.equal(stdout, ''); assert.notEqual(error, null); + assert.notEqual(stderr, ''); } }), 'relative to path': binaryContext('-r ./test/data ./test/data/partials-absolute/base.css', { diff --git a/test/custom-test.js b/test/custom-test.js index eef8039c..c01b8fcd 100644 --- a/test/custom-test.js +++ b/test/custom-test.js @@ -39,5 +39,48 @@ vows.describe('clean-custom').addBatch({ 'should give efficiency': function(minifier) { assert.equal(minifier.stats.efficiency, 0.25); } - } + }, + 'no warnings': { + topic: function() { + var minifier = new CleanCSS(); + minifier.minify('a{color:red}'); + return minifier; + }, + 'if no reasons given': function(minifier) { + assert.deepEqual(minifier.warnings, []); + } + }, + 'warnings': { + topic: function() { + var minifier = new CleanCSS({ root: 'test/data', target: 'custom-warnings.css' }); + minifier.minify('a{color:red}'); + return minifier; + }, + 'if both root and output used reasons given': function(minifier) { + assert.equal(minifier.warnings.length, 1); + assert.match(minifier.warnings[0], /Both 'root' and output file given/); + } + }, + 'no errors': { + topic: function() { + var minifier = new CleanCSS(); + minifier.minify('a{color:red}'); + return minifier; + }, + 'if no reasons given': function(minifier) { + assert.deepEqual(minifier.errors, []); + } + }, + 'errors': { + topic: function() { + return new CleanCSS(); + }, + 'if both root and output used reasons given': function(minifier) { + assert.doesNotThrow(function() { + minifier.minify('@import url(/some/fake/file);'); + }); + assert.equal(minifier.errors.length, 1); + assert.equal(minifier.errors[0], 'Broken @import declaration of "/some/fake/file"'); + } + }, }).export(module); diff --git a/test/unit-test.js b/test/unit-test.js index e15aecbf..f74652db 100644 --- a/test/unit-test.js +++ b/test/unit-test.js @@ -9,16 +9,10 @@ var ColorShortener = require('../lib/colors/shortener'); var lineBreak = process.platform == 'win32' ? '\r\n' : '\n'; var cssContext = function(groups, options) { var context = {}; - var clean = function(expectedCSS) { + var clean = function(expectedCss) { return function(css) { - var minifiedCSS = null; - try { - minifiedCSS = new CleanCSS(options).minify(css); - } catch (e) { - // swallow - minifiedCSS is set to null and that's the new expected value - } - - assert.equal(minifiedCSS, expectedCSS); + var minifiedCss = new CleanCSS(options).minify(css); + assert.equal(minifiedCss, expectedCss); }; }; @@ -926,11 +920,11 @@ title']{display:block}", '@import': cssContext({ 'empty': [ "@import url();", - null + '' ], 'of an unknown file': [ "@import url('fake.css');", - null + '' ], 'of a http file': "@import url(http://pro.goalsmashers.com/test.css);", 'of a https file': [ @@ -945,7 +939,7 @@ title']{display:block}", 'of a remote file via // url with media': "@import url(//pro.goalsmashers.com/test.css) screen,tv;", 'of a directory': [ "@import url(test/data/partials);", - null + '' ], 'of a real file': [ "@import url(test/data/partials/one.css);", @@ -1043,7 +1037,7 @@ title']{display:block}", '@import with absolute paths': cssContext({ 'of an unknown file': [ "@import url(/fake.css);", - null + '' ], 'of a real file': [ "@import url(/partials/one.css);", -- 2.34.1