refactor(ts): initial conversion to TypeScript #285

QmarkC · 2020-06-11T15:52:38Z

Second step for #265, "Switch to typescript".
This PR moves and converts most of the root index.js to lib/index.ts.

bcoe · 2020-06-11T18:07:07Z

@QmarkC it seems a little weird that we saw a drop in coverage due to conversion, I'd hope we'd still be exercising the same API surface.

I'll take a closer look towards the end of my work day 👍

QmarkC · 2020-06-11T19:01:30Z

@QmarkC it seems a little weird that we saw a drop in coverage due to conversion, I'd hope we'd still be exercising the same API surface.

I'll take a closer look towards the end of my work day 👍

Most of the new uncovered branches are due to use of optional chaining. I forgot to run coverage before submitting the PR. I'm used to the coverage check being part of test with my other projects. I've already fixed some of them, either by removing unnecessary branches or adding tests. The original commit on this PR was 94.62 % Branch and currently its up to 96.27 % Branch. The threshold is 97% and the previous was 97.79%, so still got a little way to go (~ 5 branches at least) but that's about all I have time for today. The coverage is at 100% for all the other thresholds (lines, functions, and statements).

I also noticed that I was seeing slightly different coverage percentages when running npm run coverage vs what the coverage action is reporting. This ended up being due to difference in node version. The action uses node v13.14.0 and I was on v12.16.1. After I changed nvm to use v13.14.0 the numbers matched.

npm run coverage

-------------------------|---------|----------|---------|---------|---------------------------------
File                     | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
-------------------------|---------|----------|---------|---------|---------------------------------
All files                |     100 |    96.28 |     100 |     100 |
 yargs-parser            |     100 |      100 |     100 |     100 |
  index.js               |     100 |      100 |     100 |     100 |
 yargs-parser/lib        |     100 |    96.24 |     100 |     100 |
  index.ts               |     100 |    96.07 |     100 |     100 | ...,723,750-751,764-769,850,987
  tokenize-arg-string.ts |     100 |      100 |     100 |     100 |
-------------------------|---------|----------|---------|---------|---------------------------------
ERROR: Coverage for branches (96.28%) does not meet global threshold (97%)

coverage action

-------------------------|---------|----------|---------|---------|---------------------------------
File                     | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s               
-------------------------|---------|----------|---------|---------|---------------------------------
All files                |     100 |    96.27 |     100 |     100 |                                 
 yargs-parser            |     100 |      100 |     100 |     100 |                                 
  index.js               |     100 |      100 |     100 |     100 |                                 
 yargs-parser/lib        |     100 |    96.23 |     100 |     100 |                                 
  index.ts               |     100 |    96.05 |     100 |     100 | ...,723,750-751,764-769,850,987 
  tokenize-arg-string.ts |     100 |      100 |     100 |     100 |                                 
-------------------------|---------|----------|---------|---------|---------------------------------
ERROR: Coverage for branches (96.27%) does not meet global threshold (97%)

QmarkC · 2020-06-12T17:23:22Z

@bcoe branch coverage is now 98.93%. The remaining uncovered branches were previously uncovered as well.

-------------------------|---------|----------|---------|---------|---------------------
File                     | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s   
-------------------------|---------|----------|---------|---------|---------------------
All files                |     100 |    98.93 |     100 |     100 |                     
 yargs-parser            |     100 |      100 |     100 |     100 |                     
  index.js               |     100 |      100 |     100 |     100 |                     
 yargs-parser/lib        |     100 |    98.92 |     100 |     100 |                     
  index.ts               |     100 |    98.86 |     100 |     100 | 186,758-759,777,995 
  tokenize-arg-string.ts |     100 |      100 |     100 |     100 |                     
-------------------------|---------|----------|---------|---------|---------------------

mleguen · 2020-06-14T07:43:30Z

lib/yargs-parser-types.ts

+
+export interface Options {
+  /** An object representing the set of aliases for a key: `{ alias: { foo: ['f']} }`. */
+  alias?: Dictionary<string | string[]>;


Options properties should not be all optional, for a better reuse in yargs.

You should use instead Partial<Options> when you need to use all optional options.

@mleguen I'll go ahead and make this change but I'm not sure I agree with it. Perhaps you could expand on how this makes it better for reuse in yargs?

While both methods are practically the same, in my option it is a better practice to explicitly set the properties as optional for this use case. None of the options are required and they all can indeed be undefined. I think using Partial is fine when reusing an existing type and/or there are clear default values, such as with Configuration, but Options doesn't really have clear defaults for all its properties. A quick test with using Partial on the options parse argument but not with the scoped opts variable (like configuration) and setting the option properties to their fallback values (empty objects/arrays/etc...) broke a lot of tests. So we would have to use partial again there or do quite a bit of refactoring in elsewhere. From the perspective of using yargs-parser as a dependency I think the optional properties is clearer on how to interact with the module as well. Also if later an option is required, then all of the Partial<Options> uses will have to be refactored to use explicit optional properties for Options, so the explicit optional properties provides better long term support.

Perhaps you could expand on how this makes it better for reuse in yargs

I find it always easier to add undefined to a type (using ? or Partial) than to remove it.

Yargs builds an internal options structure (including yargs-parser options, but not only), none of it being optional as it deals with defaults by itself. Reusing your Options type would have helped not writing it twice.

But if it too much of a hassle to do so, no problem for me.

mleguen · 2020-06-14T08:07:02Z

@QmarkC Sorry, I don't have a lot of bandwidth at the moment, but I will try to make a decent review of this ASAP.

bcoe · 2020-07-01T05:53:27Z

@QmarkC likewise, I have been low bandwidth this month (I'm hoping to do some coding on the long weekend that's coming up in the USA).

This work is really exciting, I was looking at deno this evening. Along with improving yargs, I think once we're done this work on yargs/yargs-parser, it would be really easy to also provide a library for the deno community which is starting to emerge.

bcoe

@QmarkC this is looking like a great start, I've left a few initial notes. One thing that jumps out at me, is there any approach we could take using generics that would make the return value of the parser more clever. Here's my example code:

import yargsParser from './lib/index.ts'

const argv = yargsParser(Deno.args, {
  string: ['foo']
})

function testStringType (input: string) {
  console.info(typeof input, input)
}

testStringType(argv.foo)

argv.foo ends up having an any type, it would be cool if we were able to:

give the appropriate string type hints, if we've set the string key for the parse.
identified that the key might be undefined, unless demand, or a default value is set.

@mleguen any thoughts on this?

bcoe · 2020-07-02T16:26:42Z

index.js

-const path = require('path')
-const { tokenizeArgString } = require('./build/lib/tokenize-arg-string')
-const util = require('util')
+const { yargsParser } = require('./build/lib')


If we're converting all of index.js to index.ts, my preference would be that we just call this a breaking change and delete index.js.

package.json would then just be updated, such that its main entry point is ./build/index.js.

bcoe · 2020-07-02T16:36:52Z

lib/index.ts

+import camelCase = require('camelcase')
+import decamelize = require('decamelize')


One fringe benefit we get for yargs-parser out of the conversion to TypeScript, is that we can publish a version for deno.land.

Not for this PR, but I think we should vendor camelCase and decamelize. They're both fairly small deps, with 0 dependencies.

bcoe · 2020-07-02T17:24:59Z

lib/yargs-parser-types.ts

+export interface Configuration {
+  /** Should variables prefixed with --no be treated as negations? Default is `true` */
+  'boolean-negation': boolean;
+  /** Should hyphenated arguments be expanded into camel-case aliases? Default is `true` */


these configuration keys should all be optional I believe, when testing I got:

TS2580 [ERROR]: Cannot find name 'process'. Do you need to install type definitions for node? Try `npm i @types/node`. process = process || {env: []}

Until I updated them to be optional.

bcoe · 2020-07-02T17:27:43Z

lib/index.ts

+  return parse(args.slice(), opts)
+}
+
+export { yargsParser }


let's make yargsParser the default export (this goes along with dropping the index.js).

bcoe · 2020-07-16T00:30:55Z

@QmarkC wanted to bump this up, would love to get us over the finish line on some of this TypeScript work 👏

bcoe

I'm going to land this as a starting point, in an effort to keep the TypeScript work moving along.

QmarkC · 2020-07-20T13:24:39Z

I'm going to land this as a starting point, in an effort to keep the TypeScript work moving along.

Sorry for the delay. I had a death in the family. I'll catch up on the out standing items you tagged me in within the next few days.

refactor(ts): move most of yargs-parser to lib/index.ts

7a42cbb

QmarkC requested review from bcoe and mleguen June 11, 2020 15:52

QmarkC added 2 commits June 11, 2020 12:50

refactor(ts): remove unnecessary branches

e6ae5d3

test: add tests for uncovered branches

f5a2dbe

QmarkC added 7 commits June 12, 2020 10:16

refactor: update flags.nargs init to use Object.entries

149bb1d

refactor: update flags.coercions init to use Object.entries

f1d3e0e

refactor: update flags.configs init to use Object.entries

6caf537

refactor: update setConfigObject to check configObjects is not undefined

0b62843

refactor(ts): remove optional chaining for RegExpMatchArrays

b5aece0

refactor(ts): remove unnecessary optional chaining

3560c3b

fix: add missing process check in applyEnvVars

dd1a629

QmarkC mentioned this pull request Jun 12, 2020

Bug: different result for -x1 and -1x when -x and -1 are both booleans #283

Closed

mleguen reviewed Jun 14, 2020

View reviewed changes

QmarkC added 2 commits June 25, 2020 15:42

refactor(ts): use Partial utility type for options

a6c9580

Merge branch 'master' into 265-ts-yargs-parser

acc9c78

bcoe requested changes Jul 2, 2020

View reviewed changes

bcoe approved these changes Jul 18, 2020

View reviewed changes

bcoe changed the title ~~refactor(ts): move and tsify most of yargs-parser to lib/index.ts~~ refactor(ts): initial conversion to TypeScript Jul 18, 2020

bcoe mentioned this pull request Jul 18, 2020

better types for parsed arguments object #291

Closed

Merge branch 'master' into 265-ts-yargs-parser

8f4ce8b

bcoe merged commit 596184f into yargs:master Jul 18, 2020

bcoe mentioned this pull request Jul 19, 2020

refactor(ts): move to TypeScript release of yargs-parser yargs/yargs#1696

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ts): initial conversion to TypeScript #285

