chore(ts): move and tsify most of root yargs.js to lib/yargs #1670

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

3rd and 4th steps of #1586: this PR moves and tsifies most of the root yargs.js to lib/yargs, and fixes some bugs discovered on the way (see separate bugfix commits).

To keep consistent with other parsing hint functions.

A single argsert signature for multi-signature functions is not enough to cover all cases.

bcoe

This looks like a wonderful start to me. The behavior of the generics is complex enough that it will be hard for me to fully understand some of the decisions you've made until we add a suite of tests for the expected behavior. I'm fine with this suite of tests being added when we finish the conversion.

Perhaps we can add a test file along the lines of **test/typescript-interface.ts** that just exercises a wide variety of expectations one might have, using the typed version of yargs?

package.json

lib/yargs.ts

bcoe · 2020-06-03T23:56:28Z

@QmarkC could I bother you for review on this one?

bcoe · 2020-06-10T00:18:33Z

Just wanted to bring this PR up again, @QmarkC if you have any feedback let us know 👍

@mleguen otherwise I'm happy to land this.

QmarkC

I wasn't able to do a deep dive into this right now but I left a few comments from an initial review.

QmarkC · 2020-06-09T21:30:04Z

lib/apply-extends.ts

@@ -32,7 +32,7 @@ function mergeDeep (config1: Dictionary, config2: Dictionary) {
  return target
 }

-export function applyExtends (config: Dictionary, cwd: string, mergeExtends: boolean) {
+export function applyExtends (config: Dictionary, cwd: string, mergeExtends?: boolean) {


I would prefer using the Configuration interface over the more generic Dictionary type.
I think mergeExtends would be cleaner by setting a default value of false rather than marking it as an optional parameter.
A return type would be nice too.

applyExtends does not only handle Configuration objects (yargs-parser and yargs config), but also more generic app config. So using Configuration would be too restrictive here.

I think mergeExtends would be cleaner by setting a default value of false rather than marking it as an optional parameter.
A return type would be nice too.

Committed

QmarkC · 2020-06-09T21:38:08Z

lib/command.ts

@@ -1,4 +1,4 @@
-import { Dictionary, assertNotUndefined, assertNotTrue } from './common-types'
+import { Dictionary, assertNotStrictEqual } from './common-types'


Should we use import type? That's a TypeScript 3.8+ feature. The current semver used for typescript is ^3.7.0, which resolves to 3.9.3. So it might be a good idea to bump that for clarity.

As I understood it, the main idea behind import type is to be able to import type only without importing real js stuff, nor executing any module initialization code (as the import disappears from the resulting js code).

As long as we do not have such an issue, would this really be a clarification?

Also note that:

regular imports only importing interfaces (as are most of our type-only imports, until we move to classes) are already removed from the resulting js code

the specific ./common-types import you point at cannot be a type import, as it also imports a helper function

I think this may however make sense in a future work when we will work on replacing self stuff by real classes (to import only class types, and not the classes themselves).

lib/common-types.ts

lib/middleware.ts

lib/typings/yargs-parser.d.ts

QmarkC · 2020-06-09T23:21:58Z

lib/typings/yargs-parser.d.ts

+    interface Options {
+        /** An object representing the set of aliases for a key: `{ alias: { foo: ['f']} }`. */
+        alias: { [key: string]: string | string[] };
+        /**
+         * Indicate that keys should be parsed as an array: `{ array: ['foo', 'bar'] }`.
+         * Indicate that keys should be parsed as an array and coerced to booleans / numbers:
+         * { array: [ { key: 'foo', boolean: true }, {key: 'bar', number: true} ] }`.
+         */
+        array: string[] | Array<{ key: string; boolean?: boolean, number?: boolean }>;
+        /** Arguments should be parsed as booleans: `{ boolean: ['x', 'y'] }`. */
+        boolean: string[];
+        /**
+         * Provide a custom synchronous function that returns a coerced value from the argument provided (or throws an error), e.g.
+         * `{ coerce: { foo: function (arg) { return modifiedArg } } }`.
+         */
+        coerce: { [key: string]: CoerceCallback };
+        /** Indicate a key that represents a path to a configuration file (this file will be loaded and parsed). */
+        config: string | string[] | { [key: string]: ConfigCallback | boolean };
+        /** Provide configuration options to the yargs-parser. */
+        configuration: Partial<Configuration>;
+        /** Indicate a key that should be used as a counter, e.g., `-vvv = {v: 3}`. */
+        count: string[];
+        /** Provide default values for keys: `{ default: { x: 33, y: 'hello world!' } }`. */
+        default: { [key: string]: any };
+        /** Environment variables (`process.env`) with the prefix provided should be parsed. */
+        envPrefix?: string;
+        /** Specify that a key requires n arguments: `{ narg: {x: 2} }`. */
+        narg: { [key: string]: number };
+        /** `path.normalize()` will be applied to values set to this key. */
+        normalize: string[];
+        /** Keys should be treated as numbers. */
+        number: string[];
+        /** Keys should be treated as strings (even if they resemble a number `-x 33`). */
+        string: string[];
+    }
+
+    interface CoerceCallback {
+        (arg: any): any
+    }
+
+    interface ConfigCallback {
+        (configPath: string): { [key: string]: any }
+    }
+
+    interface Parser {
+        (argv: string | string[], opts?: Partial<Options>): Arguments;
+        detailed(argv: string | string[], opts?: Partial<Options>): DetailedArguments;
+    }


All of the options should be optional.

Options is missing configObjects, __, and key properties.

Missing types on a few of the other options.

ConfigCallback can also return an Error

Update Parser interface to remove the Partial wrapper around Options.

Parser args can at least be string | (string | number)[] but we landed on using string | any[] in the yargs-parser PR to ensure there would not be a breaking change.

type ArrayOption = string | { key: string; string?: boolean, boolean?: boolean, number?: boolean, integer?: boolean }; type CoerceCallback = (arg: any) => any; type ConfigCallback = (configPath: string) => { [key: string]: any } | Error; interface Options { /** An object representing the set of aliases for a key: `{ alias: { foo: ['f']} }`. */ alias?: { [key: string]: string | string[] }; /** * Indicate that keys should be parsed as an array: `{ array: ['foo', 'bar'] }`. * Indicate that keys should be parsed as an array and coerced to booleans / numbers: * { array: [ { key: 'foo', boolean: true }, {key: 'bar', number: true} ] }`. */ array?: ArrayOption | ArrayOption[]; /** Arguments should be parsed as booleans: `{ boolean: ['x', 'y'] }`. */ boolean?: string | string[]; /** Indicate a key that represents a path to a configuration file (this file will be loaded and parsed). */ config?: string | string[] | { [key: string]: boolean | ConfigCallback }; /** configuration objects to parse, their properties will be set as arguments */ configObjects?: Array<{ [key: string]: any }>; /** Provide configuration options to the yargs-parser. */ configuration?: Partial<Configuration>; /** * Provide a custom synchronous function that returns a coerced value from the argument provided (or throws an error), e.g. * `{ coerce: { foo: function (arg) { return modifiedArg } } }`. */ coerce?: { [key: string]: CoerceCallback }; /** Indicate a key that should be used as a counter, e.g., `-vvv = {v: 3}`. */ count?: string | string[]; /** Provide default values for keys: `{ default: { x: 33, y: 'hello world!' } }`. */ default?: { [key: string]: any }; /** Environment variables (`process.env`) with the prefix provided should be parsed. */ envPrefix?: string; /** Specify that a key requires n arguments: `{ narg: {x: 2} }`. */ narg?: { [key: string]: number }; /** `path.normalize()` will be applied to values set to this key. */ normalize?: string | string[]; /** Keys should be treated as strings (even if they resemble a number `-x 33`). */ string?: string | string[]; /** Keys should be treated as numbers. */ number?: string | string[]; /** i18n handler, defaults to util.format */ __?: (format: any, ...param: any[]) => string; /** alias lookup table defaults */ key?: { [key: string]: any }; } interface Parser { (args: string | any[], opts?: Options): Arguments; detailed(args: string | any[], opts?: Options): DetailedArguments; }

All of the options should be optional

You should state is by using opts?: Partial<Options>, not by making Options properties optional, as it makes it hard for me to reuse your types this way (I always have to get rid of undefined).

Other fixes integrated.

lib/yargs.ts

lib/common-types.ts

bcoe · 2020-06-10T00:54:01Z

Hey @QmarkC 👋 added you as a collaborator on yargs, to make flagging you for reviews like this easier.

Could I bother you to send an email to bencoe [at] google.com and I can onboard you to the project?

QmarkC · 2020-06-10T15:37:20Z

Hey @QmarkC 👋 added you as a collaborator on yargs, to make flagging you for reviews like this easier.

Could I bother you to send an email to bencoe [at] google.com and I can onboard you to the project?

Thank you @bcoe. I just sent you an email regarding on-boarding.

bcoe · 2020-06-12T18:01:07Z

@QmarkC thank you for the review 👌

@mleguen I'm happy to land this as soon as we address feedback 😄 awesome work!

mleguen · 2020-06-14T07:51:35Z

@QmarkC Thanks for such a detailed review, and more than happy to welcome you onboard!

Would you mind checking the still open discussions, to see if you agree with my replies?

mleguen added 4 commits June 3, 2020 09:41

refactor(yargs): move most of yargs to lib/yargs

b23407d

WIP: rename lib/yargs

3d77e28

WIP: get rid of temporary lib/yargs-types

8de352d

refactor(common-types): replace assertNotXXX by assertNotStrictEqual

06dbac5

mleguen mentioned this pull request Jun 3, 2020

On the road to typescript #1586

Closed

mleguen marked this pull request as draft June 3, 2020 08:29

mleguen force-pushed the 1586-ts-yargs branch 2 times, most recently from 2346985 to 3169f48 Compare June 3, 2020 10:11

mleguen added 8 commits June 3, 2020 19:00

chore(ts): tsify lib/yargs

9f43456

fix(completion): argv used by completion can be a promise

90c50bf

feat(usage): allow describe to be called with an array of keys

6e33bfa

To keep consistent with other parsing hint functions.

fix(yargs): add type assertions not covered by argsert

4e21c4f

A single argsert signature for multi-signature functions is not enough to cover all cases.

fix(yargs): options.configObjects is not a parsing hint array

c5dd6fb

fix(yargs): array of keys in config are handled as objects

085965d

fix(yargs): hide does not accept an object

fc00c71

fix(yargs): joined args are improperly handled for completion

67eab27

mleguen force-pushed the 1586-ts-yargs branch from 3169f48 to 67eab27 Compare June 3, 2020 17:08

mleguen marked this pull request as ready for review June 3, 2020 17:10

mleguen requested a review from bcoe June 3, 2020 17:10

bcoe approved these changes Jun 3, 2020

View reviewed changes

package.json Show resolved Hide resolved

lib/yargs.ts Show resolved Hide resolved

lib/yargs.ts Show resolved Hide resolved

lib/yargs.ts Show resolved Hide resolved

bcoe mentioned this pull request Jun 4, 2020

chore(ts): ship yargs.d.ts #1671

Merged

Merge branch 'master' into 1586-ts-yargs

e3bbc43

QmarkC reviewed Jun 10, 2020

View reviewed changes

mleguen added 2 commits June 14, 2020 08:25

refactor(apply-extends): cleaner signature

f568eac

refactor(common-types): simplify assertSingleKey for readability

d06980d

mleguen added 7 commits June 14, 2020 08:53

refactor(middleware): standardize acc in reduce

344d25b

chore(yargs-parser): fix TODO comment

4eb4f01

fix(ts): /bin/bash is yargs specific, not coming from yargs-parser

9eb5872

fix(ts): add missing config flags in yargs-parser typings

d55ae5a

fix(ts): update yargs-parser Options and args definition

4f5a696

refactor(ts): Dictionary was misspelled in some function names

9c4e8c5

doc(typo): fixed typo in comment

c72ad6f

mleguen and others added 2 commits June 14, 2020 10:02

fix(package): nothing more to publish from lib directory

eac2496

Merge branch 'master' into 1586-ts-yargs

6c37da8

bcoe merged commit e68334b into master Jun 17, 2020

bcoe deleted the 1586-ts-yargs branch June 17, 2020 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(ts): move and tsify most of root yargs.js to lib/yargs #1670

chore(ts): move and tsify most of root yargs.js to lib/yargs #1670

mleguen commented Jun 3, 2020

bcoe left a comment

bcoe commented Jun 3, 2020

bcoe commented Jun 10, 2020

QmarkC left a comment

QmarkC Jun 9, 2020

mleguen Jun 14, 2020

mleguen Jun 14, 2020

QmarkC Jun 9, 2020

mleguen Jun 14, 2020

QmarkC Jun 9, 2020 •

edited

mleguen Jun 14, 2020 •

edited

mleguen Jun 14, 2020

bcoe commented Jun 10, 2020

QmarkC commented Jun 10, 2020

bcoe commented Jun 12, 2020

mleguen commented Jun 14, 2020

		@@ -1,4 +1,4 @@
		import { Dictionary, assertNotUndefined, assertNotTrue } from './common-types'
		import { Dictionary, assertNotStrictEqual } from './common-types'

chore(ts): move and tsify most of root yargs.js to lib/yargs #1670

chore(ts): move and tsify most of root yargs.js to lib/yargs #1670

Conversation

mleguen commented Jun 3, 2020

bcoe left a comment

Choose a reason for hiding this comment

bcoe commented Jun 3, 2020

bcoe commented Jun 10, 2020

QmarkC left a comment

Choose a reason for hiding this comment

QmarkC Jun 9, 2020

Choose a reason for hiding this comment

mleguen Jun 14, 2020

Choose a reason for hiding this comment

mleguen Jun 14, 2020

Choose a reason for hiding this comment

QmarkC Jun 9, 2020

Choose a reason for hiding this comment

mleguen Jun 14, 2020

Choose a reason for hiding this comment

QmarkC Jun 9, 2020 • edited

Choose a reason for hiding this comment

mleguen Jun 14, 2020 • edited

Choose a reason for hiding this comment

mleguen Jun 14, 2020

Choose a reason for hiding this comment

bcoe commented Jun 10, 2020

QmarkC commented Jun 10, 2020

bcoe commented Jun 12, 2020

mleguen commented Jun 14, 2020

QmarkC Jun 9, 2020 •

edited

mleguen Jun 14, 2020 •

edited