Preset option type discrepancy

[aweebit]

import { Option } from 'commander';

const option = new Option('--add <numbers...>')
  .argParser((value, previous?: number) => {
    return Number(previous) + Number(value);
  })
  .preset(123);
const prop = 'presetArg'; // since currently not declared in typings, see #1972
option.parseArg?.(option[prop] as Parameters<Option['preset']>[0], undefined);

This results in:

error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'string'.

The reason is that the Option.preset() parameter type is set to unknown,

commander.js/typings/index.d.ts

Line 118 in 4ef19fa

preset(arg: unknown): this;

while the parseArg type is designed in such a way that only strings are expected in the first argument.

commander.js/typings/index.d.ts

Line 97 in 4ef19fa

parseArg?: <T>(value: string, previous: T) => T;

This is problematic because custom processing (parseArg) is called for preset option values.

(The reason why the call in the library's source did not result in an error when running the type check that led to #1967 is that the .preset() parameter type is any rather than unknown in lib/option.js:

commander.js/lib/option.js

Lines 62 to 66 in 4ef19fa

	* @param {any} arg
	* @return {Option}
	*/
	preset(arg) {

I suspect this discrepancy is due to an expectation that vanilla JSDoc supports any but not unknown, but actually, there seems to be only one way to declare parameters that accept any arguments in vanilla JSDoc, and that is @param {*}, so unknown could be used here just as well.)

But anyway. My suggestion is that we require that the argument to Option.preset() be a string. Or we change the parseArg signature so that arbitrary values are accepted.

[shadowspawn]

This is as intended, albeit subtle.

Preset was added in #1652. The tests in https://github.com/tj/commander.js/blob/master/tests/options.preset.test.js include tests for string and number values.

There are essentially two usage patterns. The first is setting a preset value without a custom parser, which is a successor to some previous use of .default(). The signature matches .default() with an unknown parameter for this reason.

The second usage pattern is to support calling custom argument parser on preset value. This is different than behaviour for .default() and is an improvement to make it easier to match command-line arguments with preset values. There is a strong expectation that in practice the preset value is a string if there is a custom argument parser.

[aweebit]

There are essentially two usage patterns. The first is setting a preset value without a custom parser, which is a successor to some previous use of .default(). The signature matches .default() with an unknown parameter for this reason.

The second usage pattern is to support calling custom argument parser on preset value. This is different than behaviour for .default() and is an improvement to make it easier to match command-line arguments with preset values. There is a strong expectation that in practice the preset value is a string if there is a custom argument parser.

Thanks for the clear explanation! I would then suggest to also use unknown as the parameter type of .preset() in the JSDoc and add a type assertion in the place where parseArg is called if we decide to embrace stricter typing.

[shadowspawn]

An answer was provided, and no further activity in a month. Closing this as resolved.

Feel free to open a new issue if it comes up again, with new information and renewed interest.