refactor(ts): initial conversion to TypeScript #285

QmarkC commented Jun 11, 2020

bcoe commented Jun 11, 2020

QmarkC commented Jun 11, 2020

QmarkC commented Jun 12, 2020

mleguen Jun 14, 2020

QmarkC Jun 25, 2020

mleguen Jun 27, 2020

mleguen commented Jun 14, 2020

bcoe commented Jul 1, 2020

bcoe left a comment

bcoe Jul 2, 2020

bcoe Jul 2, 2020

bcoe Jul 2, 2020

bcoe Jul 2, 2020 •

edited

Loading

bcoe commented Jul 16, 2020

bcoe left a comment

QmarkC commented Jul 20, 2020

		import camelCase = require('camelcase')
		import decamelize = require('decamelize')

refactor(ts): initial conversion to TypeScript #285

refactor(ts): initial conversion to TypeScript #285

Conversation

QmarkC commented Jun 11, 2020

bcoe commented Jun 11, 2020

QmarkC commented Jun 11, 2020

QmarkC commented Jun 12, 2020

mleguen Jun 14, 2020

Choose a reason for hiding this comment

QmarkC Jun 25, 2020

Choose a reason for hiding this comment

mleguen Jun 27, 2020

Choose a reason for hiding this comment

mleguen commented Jun 14, 2020

bcoe commented Jul 1, 2020

bcoe left a comment

Choose a reason for hiding this comment

bcoe Jul 2, 2020

Choose a reason for hiding this comment

bcoe Jul 2, 2020

Choose a reason for hiding this comment

bcoe Jul 2, 2020

Choose a reason for hiding this comment

bcoe Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

bcoe commented Jul 16, 2020

bcoe left a comment

Choose a reason for hiding this comment

QmarkC commented Jul 20, 2020

bcoe Jul 2, 2020 •

edited

Loading