refactor: harden plugins against unknown options (#804)

This reworks the plugin API such that:

 - Unable to register a command with unknown wrap-options
 - `TypeError` raised for wrap-option type mistakes
 - Remove the `overWrite` option (it's unused, probably safest to not
   expose for now)
 - `cmdOptions` defaults to `null` instead of `false` for type
   consistency (no change to default behavior)
 - Move `pipeMethods` logic into `_register`, since it makes more sense
   there

This is not expected to have any effect on existing plugins.

src/common.js

@@ -52,9 +52,6 @@ exports.state = state;
 
 delete process.env.OLDPWD; // initially, there's no previous directory
 
-// This is populated by calls to commonl.wrap()
-var pipeMethods = [];
-
 // Reliably test if something is any sort of javascript object
 function isObject(a) {
   return typeof a === 'object' && a !== null;
@@ -315,9 +312,6 @@ exports.randomFileName = randomFileName;
 // command-logging, and other nice things
 function wrap(cmd, fn, options) {
   options = options || {};
-  if (options.canReceivePipe) {
-    pipeMethods.push(cmd);
-  }
   return function () {
     var retValue = null;
 
@@ -428,22 +422,36 @@ exports.readFromPipe = _readFromPipe;
 var DEFAULT_WRAP_OPTIONS = {
   allowGlobbing: true,
   canReceivePipe: false,
-  cmdOptions: false,
+  cmdOptions: null,
   globStart: 1,
   pipeOnly: false,
-  unix: true,
   wrapOutput: true,
-  overWrite: false,
+  unix: true,
 };
 
+// This is populated during plugin registration
+var pipeMethods = [];
+
 // Register a new ShellJS command
 function _register(name, implementation, wrapOptions) {
   wrapOptions = wrapOptions || {};
+
+  // Validate options
+  Object.keys(wrapOptions).forEach(function (option) {
+    if (!DEFAULT_WRAP_OPTIONS.hasOwnProperty(option)) {
+      throw new Error("Unknown option '" + option + "'");
+    }
+    if (typeof wrapOptions[option] !== typeof DEFAULT_WRAP_OPTIONS[option]) {
+      throw new TypeError("Unsupported type '" + typeof wrapOptions[option] +
+        "' for option '" + option + "'");
+    }
+  });
+
   // If an option isn't specified, use the default
   wrapOptions = Object.assign({}, DEFAULT_WRAP_OPTIONS, wrapOptions);
 
-  if (shell[name] && !wrapOptions.overWrite) {
-    throw new Error('unable to overwrite `' + name + '` command');
+  if (shell[name]) {
+    throw new Error('Command `' + name + '` already exists');
   }
 
   if (wrapOptions.pipeOnly) {
@@ -452,5 +460,9 @@ function _register(name, implementation, wrapOptions) {
   } else {
     shell[name] = wrap(name, implementation, wrapOptions);
   }
+
+  if (wrapOptions.canReceivePipe) {
+    pipeMethods.push(name);
+  }
 }
 exports.register = _register;

test/plugin.js

@@ -39,6 +39,25 @@ test.beforeEach(() => {
   shell.config.resetForTesting();
 });
 
+//
+// Invalids
+//
+
+test('Unable to register a plugin with unknown options', t => {
+  t.throws(() => {
+    plugin.register('foo', fooImplementation, {
+      foobar: true,
+    });
+  }, Error);
+});
+
+test('Unable to register a plugin with wrong option types', t => {
+  t.throws(() => {
+    plugin.register('foo', fooImplementation, {
+      wrapOutput: 'true', // should be a boolean
+    });
+  }, TypeError);
+});
 
 //
 // Valids
@@ -139,10 +158,10 @@ test('Plugins can continue from errors', t => {
   t.is(shell.error(), 'foo: Error, but continuing');
 });
 
-test('Cannot overwrite an existing command by default', t => {
+test('Cannot overwrite an existing command', t => {
   const oldCat = shell.cat;
   t.throws(() => {
     plugin.register('cat', fooImplementation);
-  }, 'unable to overwrite `cat` command');
+  }, 'Command `cat` already exists');
   t.is(shell.cat, oldCat);
 });