fix(yargs): type 'string' is not assignable to type '"string"'

[cnishina]

Reasoning for change. There is logic built into the interface Options where the property of types must be either the value of "string", "number" , etc. However, the type of the input is just a string. Having the logic built in makes it unusable.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change. (Run with npm test.)
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm run lint package-name (or tsc if no tslint.json is present).
• Increase the version number in the header if appropriate.

[cnishina]

In my project, I have the following error in my CircleCI:

Transpile errors due to

lib/cli/index.ts:62:38 - error TS2345: Argument of type '{ describe: string; default: string; type: string; }' is not assignable to parameter of type 'Options'.
  Types of property 'type' are incompatible.
    Type 'string' is not assignable to type '"string" | "number" | "boolean" | "array" | "count"'.

62       return yargs.option('out_dir', outDir)
                                        ~~~~~~

When applying the fix in the PR to the local node_modules/@types/yargs, the error disappears.

[typescript-bot]

@cnishina Thank you for submitting this PR!

🔔 @poelstra @mizunashi-mana @pushplay @jeffkenney @JimiC - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

[JimiC]

@cnishina I'm afraid this is the wrong approach. The definition for option is option(key: string, options: Options): Argv; . Your outDir should be an Options type and not a string type.

In case outDir is indeed an Options type, the type declaration needs to be literally the word 'string';

[cnishina]

It does follow the Options interface.

const outDir = {
  describe: 'Location of output.',
  default: 'downloads',
  type: 'string'
};

type: 'string' does not work.

Your comment above says my outDir should be an Option and I believe it is. In my use of the options method, I pass a string and an object.

[cnishina]

type is defined as: "array" | "count" | PositionalOptionsType

And the PositionalOptionsType is defined as: type PositionalOptionsType = "boolean" | "number" | "string";

If the value of type can be an "array", "count", "boolean", "number", or "string" then this should be built into the yargs node module logic. Those values are still of type string.

However, if they actually can take on those types, then it should be:

Array<any> | number | boolean | string

[JimiC]

Have a look at the official docs. type needs to be literal.
And we did so in the declaration so we constrain what literal strings are used.

[JimiC]

I do see that using

const outDir = {
  default: 'downloads',
  describe: 'Location of output.',
  type: 'string',
};
yargs.option('out_dir', outDir);

complains but

yargs.option('out_dir', {
  default: 'downloads',
  describe: 'Location of output.',
  type: 'string',
});

is more than fine.

I'll investigate further why the definition complains.

[JimiC]

Now when I use

const outDir: yargs.Options = {
  default: 'downloads',
  describe: 'Location of output.',
  type: 'string',
};
yargs.option('out_dir', outDir);

the complain goes away.

[cnishina]

New error with yargs.Options

It works for outDir. Here is the new error:

const chrome: yargs.Options = {
  describe: 'Install or update chromedriver.',
  default: true,
  type: 'boolean'
};


Type '{ describe: string; defualt: boolean; type: "boolean"; }'
 Is not assignable to type 'Options'.
  Types of property 'type' are incompatible.
   Type '"boolean"' is not assignable to type '"string"'.

Should we cast into yargs.Options just to get an interface to work?

An interface is just an object and you should not have to cast it as yargs.Options. It appears to go away by casting it into yarg.Options but that's probably not a good thing. When calling .option I should not have to do:

.option('foo', { describe: 'foo describe', default: 'bar', type: 'string'}  as yargs.Options)

I really just want to do:

.option('foo', { describe: 'foo describe', default: 'bar', type: 'string'})

[cnishina]

@JimiC @andy-ms Please see comment above.

[JimiC]

.option('foo', { describe: 'foo describe', default: 'bar', type: 'string'}) works fine as I've already mentioned in #28061 (comment).

[cnishina]

Again please review the part about the boolean.

[JimiC]

const chrome: yargs.Options = {
  describe: 'Install or update chromedriver.',
  default: true,
  type: 'boolean'
};
yargs.option('driver', chrome);

also works fine.

[JimiC]

Maybe it's best to use .options. Have a look at how I use it here.

[cnishina]

Well I'm not sure. Let's see, I'm running:

[email protected]
[email protected]
@types/[email protected]

This fails on my computer and circleci.

https://circleci.com/gh/cnishina/webdriver-manager-replacement/128

[cnishina]

Here is me adding the object into the .option method. Again if it is an object described by an interface, I should be able to create it separately. I have also experimented on this and changed the declaration to let instead of a const and that also does not work.

Is your opinion here that this is only my issue and should not be merged? This feels like a lot of push back for my issue that I've investigated and also described.

[cnishina]

I don't think this is right. It should be typed strongly but we should let the yargs package decide what a valid input is for the second argument of yargs.Options. Strongly typed should help the developer write TypeScript. This error again is not obvious. The interface is just an object and when declared, it should not have to be casted just to get TypeScript to work.

[JimiC]

It does because typescript does an internal type check.

[JimiC]

Let me explain further.
This

const test = {
  describe: 'A test case.',
  default: {},
  type: 'object'
};

is a valid variable declaration for typescript, but it's an invalid Options type for yargs.

Now if you write it as

const test: yargs.Options = {
  describe: 'A test case.',
  default: {},
  type: 'object'
};

typescript immediately infers that it's not a valid Options type, and warns you ahead. And it's not valid because there can't be a type: 'object', as it's rescricted by the typings.

Your change, on the other hand, will allow this declaration and it's not valid according to the official yargs docs.

I hope I made myself clear and why your PR is invalid.

[cnishina]

Again, comment above is that interfaces are just objects. When writing out this interface, if I satisfy that the type of type is just a string, then it satisfies the type definition. After it is transpiled, when executing the project, yargs would throw an error during runtime. You are preventing a runtime error with logic in your typings.

This is not productive and I think we need another person to weigh in. @andy-ms. Thoughts?

[JimiC]

@andy-ms is not a member of the authoring team.

[JimiC]

PR is invalid.

[cnishina]

I spoke too soon that it is a runtime error. There is no runtime error so maybe the logic in typings is fine until we fix yargs.

yargs/yargs#1198

[typescript-bot]

@cnishina One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

[cnishina]

Is the revision to close the PR? I think this is the wrong label.

[cnishina]

One last thing:

yargs.option('them_bananas', { type: 'bananas' })

is valid. There's nothing wrong with this... yargs just figures out what type it is when the option is used.

[ghost]

See microsoft/TypeScript#25889 -- string literals need a contextual type to avoid being turned into string.
It might be true that { type: "bananas" } doesn't crash, but neither does Math.abs(true) and we issue an error for that. See https://stackoverflow.com/questions/41750390/what-does-all-legal-javascript-is-legal-typescript-mean

[cnishina]

Since this is my first project with yargs, I was not sure if I was using yargs wrong or not. For the most part, the yargs type definitions did help but things did not go well when I started using the option method. When I received the sort of vague error message, I created the PR to remove the string literals. It took several back and forth messages from the author to nail down that the object created would need to be cast into the interface and not how I was using it. #28061 (comment)

It looks like you have this: microsoft/TypeScript#26413 which will help debugging messages for string literals in interfaces. If the error message said I needed to cast into the yargs.Options then I would be fine.

In its current state, I am still not a fan with the error message but I will close the PR. Thank you @andy-ms for being referee / third party opinion.

Thank you @JimiC for debugging this with me.