Regression in the command line parsing

[dougwilson]

I have been working to upgrade to the latest handlebars where I can (environment supports it), and I ran into an error and tracked it down to a regression in the handling of command line arguments when the same boolean was specified more than once.

For example, in version 4.7.3 and below, handlebars --amd --no-amd would result in the standard output, but in 4.7.4 and 4.7.5 it ends up resulting in AMD output, as if the --no-amd was not there.

The is appearing in some complex tooling internally where we have "default arguments" and some tools just add their own arguments for their own behavior. Think of it like "handlebars --amd ${args} ${template}" and the caller includes --no-amd in the args. This is a simplified example.

Here is the issue distilled down to the commands:

Version 4.7.5 (current):

$ touch empty
$ handlebars -v
4.7.5
$ handlebars --amd --no-amd empty
define(['handlebars.runtime'], function(Handlebars) {
  Handlebars = Handlebars["default"];  var template = Handlebars.template, templates = Handlebars.templates = Handlebars.templates || {};
return templates['empty'] = template({"compiler":[8,">= 4.3.0"],"main":function(container,depth0,helpers,partials,data) {
    return "";
},"useData":true});
});

Version 4.7.3:

$ touch empty
$ handlebars -v
4.7.3
$ handlebars --amd --no-amd empty
(function() {
  var template = Handlebars.template, templates = Handlebars.templates = Handlebars.templates || {};
templates['empty'] = template({"compiler":[8,">= 4.3.0"],"main":function(container,depth0,helpers,partials,data) {
    return "";
},"useData":true});
})();

Note that PR #1672 is a fix for this, liekly because it's just using the same parsing engine as all previous versions (minimist).

/cc @ErisDS

[ErisDS]

Sounds related to yargs/yargs#465

TL;DR there's a parser option duplicate-arguments-array we can play with to see if we can get the original behaviour back.

[ErisDS]

Verified that the following resolves the regression:

 .parserConfiguration({
    'duplicate-arguments-array': false
  })

[ErisDS]

Ok so I've had a go at trying to fix the issue #1674, but this parser config option is too heavy handed.

I wonder if @bcoe could provide some advice here on whether there's some other option I've missed, or if not, what a possible way forward might be, e.g. adding a new parser option? Can maybe move this discussion over to yargs.

Either way the problem is now only on master, is tracked, and we're not in a hurry to release.

I guess it would also be OK to ship with this as a breaking change, but I think we'd all prefer not to.

[bcoe]

👋 I think we fixed a bug for this in yargs@16, I can backport it to 14 this weekend if that helps.

[ErisDS]

@bcoe Oooooh if you can confirm it's fixed in 16 that's 👌 perfect. We've only got yargs on master now and only need support for Node v10 there :)

[dougwilson]

Yea, I was going to mention the same thing: master likely has no reason to not be on yargs 16, so if that works for everyone, then better all-around 🥂

[ErisDS]

Just tested and it works in 15 🙌 , bumping now 👍

[bcoe]

@ErisDS @dougwilson let me know if it still causes issues, and I can add a unit tests (I just seem to remember a fix around this, but might not be fully understanding).

[ErisDS]

Seems to be fixed to me. @dougwilson lemme know if you find any more issues like this 👍