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

Switch to typescript #265

Closed
mleguen opened this issue Apr 2, 2020 · 13 comments
Closed

Switch to typescript #265

mleguen opened this issue Apr 2, 2020 · 13 comments

Comments

@mleguen
Copy link
Member

mleguen commented Apr 2, 2020

Needed for yargs/yargs#1586

@QmarkC
Copy link
Contributor

QmarkC commented May 21, 2020

I would like to help with this. I reviewed yargs/yargs#1586 and the related pull requests. Based on that, I propose the first step be the initial TypeScript configuration and converting lib/tokenize-arg-string.js to TypeScript. This is similar to the first step PR for yargs/yargs#1586.

I created a draft PR (#272) with this proposed first step to make feedback/discussion a little easier for any of those changes.

Beyond the first step, is there any guidance on how to proceed?
To avoid big-bang merge requests I imagine you would want to convert index.js in parts to lib/*.ts files.
I created the following list with all the functions in index.js and took a first pass at grouping them together by topic/concern. Each group could be their own lib module or some you might prefer to be their own module (e.g. is-number.ts vs a utils.ts that exports isNumber) since yargs/yargs has a is-promise.ts module.

  • Parser
    • Parser
    • Parser.detailed
  • configuration
    • defaultConfiguration
    • checkConfiguration
    • setConfig
    • setConfigObject
    • setConfigObjects
  • parse
    • parse
    • applyDefaultsAndAliases
  • parse-argv
    • argv variable assignment in parse
    • isUnknownOptionAsArg
    • isUnknownOption
    • setArg
    • setPlaceholderKeys
    • applyCoercions
    • applyEnvVars
    • eatArray
    • eatNargs
    • processValue
    • maybeCoerceNumber
    • defaultValue
    • guessType
    • defaultForType
  • parse-aliases
    • addNewAlias
    • combineAliases
    • checkAllAliases
    • extendAliases
  • parse-flags
    • hasAnyFlag
    • hasFlagsMatching
    • hasAllShortFlags
  • utils
    • isNumber
    • isUndefined
    • sanitizeKey
    • hasKey
    • setKey
    • increment

@bcoe
Copy link
Member

bcoe commented May 22, 2020

If @mleguen is open to the help with the conversion, to me it seems like it would be great to have another person pitching in.

@mleguen
Copy link
Member Author

mleguen commented May 25, 2020

Thanks @QmarkC for the help!

The approach I took (and try to hold) for yargs, is to focus only on switching to TS, and not to refactor at the same time, as it is already difficult enough to review as is.

So I would recommend keeping at the moment a single lib/index.ts module, and refactor it later into several modules in separate issues/PR.

@QmarkC
Copy link
Contributor

QmarkC commented May 27, 2020

Since @types/camelcase and @types/decamelize packages are no longer available, do you want to upgrade those dependencies as part of this issue?

decamelize from ^1.2.0 to ^3.2.0
camelcase from ^5.0.0 to ^5.3.1

@bcoe
Copy link
Member

bcoe commented May 27, 2020

@QmarkC this would unfortunately have to b e a breaking change I think, because the latest versions of those libraries drop support for Node 6.

Until we take a breaking change, perhaps we can use require to pull those in without types? unless @mleguen has a better approach he's used.

@QmarkC
Copy link
Contributor

QmarkC commented May 28, 2020

@bcoe The versions listed are the last before they dropped support for Node 6. The upgrades are not needed after all. The camelcase package has a declaration file and I can add one for decamelize. I'll follow the same pattern in yargs to add it at lib/typings/decamelize.d.ts.

We can import them like so: import camelCase = require('camelcase') but I'll have to move them below the other import statements. If you would prefer import camelCase from 'camelcase then esModuleInterop and allowSyntheticDefaultImports need to be enabled in the tsconfig compilerOptions.

@mleguen
Copy link
Member Author

mleguen commented May 29, 2020

I'd rather we stick to import camelCase = require('camelcase'), personally.

@bcoe
Copy link
Member

bcoe commented Jun 2, 2020

@QmarkC as long as those versions have both TypeScript and the definitions upgrading sounds great 👍

edit: one other thing to check, I think that there was a time when camelcase or decamelize shipped with the full unicode library, which increased the package size by a huge amount, try running npm pack before and after the upgrade, see if I'm imagining that reason for not upgrading.

@QmarkC
Copy link
Contributor

QmarkC commented Jun 3, 2020

@bcoe The current semver for camelcase, ^5.0.0, already matches 5.3.1 so no change there. The package size was unchanged (19.3 kB, unpacked: 77.6 kB) after upgrading decamelize to ^3.2.0.

@QmarkC
Copy link
Contributor

QmarkC commented Jun 4, 2020

I've been working on switching the rest of yargs-parser to TypeScript. It's pretty much done, mainly just waiting on the couple related open PRs. I did want to get some feedback on a few items while waiting on that.

Current Arguments interface:

interface Arguments {
  /** Non-option arguments */
  _: string[];
  /** The script name or node command */
  $0: string;
  /** All remaining options */
  [argName: string]: any;
}
  • Arguments['_'] seems like it should be (string | number)[] at least when configuration['unknown-options-as-args'] === true, see test at L3113 and L3139.
  • Arguments['$0'] - the script name doesn't seem to be required as yargs-parser never sets this. This is set by yargs after calling the parser, see L1111. Leaving it as is doesn't seem to hurt anything but I thought I would point this out to see if it should be removed or marked as optional.

There are a few options not covered by the existing types that I wanted to confirm types for:

  • configObjects: Array<{ [key: string]: any }>. Example in README: {configObjects: [{'x': 5, 'y': 33}, {'z': 44}]}.
  • __: i18n handler, defaults to util.format. Use same as util.format? (format: any, ...param: any[]) => string. yargs passes y18n.__ which is (str: string, arg1?: string, arg2?: string, arg3?: string): string so the utils.format interface works for that too.
  • key: I didn't see any examples of the key option being used in the tests. Seems to be used as defaults for aliases lookup table. I can tell it's a string indexed object and opts.default is also passed to extendAliases so use same as default option { [key: string]: any }?

@mleguen
Copy link
Member Author

mleguen commented Jun 6, 2020

I meet the same kind of problems with @types/yargs-parser in yargs/yargs#1586: it looks like they were not written to match yargs-parser js code (or even use it from typescript), but rather as a submodule of @types/yargs.

So I would suggest not to rely to much on the existing @types/yargs.

@mleguen
Copy link
Member Author

mleguen commented Jun 6, 2020

Here are the yargs-parser typings I am using at the moment in yargs, but they still may include incorrect or incomplete types. The "source of truth" must be yargs-parser js code.

@bcoe
Copy link
Member

bcoe commented Sep 20, 2020

this work is done \o/

@bcoe bcoe closed this as completed Sep 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants