Skip unknown options check if there is a better error to display (#1464)

* Check for uknown options in multiple places, to allow skipping check

* Missing command shows help, does not check for options

* Check unknown commands before unknown options

* Add comments

* Restore legacy command:* handling

* Add more tests for legacy command('*')

* Add comment explaining history for test

* Improve comment

* Test for command suggestion despite unknown option

* Improve comments and suppress test output

* Add test for competing unknown command and option

* Test that unknown option is an error in simple program

* Fix test and comment

Author: shadowspawn

index.js

@@ -1475,12 +1475,17 @@ class Command extends EventEmitter {
 
       outputHelpIfRequested(this, parsed.unknown);
       this._checkForMissingMandatoryOptions();
-      if (parsed.unknown.length > 0) {
-        this.unknownOption(parsed.unknown[0]);
-      }
+
+      // We do not always call this check to avoid masking a "better" error, like unknown command.
+      const checkForUnknownOptions = () => {
+        if (parsed.unknown.length > 0) {
+          this.unknownOption(parsed.unknown[0]);
+        }
+      };
 
       const commandEvent = `command:${this.name()}`;
       if (this._actionHandler) {
+        checkForUnknownOptions();
         // Check expected arguments and collect variadic together.
         const args = this.args.slice();
         this._args.forEach((arg, i) => {
@@ -1498,19 +1503,24 @@ class Command extends EventEmitter {
         this._actionHandler(args);
         if (this.parent) this.parent.emit(commandEvent, operands, unknown); // legacy
       } else if (this.parent && this.parent.listenerCount(commandEvent)) {
+        checkForUnknownOptions();
         this.parent.emit(commandEvent, operands, unknown); // legacy
       } else if (operands.length) {
-        if (this._findCommand('*')) { // legacy
+        if (this._findCommand('*')) { // legacy default command
           this._dispatchSubcommand('*', operands, unknown);
         } else if (this.listenerCount('command:*')) {
+          // skip option check, emit event for possible misspelling suggestion
           this.emit('command:*', operands, unknown);
         } else if (this.commands.length) {
           this.unknownCommand();
+        } else {
+          checkForUnknownOptions();
         }
       } else if (this.commands.length) {
         // This command has subcommands and nothing hooked up at this level, so display help.
         this.help({ error: true });
       } else {
+        checkForUnknownOptions();
         // fall through for caller to handle after calling .parse()
       }
     }

tests/command.asterisk.test.js

@@ -64,6 +64,45 @@ describe(".command('*')", () => {
     program.parse(['node', 'test', 'unrecognised-command']);
     expect(mockAction).toHaveBeenCalled();
   });
+
+  test('when unrecognised argument and known option then asterisk action called', () => {
+    // This tests for a regression between v4 and v5. Known default option should not be rejected by program.
+    const mockAction = jest.fn();
+    const program = new commander.Command();
+    program
+      .command('install');
+    const star = program
+      .command('*')
+      .arguments('[args...]')
+      .option('-d, --debug')
+      .action(mockAction);
+    program.parse(['node', 'test', 'unrecognised-command', '--debug']);
+    expect(mockAction).toHaveBeenCalled();
+    expect(star.opts().debug).toEqual(true);
+  });
+
+  test('when non-command argument and unknown option then error for unknown option', () => {
+    // This is a change in behaviour from v2 which did not error, but is consistent with modern better detection of invalid options
+    const mockAction = jest.fn();
+    const program = new commander.Command();
+    program
+      .exitOverride()
+      .configureOutput({
+        writeErr: () => {}
+      })
+      .command('install');
+    program
+      .command('*')
+      .arguments('[args...]')
+      .action(mockAction);
+    let caughtErr;
+    try {
+      program.parse(['node', 'test', 'some-argument', '--unknown']);
+    } catch (err) {
+      caughtErr = err;
+    }
+    expect(caughtErr.code).toEqual('commander.unknownOption');
+  });
 });
 
 // Test .on explicitly rather than assuming covered by .command
@@ -108,4 +147,18 @@ describe(".on('command:*')", () => {
     program.parse(['node', 'test', 'unrecognised-command']);
     expect(mockAction).toHaveBeenCalled();
   });
+
+  test('when unrecognised command/argument and unknown option then listener called', () => {
+    // Give listener a chance to make a suggestion for misspelled command. The option
+    // could only be unknown because the command is not correct.
+    // Regression identified in https://github.com/tj/commander.js/issues/1460#issuecomment-772313494
+    const mockAction = jest.fn();
+    const program = new commander.Command();
+    program
+      .command('install');
+    program
+      .on('command:*', mockAction);
+    program.parse(['node', 'test', 'intsall', '--unknown']);
+    expect(mockAction).toHaveBeenCalled();
+  });
 });

tests/command.unknownCommand.test.js

@@ -1,6 +1,6 @@
 const commander = require('../');
 
-describe('unknownOption', () => {
+describe('unknownCommand', () => {
   // Optional. Use internal knowledge to suppress output to keep test output clean.
   let writeErrorSpy;
 
@@ -63,6 +63,22 @@ describe('unknownOption', () => {
     expect(caughtErr.code).toBe('commander.unknownCommand');
   });
 
+  test('when unknown command and unknown option then error is for unknown command', () => {
+    //  The unknown command is more useful since the option is for an unknown command (and might be
+    // ok if the command had been correctly spelled, say).
+    const program = new commander.Command();
+    program
+      .exitOverride()
+      .command('sub');
+    let caughtErr;
+    try {
+      program.parse('node test.js sbu --silly'.split(' '));
+    } catch (err) {
+      caughtErr = err;
+    }
+    expect(caughtErr.code).toBe('commander.unknownCommand');
+  });
+
   test('when unknown subcommand then help suggestion includes command path', () => {
     const program = new commander.Command();
     program

tests/command.unknownOption.test.js

@@ -82,4 +82,17 @@ describe('unknownOption', () => {
     }
     expect(caughtErr.code).toBe('commander.unknownOption');
   });
+
+  test('when specify unknown option with simple program then error', () => {
+    const program = new commander.Command();
+    program
+      .exitOverride();
+    let caughtErr;
+    try {
+      program.parse(['node', 'test', '--NONSENSE']);
+    } catch (err) {
+      caughtErr = err;
+    }
+    expect(caughtErr.code).toBe('commander.unknownOption');
+  });
 });