Allow readonly array as parameter of .choices() (#1667)

* Allow readonly array as parameter of .choices()

Without `readonly`, readonly arrays cannot be passed as parameters.

Example: https://www.typescriptlang.org/play?#code/GYVwdgxgLglg9mABMAjACgIYCcsC5EDOUWMYA5gNoC6AlIgN4C+AUKJLAsgEyY75YBTDABMEAGwCehYqUq0GLZhARFE2LCkQBeRBQDkGPVQDcSlVDU4u23QaNqCiZWCKnW6dShqngPT9-deLC4A3yCQ4yA

* Add tests for .choices() typings

* Take a defensive copy of .choices argument to make sure it's readonly

* Expand choices tests to cover readonly contract (#1)

* Use the defensive copy of the parameter of .choices() in parseArg

Co-authored-by: John Gee <[email protected]>

lib/argument.js

@@ -97,10 +97,10 @@ class Argument {
    */
 
   choices(values) {
-    this.argChoices = values;
+    this.argChoices = values.slice();
     this.parseArg = (arg, previous) => {
-      if (!values.includes(arg)) {
-        throw new InvalidArgumentError(`Allowed choices are ${values.join(', ')}.`);
+      if (!this.argChoices.includes(arg)) {
+        throw new InvalidArgumentError(`Allowed choices are ${this.argChoices.join(', ')}.`);
       }
       if (this.variadic) {
         return this._concatValue(arg, previous);

lib/option.js

@@ -135,10 +135,10 @@ class Option {
    */
 
   choices(values) {
-    this.argChoices = values;
+    this.argChoices = values.slice();
     this.parseArg = (arg, previous) => {
-      if (!values.includes(arg)) {
-        throw new InvalidArgumentError(`Allowed choices are ${values.join(', ')}.`);
+      if (!this.argChoices.includes(arg)) {
+        throw new InvalidArgumentError(`Allowed choices are ${this.argChoices.join(', ')}.`);
       }
       if (this.variadic) {
         return this._concatValue(arg, previous);

tests/argument.choices.test.js

@@ -0,0 +1,59 @@
+const commander = require('../');
+
+test('when command argument in choices then argument set', () => {
+  const program = new commander.Command();
+  let shade;
+  program
+    .exitOverride()
+    .addArgument(new commander.Argument('<shade>').choices(['red', 'blue']))
+    .action((shadeParam) => { shade = shadeParam; });
+  program.parse(['red'], { from: 'user' });
+  expect(shade).toBe('red');
+});
+
+test('when command argument is not in choices then error', () => {
+  // Lightweight check, more detailed testing of behaviour in command.exitOverride.test.js
+  const program = new commander.Command();
+  program
+    .exitOverride()
+    .configureOutput({
+      writeErr: () => {}
+    })
+    .addArgument(new commander.Argument('<shade>').choices(['red', 'blue']));
+  expect(() => {
+    program.parse(['orange'], { from: 'user' });
+  }).toThrow();
+});
+
+describe('choices parameter is treated as readonly, per TypeScript declaration', () => {
+  test('when choices called then parameter does not change', () => {
+    // Unlikely this could break, but check the API we are declaring in TypeScript.
+    const original = ['red', 'blue', 'green'];
+    const param = original.slice();
+    new commander.Argument('<shade>').choices(param);
+    expect(param).toEqual(original);
+  });
+
+  test('when choices called and argChoices later changed then parameter does not change', () => {
+    const original = ['red', 'blue', 'green'];
+    const param = original.slice();
+    const argument = new commander.Argument('<shade>').choices(param);
+    argument.argChoices.push('purple');
+    expect(param).toEqual(original);
+  });
+
+  test('when choices called and parameter changed the choices does not change', () => {
+    const program = new commander.Command();
+    const param = ['red', 'blue'];
+    program
+      .exitOverride()
+      .configureOutput({
+        writeErr: () => {}
+      })
+      .addArgument(new commander.Argument('<shade>').choices(param));
+    param.push('orange');
+    expect(() => {
+      program.parse(['orange'], { from: 'user' });
+    }).toThrow();
+  });
+});

tests/argument.custom-processing.test.js

@@ -205,15 +205,3 @@ test('when custom processing for argument throws plain error then not CommanderE
   expect(caughtErr).toBeInstanceOf(Error);
   expect(caughtErr).not.toBeInstanceOf(commander.CommanderError);
 });
-
-// this is the happy path, testing failure case in command.exitOverride.test.js
-test('when argument argument in choices then argument set', () => {
-  const program = new commander.Command();
-  let shade;
-  program
-    .exitOverride()
-    .addArgument(new commander.Argument('<shade>').choices(['red', 'blue']))
-    .action((shadeParam) => { shade = shadeParam; });
-  program.parse(['red'], { from: 'user' });
-  expect(shade).toBe('red');
-});

tests/options.choices.test.js

@@ -0,0 +1,57 @@
+const commander = require('../');
+
+test('when option argument in choices then option set', () => {
+  const program = new commander.Command();
+  program
+    .exitOverride()
+    .addOption(new commander.Option('--colour <shade>').choices(['red', 'blue']));
+  program.parse(['--colour', 'red'], { from: 'user' });
+  expect(program.opts().colour).toBe('red');
+});
+
+test('when option argument is not in choices then error', () => {
+  // Lightweight check, more detailed testing of behaviour in command.exitOverride.test.js
+  const program = new commander.Command();
+  program
+    .exitOverride()
+    .configureOutput({
+      writeErr: () => {}
+    })
+    .addOption(new commander.Option('--colour <shade>').choices(['red', 'blue']));
+  expect(() => {
+    program.parse(['--colour', 'orange'], { from: 'user' });
+  }).toThrow();
+});
+
+describe('choices parameter is treated as readonly, per TypeScript declaration', () => {
+  test('when choices called then parameter does not change', () => {
+    // Unlikely this could break, but check the API we are declaring in TypeScript.
+    const original = ['red', 'blue', 'green'];
+    const param = original.slice();
+    new commander.Option('--colour <shade>').choices(param);
+    expect(param).toEqual(original);
+  });
+
+  test('when choices called and argChoices later changed then parameter does not change', () => {
+    const original = ['red', 'blue', 'green'];
+    const param = original.slice();
+    const option = new commander.Option('--colour <shade>').choices(param);
+    option.argChoices.push('purple');
+    expect(param).toEqual(original);
+  });
+
+  test('when choices called and parameter changed the choices does not change', () => {
+    const program = new commander.Command();
+    const param = ['red', 'blue'];
+    program
+      .exitOverride()
+      .configureOutput({
+        writeErr: () => {}
+      })
+      .addOption(new commander.Option('--colour <shade>').choices(param));
+    param.push('orange');
+    expect(() => {
+      program.parse(['--colour', 'orange'], { from: 'user' });
+    }).toThrow();
+  });
+});

tests/options.custom-processing.test.js

@@ -98,16 +98,6 @@ test('when option specified multiple times then callback called with value and p
   expect(mockCoercion).toHaveBeenNthCalledWith(2, '2', 'callback');
 });
 
-// this is the happy path, testing failure case in command.exitOverride.test.js
-test('when option argument in choices then option set', () => {
-  const program = new commander.Command();
-  program
-    .exitOverride()
-    .addOption(new commander.Option('--colour <shade>').choices(['red', 'blue']));
-  program.parse(['--colour', 'red'], { from: 'user' });
-  expect(program.opts().colour).toBe('red');
-});
-
 // Now some functional tests like the examples in the README!
 
 test('when parseFloat "1e2" then value is 100', () => {

typings/index.d.ts

@@ -61,7 +61,7 @@ export class Argument {
   /**
    * Only allow argument value to be one of choices.
    */
-  choices(values: string[]): this;
+  choices(values: readonly string[]): this;
 
   /**
    * Make argument required.
@@ -140,7 +140,7 @@ export class Option {
   /**
    * Only allow option value to be one of choices.
    */
-  choices(values: string[]): this;
+  choices(values: readonly string[]): this;
 
   /**
    * Return option name.

typings/index.test-d.ts

@@ -394,6 +394,7 @@ expectType<commander.Option>(baseOption.hideHelp(false));
 
 // choices
 expectType<commander.Option>(baseOption.choices(['a', 'b']));
+expectType<commander.Option>(baseOption.choices(['a', 'b'] as const));
 
 // name
 expectType<string>(baseOption.name());
@@ -425,6 +426,7 @@ expectType<commander.Argument>(baseArgument.argParser((value: string, previous:
 
 // choices
 expectType<commander.Argument>(baseArgument.choices(['a', 'b']));
+expectType<commander.Argument>(baseArgument.choices(['a', 'b'] as const));
 
 // argRequired
 expectType<commander.Argument>(baseArgument.argRequired());