Opt-in behaviour to avoid name pollution (#1102)

* Add object to hold option values separately from properties on command. Return directly from .opts().

* Add configureCommand, and support for passing options rather than command to action handler

* Restore original opts() implementation when using old configuration

* Use either/or new/old option storage, not both

* Turn version test on again, old behaviour restored

* Add tests for configureCommand, and fix bugs

* Expand .opts tests to include modern configuration

* Add TypeScript and inline documentation for configureCommand

* Switch from modern:boolean to combo:string

* Rework new behaviour with matching named routines.

* Add example files for storeOptionsAsProperties

* Add usage error, and make value default to true for new routines (so simpler call for that case)

* Simpify description

* Add section on avoiding option name clashes

* Do not use else after a return

.eslintignore

@@ -0,0 +1,2 @@
+# We have not configured eslint for TypeScript so avoid bogus error messages in the test file and typings file.
+**/*.ts

.eslintrc.json

@@ -4,6 +4,7 @@
   "rules": {
     "one-var": "off",
     "semi": ["error", "always"],
-    "space-before-function-paren": ["error", "never"]
+    "space-before-function-paren": ["error", "never"],
+    "no-else-return": ["error", { "allowElseIf": false }]
   }
 }

Readme.md

@@ -31,6 +31,7 @@ Read this in other languages: English | [简体中文](./Readme_zh-CN.md)
     - [.help(cb)](#helpcb)
   - [Custom event listeners](#custom-event-listeners)
   - [Bits and pieces](#bits-and-pieces)
+    - [Avoiding option name clashes](#avoiding-option-name-clashes)
     - [TypeScript](#typescript)
     - [Node options such as `--harmony`](#node-options-such-as---harmony)
     - [Node debugging](#node-debugging)
@@ -70,6 +71,8 @@ Options are defined with the `.option()` method, also serving as documentation f
 
 The options can be accessed as properties on the Command object. Multi-word options such as "--template-engine" are camel-cased, becoming `program.templateEngine` etc. Multiple short flags may be combined as a single arg, for example `-abc` is equivalent to `-a -b -c`.
 
+See also optional new behaviour to [avoid name clashes](#avoiding-option-name-clashes).
+
 ### Common option types, boolean and value
 
 The two most used option types are a boolean flag, and an option which takes a value (declared using angle brackets). Both are `undefined` unless specified on command line.
@@ -550,6 +553,43 @@ program.on('command:*', function () {
 
 ## Bits and pieces
 
+### Avoiding option name clashes
+
+The original and default behaviour is that the option values are stored
+as properties on the program, and the action handler is passed a
+command object with the options values stored as properties.
+This is very convenient to code, but the downside is possible clashes with
+existing properties of Command.
+
+There are two new routines to change the behaviour, and the default behaviour may change in the future:
+
+- `storeOptionsAsProperties`: whether to store option values as properties on command object, or store separately (specify false) and access using `.opts()`
+- `passCommandToAction`: whether to pass command to action handler,
+or just the options (specify false)
+
+```js
+// file: ./examples/storeOptionsAsProperties.action.js
+program
+  .storeOptionsAsProperties(false)
+  .passCommandToAction(false);
+
+program
+  .name('my-program-name')
+  .option('-n,--name <name>');
+
+program
+  .command('show')
+  .option('-a,--action <action>')
+  .action((options) => {
+    console.log(options.action);
+  });
+
+program.parse(process.argv);
+
+const programOptions = program.opts();
+console.log(programOptions.name);
+```
+
 ### TypeScript
 
 The Commander package includes its TypeScript Definition file, but also requires the node types which you need to install yourself. e.g.

examples/storeOptionsAsProperties-action.js

@@ -0,0 +1,40 @@
+#!/usr/bin/env node
+
+// To avoid possible name clashes, you can change the default behaviour
+// of storing the options values as properties on the command object.
+// In addition, you can pass just the options to the action handler
+// instead of a commmand object. This allows simpler code, and is more consistent
+// with the previous behaviour so less code changes from old code.
+//
+// Example output:
+//
+// $ node storeOptionsAsProperties-action.js show
+// undefined
+// undefined
+//
+// $ node storeOptionsAsProperties-action.js --name foo show --action jump
+// jump
+// foo
+
+const commander = require('../');
+const program = new commander.Command();
+
+program
+  .storeOptionsAsProperties(false) // <--- change behaviour
+  .passCommandToAction(false); // <--- change behaviour
+
+program
+  .name('my-program-name')
+  .option('-n,--name <name>');
+
+program
+  .command('show')
+  .option('-a,--action <action>')
+  .action((options) => { // <--- passed options, not Command
+    console.log(options.action); // <--- matches old code
+  });
+
+program.parse(process.argv);
+
+const programOptions = program.opts(); // <--- use opts to access option values
+console.log(programOptions.name);

examples/storeOptionsAsProperties-opts.js

@@ -0,0 +1,38 @@
+#!/usr/bin/env node
+
+// To avoid possible name clashes, you can change the default behaviour
+// of storing the options values as properties on the command object.
+// You access the option values using the .opts() function.
+//
+// Example output:
+//
+// $ node storeOptionsAsProperties-opts.js show
+// undefined
+// undefined
+//
+// $ node storeOptionsAsProperties-opts.js --name foo show --action jump
+// jump
+// foo
+
+const commander = require('../');
+const program = new commander.Command();
+
+program
+  .storeOptionsAsProperties(false); // <--- change behaviour
+
+program
+  .name('my-program-name')
+  .option('-n,--name <name>');
+
+program
+  .command('show')
+  .option('-a,--action <action>')
+  .action((cmd) => {
+    const options = cmd.opts(); // <--- use opts to access option values
+    console.log(options.action);
+  });
+
+program.parse(process.argv);
+
+const programOptions = program.opts(); // <--- use opts to access option values
+console.log(programOptions.name);

examples/storeOptionsAsProperties-problem.js

@@ -0,0 +1,35 @@
+#!/usr/bin/env node
+
+// The original and default behaviour is that the option values are stored
+// as properties on the program (Command). The action handler is passed a
+// command object (Command) with the options values also stored as properties.
+// This is very convenient to code, but the downside is possible clashes with
+// existing properties of Command.
+//
+// Example output, note the issues in the first call:
+//
+// $ node storeOptionsAsProperties-problem.js show
+// [Function]
+// [Function]
+//
+// $ node storeOptionsAsProperties-problem.js --name foo show --action jump
+// jump
+// foo
+
+const commander = require('../');
+const program = new commander.Command();
+
+program
+  .name('my-program-name')
+  .option('-n,--name <name>'); // Oops, clash with .name()
+
+program
+  .command('show')
+  .option('-a,--action <action>') // Oops, clash with .action()
+  .action((cmd) => {
+    console.log(cmd.action);
+  });
+
+program.parse(process.argv);
+
+console.log(program.name);

index.js

@@ -126,6 +126,9 @@ function Command(name) {
   this._allowUnknownOption = false;
   this._args = [];
   this._name = name || '';
+  this._optionValues = {};
+  this._storeOptionsAsProperties = true; // backwards compatible by default
+  this._passCommandToAction = true; // backwards compatible by default
 
   this._helpFlags = '-h, --help';
   this._helpDescription = 'output usage information';
@@ -183,6 +186,8 @@ Command.prototype.command = function(nameAndArgs, actionOptsOrExecDesc, execOpts
   cmd._helpShortFlag = this._helpShortFlag;
   cmd._helpLongFlag = this._helpLongFlag;
   cmd._exitCallback = this._exitCallback;
+  cmd._storeOptionsAsProperties = this._storeOptionsAsProperties;
+  cmd._passCommandToAction = this._passCommandToAction;
 
   cmd._executableFile = opts.executableFile; // Custom name for executable file
   this.commands.push(cmd);
@@ -351,7 +356,11 @@ Command.prototype.action = function(fn) {
     // The .action callback takes an extra parameter which is the command itself.
     var expectedArgsCount = self._args.length;
     var actionArgs = args.slice(0, expectedArgsCount);
-    actionArgs[expectedArgsCount] = self;
+    if (self._passCommandToAction) {
+      actionArgs[expectedArgsCount] = self;
+    } else {
+      actionArgs[expectedArgsCount] = self.opts();
+    }
     // Add the extra arguments so available too.
     if (args.length > expectedArgsCount) {
       actionArgs.push(args.slice(expectedArgsCount));
@@ -405,12 +414,12 @@ Command.prototype._optionEx = function(config, flags, description, fn, defaultVa
   if (option.negate || option.optional || option.required || typeof defaultValue === 'boolean') {
     // when --no-foo we make sure default is true, unless a --foo option is already defined
     if (option.negate) {
-      var opts = self.opts();
-      defaultValue = Object.prototype.hasOwnProperty.call(opts, name) ? opts[name] : true;
+      const positiveLongFlag = option.long.replace(/^--no-/, '--');
+      defaultValue = self.optionFor(positiveLongFlag) ? self._getOptionValue(name) : true;
     }
     // preassign only if we have a default
     if (defaultValue !== undefined) {
-      self[name] = defaultValue;
+      self._setOptionValue(name, defaultValue);
       option.defaultValue = defaultValue;
     }
   }
@@ -423,22 +432,22 @@ Command.prototype._optionEx = function(config, flags, description, fn, defaultVa
   this.on('option:' + oname, function(val) {
     // coercion
     if (val !== null && fn) {
-      val = fn(val, self[name] === undefined ? defaultValue : self[name]);
+      val = fn(val, self._getOptionValue(name) === undefined ? defaultValue : self._getOptionValue(name));
     }
 
     // unassigned or boolean value
-    if (typeof self[name] === 'boolean' || typeof self[name] === 'undefined') {
+    if (typeof self._getOptionValue(name) === 'boolean' || typeof self._getOptionValue(name) === 'undefined') {
       // if no value, negate false, and we have a default, then use it!
       if (val == null) {
-        self[name] = option.negate
+        self._setOptionValue(name, option.negate
           ? false
-          : defaultValue || true;
+          : defaultValue || true);
       } else {
-        self[name] = val;
+        self._setOptionValue(name, val);
       }
     } else if (val !== null) {
       // reassign
-      self[name] = option.negate ? false : val;
+      self._setOptionValue(name, option.negate ? false : val);
     }
   });
 
@@ -531,6 +540,69 @@ Command.prototype.allowUnknownOption = function(arg) {
   return this;
 };
 
+/**
+  * Whether to store option values as properties on command object,
+  * or store separately (specify false). In both cases the option values can be accessed using .opts().
+  *
+  * @param {boolean} value
+  * @return {Command} Command for chaining
+  * @api public
+  */
+
+Command.prototype.storeOptionsAsProperties = function(value) {
+  this._storeOptionsAsProperties = (value === undefined) || value;
+  if (this.options.length) {
+    // This is for programmer, not end user.
+    console.error('Commander usage error: call storeOptionsAsProperties before adding options');
+  }
+  return this;
+};
+
+/**
+  * Whether to pass command to action handler,
+  * or just the options (specify false).
+  *
+  * @param {boolean} value
+  * @return {Command} Command for chaining
+  * @api public
+  */
+
+Command.prototype.passCommandToAction = function(value) {
+  this._passCommandToAction = (value === undefined) || value;
+  return this;
+};
+
+/**
+ * Store option value
+ *
+ * @param {String} key
+ * @param {Object} value
+ * @api private
+ */
+
+Command.prototype._setOptionValue = function(key, value) {
+  if (this._storeOptionsAsProperties) {
+    this[key] = value;
+  } else {
+    this._optionValues[key] = value;
+  }
+};
+
+/**
+ * Retrieve option value
+ *
+ * @param {String} key
+ * @return {Object} value
+ * @api private
+ */
+
+Command.prototype._getOptionValue = function(key) {
+  if (this._storeOptionsAsProperties) {
+    return this[key];
+  }
+  return this._optionValues[key];
+};
+
 /**
  * Parse `argv`, settings options and invoking commands when defined.
  *
@@ -843,7 +915,7 @@ Command.prototype._checkForMissingMandatoryOptions = function() {
   // Walk up hierarchy so can call from action handler after checking for displaying help.
   for (var cmd = this; cmd; cmd = cmd.parent) {
     cmd.options.forEach((anOption) => {
-      if (anOption.mandatory && (cmd[anOption.attributeName()] === undefined)) {
+      if (anOption.mandatory && (cmd._getOptionValue(anOption.attributeName()) === undefined)) {
         cmd.missingMandatoryOptionValue(anOption);
       }
     });
@@ -936,14 +1008,19 @@ Command.prototype.parseOptions = function(argv) {
  * @api public
  */
 Command.prototype.opts = function() {
-  var result = {},
-    len = this.options.length;
-
-  for (var i = 0; i < len; i++) {
-    var key = this.options[i].attributeName();
-    result[key] = key === this._versionOptionName ? this._version : this[key];
+  if (this._storeOptionsAsProperties) {
+    // Preserve original behaviour so backwards compatible when still using properties
+    var result = {},
+      len = this.options.length;
+
+    for (var i = 0; i < len; i++) {
+      var key = this.options[i].attributeName();
+      result[key] = key === this._versionOptionName ? this._version : this[key];
+    }
+    return result;
   }
-  return result;
+
+  return this._optionValues;
 };
 
 /**