From 61473f8731343766e8b86270f394e549dfc1f3cb Mon Sep 17 00:00:00 2001
From: Blaine Bublitz <blaine.bublitz@gmail.com>
Date: Tue, 21 Mar 2017 12:16:14 -0700
Subject: [PATCH] Breaking: Reduce argument juggling (closes #15)

---
 index.js      |  31 ++++++++----
 test/add.js   | 121 ++++++++++++++++++++++++++++++++--------------
 test/write.js | 131 ++++++++++++++++++++++++++++++++++----------------
 3 files changed, 195 insertions(+), 88 deletions(-)

diff --git a/index.js b/index.js
index 7187a8a..a0040b5 100644
--- a/index.js
+++ b/index.js
@@ -6,6 +6,10 @@ var helpers = require('./lib/helpers');
 
 var PLUGIN_NAME = 'vinyl-sourcemap';
 
+function isObject(value) {
+	return value && typeof value === 'object' && !Array.isArray(value);
+}
+
 /**
  * Add a sourcemap to a vinyl file (async, with callback function)
  * @param file
@@ -14,21 +18,23 @@ var PLUGIN_NAME = 'vinyl-sourcemap';
  */
 function add(file, options, callback) {
 
-	// check if options are passed or a callback as second argument
-	// if there are 3 arguments, the options param should be an object
+	// Check if options or a callback are passed as second argument
 	if (typeof options === 'function') {
 		callback = options;
 		options = {};
-	} else if (!options || typeof options !== 'object') {
-		return callback(new Error(PLUGIN_NAME + '-add: Invalid argument: options'));
 	}
 
-	// Throw an error if the file argument is not a vinyl file
+	// Default options if not an object
+	if (!isObject(options)) {
+		options = {};
+	}
+
+	// Bail early an error if the file argument is not a Vinyl file
 	if (!File.isVinyl(file)) {
 		return callback(new Error(PLUGIN_NAME + '-add: Not a vinyl file'));
 	}
 
-	// Return the file if already has sourcemaps
+	// Bail early successfully if file already has sourcemap
 	if (file.sourceMap) {
 		return callback(null, file);
 	}
@@ -56,23 +62,30 @@ function add(file, options, callback) {
  */
 function write(file, options, callback) {
 
-	// Check arguments for optional destPath, options, or callback function
+	// Check if options or a callback are passed as second argument
 	if (typeof options === 'function') {
 		callback = options;
 		options = {};
 	}
 
-	options = options || {};
+	// Default options if not an object
+	if (!isObject(options)) {
+		options = {};
+	}
 
-	// Throw an error if the file argument is not a vinyl file
+	// Bail early with an error if the file argument is not a Vinyl file
 	if (!File.isVinyl(file)) {
 		return callback(new Error(PLUGIN_NAME + '-write: Not a vinyl file'));
 	}
 
+	// Bail early with an error if file has streaming contents
+	// TODO: needs test
 	if (file.isStream()) {
 		return callback(new Error(PLUGIN_NAME + '-write: Streaming not supported'));
 	}
 
+	// Bail early successfully if file is null or doesn't have sourcemap
+	// TODO: needs test (at least for null contents?)
 	if (file.isNull() || !file.sourceMap) {
 		return callback(null, file);
 	}
diff --git a/test/add.js b/test/add.js
index 93bfbb2..5f30f6c 100644
--- a/test/add.js
+++ b/test/add.js
@@ -39,56 +39,103 @@ function makeFileWithInlineSourceMap() {
 
 describe('add', function() {
 
-	it('should not accept null as argument', function(done) {
-		sourcemaps.add(null, function(err) {
-			expect(err instanceof Error && err.message === 'vinyl-sourcemap-add: Not a vinyl file').toExist();
-			done();
+	describe('ensures file argument', function() {
+
+		it('is not undefined', function(done) {
+			sourcemaps.add(undefined, function(err) {
+				expect(err instanceof Error && err.message === 'vinyl-sourcemap-add: Not a vinyl file').toExist();
+				done();
+			});
 		});
-	});
 
-	it('should not accept an empty object as argument', function(done) {
-		sourcemaps.add({}, function(err) {
-			expect(err instanceof Error && err.message === 'vinyl-sourcemap-add: Not a vinyl file').toExist();
-			done();
+		it('is not null', function(done) {
+			sourcemaps.add(null, function(err) {
+				expect(err instanceof Error && err.message === 'vinyl-sourcemap-add: Not a vinyl file').toExist();
+				done();
+			});
 		});
-	});
 
-	it('should not accept a stream as argument', function(done) {
-		sourcemaps.add(new stream.Readable(), function(err) {
-			expect(err instanceof Error && err.message === 'vinyl-sourcemap-add: Not a vinyl file').toExist();
-			done();
+		it('is not a plain object', function(done) {
+			sourcemaps.add({}, function(err) {
+				expect(err instanceof Error && err.message === 'vinyl-sourcemap-add: Not a vinyl file').toExist();
+				done();
+			});
 		});
-	});
 
-	it('should not accept undefined as options argument', function(done) {
-		var file = makeFile();
-		sourcemaps.add(file, undefined, function(err) {
-			expect(err instanceof Error && err.message === 'vinyl-sourcemap-add: Invalid argument: options').toExist();
-			done();
+		// TODO: seems like a bad test
+		it('is not a stream', function(done) {
+			sourcemaps.add(new stream.Readable(), function(err) {
+				expect(err instanceof Error && err.message === 'vinyl-sourcemap-add: Not a vinyl file').toExist();
+				done();
+			});
 		});
-	});
 
-	it('should not accept null as options argument', function(done) {
-		var file = makeFile();
-		sourcemaps.add(file, null, function(err) {
-			expect(err instanceof Error && err.message === 'vinyl-sourcemap-add: Invalid argument: options').toExist();
-			done();
+		it('is a vinyl object', function(done) {
+			var file = makeFile();
+			sourcemaps.add(file, function(err) {
+				expect(err).toNotExist();
+				done();
+			});
 		});
 	});
 
-	it('should not accept empty string as options argument', function(done) {
-		var file = makeFile();
-		sourcemaps.add(file, '', function(err) {
-			expect(err instanceof Error && err.message === 'vinyl-sourcemap-add: Invalid argument: options').toExist();
-			done();
+	describe('ensures options argument', function() {
+
+
+		it('is defaulted if undefined', function(done) {
+			var file = makeFile();
+			sourcemaps.add(file, undefined, function(err) {
+				expect(err).toNotExist();
+				done();
+			});
 		});
-	});
 
-	it('should not accept boolean as options argument', function(done) {
-		var file = makeFile();
-		sourcemaps.add(file, true, function(err) {
-			expect(err instanceof Error && err.message === 'vinyl-sourcemap-add: Invalid argument: options').toExist();
-			done();
+		it('is defaulted if null', function(done) {
+			var file = makeFile();
+			sourcemaps.add(file, null, function(err) {
+				expect(err).toNotExist();
+				done();
+			});
+		});
+
+		it('is defaulted if empty string', function(done) {
+			var file = makeFile();
+			sourcemaps.add(file, '', function(err) {
+				expect(err).toNotExist();
+				done();
+			});
+		});
+
+		it('is defaulted if non-empty string', function(done) {
+			var file = makeFile();
+			sourcemaps.add(file, 'invalid', function(err) {
+				expect(err).toNotExist();
+				done();
+			});
+		});
+
+		it('is defaulted if boolean false', function(done) {
+			var file = makeFile();
+			sourcemaps.add(file, false, function(err) {
+				expect(err).toNotExist();
+				done();
+			});
+		});
+
+		it('is defaulted if boolean true', function(done) {
+			var file = makeFile();
+			sourcemaps.add(file, true, function(err) {
+				expect(err).toNotExist();
+				done();
+			});
+		});
+
+		it('is defaulted if array', function(done) {
+			var file = makeFile();
+			sourcemaps.add(file, [], function(err) {
+				expect(err).toNotExist();
+				done();
+			});
 		});
 	});
 
diff --git a/test/write.js b/test/write.js
index 5993020..b43ca22 100644
--- a/test/write.js
+++ b/test/write.js
@@ -50,66 +50,113 @@ function base64JSON(object) {
 
 describe('write', function() {
 
-	it('should return an error when on valid vinyl file is provided', function(done) {
-		sourcemaps.write(undefined, function(err) {
-			expect(err instanceof Error && err.message === 'vinyl-sourcemap-write: Not a vinyl file').toExist();
-			done();
+	describe('ensures file argument', function() {
+
+		it('is not undefined', function(done) {
+			sourcemaps.write(undefined, function(err) {
+				expect(err instanceof Error && err.message === 'vinyl-sourcemap-write: Not a vinyl file').toExist();
+				done();
+			});
 		});
-	});
 
-	it('should return an error when on valid vinyl file is provided', function(done) {
-		sourcemaps.write(null, function(err) {
-			expect(err instanceof Error && err.message === 'vinyl-sourcemap-write: Not a vinyl file').toExist();
-			done();
+		it('is not null', function(done) {
+			sourcemaps.write(null, function(err) {
+				expect(err instanceof Error && err.message === 'vinyl-sourcemap-write: Not a vinyl file').toExist();
+				done();
+			});
 		});
-	});
 
-	it('should return an error when on valid vinyl file is provided', function(done) {
-		sourcemaps.write({}, function(err) {
-			expect(err instanceof Error && err.message === 'vinyl-sourcemap-write: Not a vinyl file').toExist();
-			done();
+		it('is not a plain object', function(done) {
+			sourcemaps.write({}, function(err) {
+				expect(err instanceof Error && err.message === 'vinyl-sourcemap-write: Not a vinyl file').toExist();
+				done();
+			});
 		});
-	});
 
-	it('should return an error when on valid vinyl file is provided', function(done) {
-		sourcemaps.write(new stream.Readable(), function(err) {
-			expect(err instanceof Error && err.message === 'vinyl-sourcemap-write: Not a vinyl file').toExist();
-			done();
+		// TODO: seems like a bad test
+		it('is not a stream', function(done) {
+			sourcemaps.write(new stream.Readable(), function(err) {
+				expect(err instanceof Error && err.message === 'vinyl-sourcemap-write: Not a vinyl file').toExist();
+				done();
+			});
 		});
-	});
 
-	it('calls back with the untouched file if sourceMap property does not exist', function(done) {
-		var file = makeFile();
-		delete file.sourceMap;
-		sourcemaps.write(file, function(err, outFile) {
-			expect(err).toNotExist();
-			expect(file).toExist();
-			expect(outFile).toEqual(file);
-			done(err);
+		it('is a vinyl object', function(done) {
+			var file = makeFile();
+			sourcemaps.write(file, function(err) {
+				expect(err).toNotExist();
+				done();
+			});
 		});
 	});
 
-	it('should return an error when invalid arguments are provided', function(done) {
-		var file = makeFile();
-		sourcemaps.write(file, undefined, function(err) {
-			expect(err).toNotExist();
-			done();
+	describe('ensures options argument', function() {
+
+		it('is defaulted if undefined', function(done) {
+			var file = makeFile();
+			sourcemaps.write(file, undefined, function(err) {
+				expect(err).toNotExist();
+				done();
+			});
 		});
-	});
 
-	it('should return an error when invalid arguments are provided', function(done) {
-		var file = makeFile();
-		sourcemaps.write(file, null, function(err) {
-			expect(err).toNotExist();
-			done();
+		it('is defaulted if null', function(done) {
+			var file = makeFile();
+			sourcemaps.write(file, null, function(err) {
+				expect(err).toNotExist();
+				done();
+			});
+		});
+
+		it('is defaulted if empty string', function(done) {
+			var file = makeFile();
+			sourcemaps.write(file, '', function(err) {
+				expect(err).toNotExist();
+				done();
+			});
+		});
+
+		it('is defaulted if non-empty string', function(done) {
+			var file = makeFile();
+			sourcemaps.write(file, 'invalid', function(err) {
+				expect(err).toNotExist();
+				done();
+			});
+		});
+
+		it('is defaulted if boolean false', function(done) {
+			var file = makeFile();
+			sourcemaps.write(file, false, function(err) {
+				expect(err).toNotExist();
+				done();
+			});
+		});
+
+		it('is defaulted if boolean true', function(done) {
+			var file = makeFile();
+			sourcemaps.write(file, true, function(err) {
+				expect(err).toNotExist();
+				done();
+			});
+		});
+
+		it('is defaulted if array', function(done) {
+			var file = makeFile();
+			sourcemaps.write(file, [], function(err) {
+				expect(err).toNotExist();
+				done();
+			});
 		});
 	});
 
-	it('should return an error when invalid arguments are provided', function(done) {
+	it('calls back with the untouched file if sourceMap property does not exist', function(done) {
 		var file = makeFile();
-		sourcemaps.write(file, true, function(err) {
+		delete file.sourceMap;
+		sourcemaps.write(file, function(err, outFile) {
 			expect(err).toNotExist();
-			done();
+			expect(file).toExist();
+			expect(outFile).toEqual(file);
+			done(err);
 		});
 	});