argument 'once' (and probably others) cause naming collision

[hotaru355]

Just want to give some feedback here to help others avoid a little pitfall: you cannot use --once as an argument, probably because the name collides with some internals of commander. Interestingly, the once argument will revert the expected behavior: myprog --once will set commander.once=undefined, so a falsy value, while running myprog will set it to a function as expected. Maybe you guys can fix the documentation to mention these reserved arguments, so others can save their time. Thanks!

[tonylukasavage]

There's a handful of these since when you create and parse options, they are placed directly on the program object, which an EventEmitter. This means you'd also collide with anything on the Command object/prototype contained with commander.js. Frankly, there's lots of them and you'd have to check the source to know exactly which ones, though some of the ones that are likely to cause collisions are action, option, command, opts, and JS internals like apply and call.

A while ago I submitted #262 that added the opts() function to return the set of options defined on the program object in order to return options cleanly and reduce the number of possible collisions. This was only a first step though. The change that would reduce the collisions for good would be then no longer writing the options back onto the program object after parsing. This, IMO, is unlikely to happen since it would be a major breaking change, fundamentally changing how commander is used.

As far as the docs, I'm sure if someone submits an accurate PR I'm sure it would be considered for a caveats section or something similar.

[tonylukasavage]

So I hacked together a script that will list every possible collision (I think). Note that these are possible collisions. For example, Function.name is never used in commander.js, so while name is on the list, it won't be a problem. If Function.name is ever used, it would then become a real conflict, not just a possible one.

Got the getAllPropertyNames function on this SO post

function getAllPropertyNames( obj ) {
    var props = [];
    do {
        props= props.concat(Object.getOwnPropertyNames( obj ));
    } while ( obj = Object.getPrototypeOf( obj ) );
    return props;
}

function unique(val, index, array) { 
    return array.indexOf(val) === index;
}

var program = require('commander');
program.parse(['node','test']);
getAllPropertyNames(program.Command).concat(getAllPropertyNames(program)).sort().filter(unique);

And right now, you'll come out with a list like this:

[ 'Command',
  'Option',
  '__defineGetter__',
  '__defineSetter__',
  '__lookupGetter__',
  '__lookupSetter__',
  '__proto__',
  '_allowUnknownOption',
  '_args',
  '_events',
  '_execs',
  '_maxListeners',
  '_name',
  'action',
  'addImplicitHelpCommand',
  'addListener',
  'alias',
  'allowUnknownOption',
  'apply',
  'args',
  'arguments',
  'bind',
  'call',
  'caller',
  'command',
  'commandHelp',
  'commands',
  'constructor',
  'description',
  'domain',
  'emit',
  'executeSubCommand',
  'hasOwnProperty',
  'help',
  'helpInformation',
  'isPrototypeOf',
  'largestOptionLength',
  'length',
  'listeners',
  'missingArgument',
  'name',
  'normalize',
  'on',
  'once',
  'option',
  'optionFor',
  'optionHelp',
  'optionMissingArgument',
  'options',
  'opts',
  'outputHelp',
  'parse',
  'parseArgs',
  'parseExpectedArgs',
  'parseOptions',
  'propertyIsEnumerable',
  'prototype',
  'rawArgs',
  'removeAllListeners',
  'removeListener',
  'setMaxListeners',
  'toLocaleString',
  'toString',
  'unknownOption',
  'usage',
  'valueOf',
  'variadicArgNotLast',
  'version' ]

Perhaps a @zhiyelee could weigh in on how we might want to document this.

[zhiyelee]

@tonylukasavage thank you! So long a list, I really think a v3.0 version is needed to fix this problem.

[hotaru355]

Thank you very much for the prompt and detailed response, it's really great to see active projects like this.
I cannot speak to what went into the initial design decision of attaching the options to the commander object itself, probably ease of use was one thought. But I would do my +1 here for a design break in the next major version to use a plain object that gets the options attached, something like commander.commands or commander.opts.

[ohEmily]

I also ran into this problem. @tonylukasavage's fix is very helpful (thank you!) But +1 to @hotaru355's design change proposal.

[loveencounterflow]

+1 for a breaking change in v3. The list of conflicts is just ridiculously long. IMHO it's always a mistake to throw together API/methods and business data in one namespace, and even if you do that, you should not allow user data with arbitrary names on your API objects at any rate.

[royling]

While I was reading the examples in the doc, the first question jumped into my head was this collision problem, considering the parsed results are all added to the commander object itself. so 👍 to the breaking change in the new version.

[ericuldall]

My pull request #515 seems to fix this behavior as the user defined options are added to Commander._data instead

Unfortunately this will make it a major release as reverse compatibility would be tricky

[rijnhard]

Same issue in #584

Do we need an _data property when you can just use opts()? although calling opts() each time will be less efficient.

[bySabi]

I got bitten too! :) #648

I created a workaround module for this problem. Tomorrow will be published.