search

twolfson / commander-completion

Shell completion for commander.js
The Unlicense
26 stars 4 forks source link

Not fully compatible with commander #7

Closed tkarls closed 7 years ago

tkarls commented 7 years ago

Just putting it here as reference for others.

Commander completion is not fully compatible with commander. So you cannot expect everything to work if just switching commander to commander-competion.

What I've found so far: Not possible to use the allowUnknownOption() Coercion with a collect function

function collect(val, memo) {
  memo.push(val);
  return memo;
}

program
    .command('sign-certificates')
    .description('Signs TLS certificates for the specified domain names ans installs them')
    .option('--fqdn <fqdn>', "Fully qualified domain name. Can be repeated", collect, [])

does not work. When commander completion is used the collect function is called with 'memo' set to undefined.

There may be more things, but this is what I 've found.

twolfson commented 7 years ago

Can you elaborate on allowUnknownOption()? Is there expected behavior or is it erroring out?

With respect to coercion, I think this was negligence -- it's hard to predict all cases upfront so I build for what I know and expect the rest to come in issues (like this one). I'm actually surprised Commander supports multiple options. Iirc, I had to switch to yargs to get support for:

--fqdn fqdn1 fqdn2 fqdn3

as I hated the syntax of

--fqdn fqdn1 --fqdn fqdn2 --fqdn fqdn3
tkarls commented 7 years ago

Yes, I totally understand not everything can be done from the start!

With respect to the allowUnknownOption() I get:

TypeError: program.command(...).allowUnknownOption is not a function
    at Object.<anonymous> (c:\work\gitwork\headend_install_cli\index.js:97:6)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Timeout.Module.runMain [as _onTimeout] (module.js:605:10)
    at ontimeout (timers.js:365:14)
    at tryOnTimeout (timers.js:237:5)
    at Timer.listOnTimeout (timers.js:207:5)

when used like:

program
    .command('sign-certificates')
    .allowUnknownOption()
    .description('Signs TLS certificates for the specified domain names ans installs them')
    .option('--fqdn <fqdn>', "Fully qualified domain name. Can be repeated", collect, [])
twolfson commented 7 years ago

Ah, I think this is being caused by us loading our own dependency instead of allowing someone to pass in their own commander instance. It looks like we exposed a mixin function so a workaround is possible via:

// In program using commander-completion
// Load in our dependencies
var program = require('commander');
var commanderCompletion = require('commander-completion');

// Mixin our completion logic to `commander`
commanderCompletion.mixinCompletable(program.Command.prototype);

// Use `commander` as per usual
program.command('sign-certificates')...

https://github.com/twolfson/commander-completion/blob/0.4.1/lib/commander-completion.js#L116-L125

As for long term solutions, we should probably redesign the API to always receive commander itself:

var program = require('commander-completion')(require('commander'));
program.command('sign-certificates')...
twolfson commented 7 years ago

Going to take a shot at updating the package to support allowUnknownOption

twolfson commented 7 years ago

Alright, allowUnknownOption should now work in both an upgraded direct version of Commander.js (done in 0.5.0) as well as our removal of directly pointing to Commander.js (done in 1.0.0)

I'm going to take a shot at reproducing/triaging the multiple option issue now but I have a feeling I'll need a deeper example

twolfson commented 7 years ago

We completed a test case to reproduce the issue but it seems to be completing everything just fine:

https://github.com/twolfson/commander-completion/blob/0e2f2e3c01f70209ea53f0d1b415f4d76a7682d8/test/commander-completion_test.js#L167-L199

Could you verify the issue is still occurring in 1.0.0 and if so, provide us example input where it fails?

tkarls commented 7 years ago

Wow, thanks for your support!

I will look a bit closer on it. I'll update you soon.

tkarls commented 7 years ago

Ok, first I tested with the original version to confirm the problem was there and it was. Then I upgraded to 1.0.0 and using the syntax:

var program = require('commander-completion')(require('commander'));

With this version the problem goes away and repeated values are collected just fine!

I can also verify that the allowUnknownOption() now works for me.

Great stuff, thanks.

twolfson commented 7 years ago

Great! Thanks for the quick responses and thorough bug reports =) I will close this issue as it seems resolved

I'm going to leave #6 as "help wanted" as it has high potential to be a time sink and I don't have much of that to spare =/