Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change to safe storage of options by default, and change action parameters #1409

Merged

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Dec 3, 2020

Pull Request

Problem

  1. The default behaviour is to store option values as properties on the command. This is a frequent cause of problems with clashes with existing properties of Command (and EventEmitter).
  1. In Commander v6 the action handler may get passed various combinations of parameters based on settings and unexpected args:
  • expected args + command with options as properties (most common case)
  • expected args + options
  • expected args + command without options as properties
  • any of above + array of extra args

This has not worked out as nicely as hoped for moving to safe options incrementally! The various combinations make implementations inconsistent, make it hard to write examples that work whatever the settings, and make it harder to write write a generic action handler.

Solution

These are BREAKING changes for many existing programs. But hopefully the code changes required are small and easy!

  1. Store the options values safely by default!! Effectively .storeOptionsAsProperties(false).

This affects especially use of program options.

  1. This PR includes the proposal from WIP: Rework action parameters for more stable interface #1405 to change the action parameters to consistently be options+command.

The array of extra args is gone, but one of the common uses was to show an error for excess/unexpected command-arguments. There is now support for Commander to display an error for excess command-arguments, and this has been turned on by default in this PR. See #1407

The method .passCommandToAction() is gone. No longer needed with this pattern, the options and command are always available as parameters.


Lots of files changed:

  • examples updated
  • example output removed from several example files (replaced with "Try the following")
  • updated tests for new behaviours and deleted routines

ChangeLog

Changed

  • Breaking: options are stored safely by default, not as properties on the command
    • this especially affects accessing options on program, use program.opts()
    • revert behaviour with .storeOptionsAsProperties()
  • Breaking: action handlers are passed options and command separately

Added

  • Breaking: error message if there are too many command-arguments on command line for the action handler
    • if should be allowed then declare extra arguments, or use .allowExcessArguments()

Deleted

  • Breaking: .passCommandToAction()
    • no longer needed as action handler is passed options and command
  • Breaking: "extra arguments" parameter to action handler
    • if being used to detect excess arguments, there is now an error displayed by default

Migration Tips

The biggest change is the parsed option values. Previously the options were stored by default as properties on the command object, and now the options are stored separately.

If you wish to restore the old behaviour and get running quickly you can call .storeOptionsAsProperties().
To allow you to move to the new code patterns incrementally, the action handler will be passed the command twice,
to match the new "options" and "command" parameters (see below).

program options

Use the .opts() method to access the options. This is available on any command but is used most with the program.

program.option('-d, --debug');
program.parse();
// Old code before Commander 7
if (program.debug) console.log(`Program name is ${program.name()}`);
// New code
const options = program.opts();
if (options.debug) console.log(`Program name is ${program.name()}`);

action handler

The action handler gets passed a parameter for each command-argument you declared. Previously by default the next parameter was the command object with the options as properties. Now the next two parameters are instead the options and the command. If you
only accessed the options there may be no code changes required.

program
  .command('compress <filename>')
  .option('-t, --trace')
  // Old code before Commander 7
  .action((filename, cmd) => {
    if (cmd.trace) console.log(`Command name is ${cmd.name()}`);
  });
  // New code
  .action((filename, options, command) => {
    if (options.trace) console.log(`Command name is ${command.name()}`);
  });

If you already set .storeOptionsAsProperties(false) you may still need to adjust your code.

program
  .command('compress <filename>')
  .storeOptionsAsProperties(false)
  .option('-t, --trace')
  // Old code before Commander 7
  .action((filename, command) => {
    if (command.opts().trace) console.log(`Command name is ${command.name()}`);
  });
   // New code
   .action((filename, options, command) => {
      if (command.opts().trace) console.log(`Command name is ${command.name()}`);
   });

excess command-arguments

There is now an error if there are too many command-arguments on the command line (only checked if there is an action handler).
If the extra arguments are supported by your command then you can either declare the expected arguments, or allow excess arguments.

// Old code before Commander 7
program
  .action(() => {});
program.parse(['a', 'b', 'c'], { from: 'user' }); // now causes an error
// New code, declare arguments
program
  .arguments('[args...]')
  .action(() => {});
// New code, allow excess arguments
program
  .allowExcessArguments()
  .action(() => {});

@shadowspawn shadowspawn mentioned this pull request Dec 6, 2020
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Dec 6, 2020

Finished first pass through code and README and examples, including migration tips in the CHANGELOG. I intend to update the Examples section in the README next.

Update: added single command example and tidied multi-command example.

@shadowspawn shadowspawn added this to the v7.0.0 milestone Dec 14, 2020
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Jan 16, 2021
davidjb added a commit to davidjb/clean-css-cli that referenced this pull request May 5, 2021
This change removes mention of the `--level` option which never existed. In versions earlier than 5.0.0, clean-css-cli used to accept the invalid option silently.  With versions > 5.0.0, trying to use the option results in the following: `error: unknown option '--level'`.  The root cause of the change the upgrade to [commander v7.0.0](tj/commander.js#1409), where unknown (excess) command arguments are now raised as an error.
nickebbitt added a commit to nickebbitt/helm-test that referenced this pull request Jun 17, 2021
commander.js changed the way it handles options which broke this.

See:
- https://github.com/tj/commander.js/blob/master/CHANGELOG.md\#changed-2
- tj/commander.js#1409

Co-authored-by: Michael Eves <meves23@gmail.com>
@glensc

This comment was marked as outdated.

@shadowspawn

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants