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

chore(ts): init typescript conf + tsify tokenize-arg-string #272

Merged
merged 7 commits into from Jun 11, 2020

Conversation

QmarkC
Copy link
Contributor

@QmarkC QmarkC commented May 21, 2020

First step of #265, "Switch to typescript".

  • Add initial TypeScript configuration
    Modeled after TypeScript configuration used in yargs/yargs.

    Matched semantic versions used in yargs package.json for added devDependencies.

    • Add/Update configs related to build output folder.
    • Move coverage thresholds from npm script to .nycrc config
  • Add lint configuration for .ts files.
    Modeled after yargs setup.

    • Switch from standard to standardx
    • Add .editorconfig
    • Add ESLint config
  • Convert lib/tokenize-arg-string.js and test/tokenize-arg-string.js to TypeScript

  • Update index.js to use tokenizeArgString from build

@QmarkC QmarkC mentioned this pull request May 21, 2020
Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments, but this is looking solid.

.eslintrc Show resolved Hide resolved
lib/tokenize-arg-string.ts Outdated Show resolved Hide resolved
lib/tokenize-arg-string.ts Show resolved Hide resolved
package.json Show resolved Hide resolved
test/tokenize-arg-string.ts Show resolved Hide resolved
.versionrc Outdated Show resolved Hide resolved
@bcoe
Copy link
Member

bcoe commented May 30, 2020

@QmarkC this is looking pretty solid to me, happy to take the PR whenever you're ready.

@QmarkC
Copy link
Contributor Author

QmarkC commented Jun 1, 2020

@QmarkC this is looking pretty solid to me, happy to take the PR whenever you're ready.

I've updated the PR based on the current feedback.
There is still an open question regarding the type to use the argString argument. I would like to get confirmation on using string | any[] or if this should be more strict such as string | (string | number)[]. The same type will need to be used for Parser, Parser.detailed, and parse.

Also should I rebase to merge all the commits for cleaner history? That's what I typically do after getting feedback on a PR.

@QmarkC QmarkC force-pushed the 265-tsify-tokenize-arg-string branch from 09062a3 to 7c24670 Compare June 3, 2020 00:56
@QmarkC QmarkC marked this pull request as ready for review June 3, 2020 00:59
@bcoe
Copy link
Member

bcoe commented Jun 3, 2020

@QmarkC I would like to stick with the string | any[], as other changes would be breaking (the "" + arg is coercing any types into a string any ways). The current approach looks good to me.

Looks like your rebased already, in general this is optional, as I squash and merge when I land 👍 I sometimes do so any ways on a large feature.

@bcoe
Copy link
Member

bcoe commented Jun 3, 2020

we can land this as soon as @mleguen approves. Thanks for getting this work started @QmarkC.

@bcoe
Copy link
Member

bcoe commented Jun 3, 2020

@QmarkC I've been talking with @mleguen, I think we'll make the final TypeScript release of yargs a major release any ways... so, let's plan that this work on yargs-parser will be a major, at which point let's make a PR to drop Node 8, move to Node 10, and move to the latest version of decamelize.

The problem with decamelize@3.x is the xregexp dep, which is 700kb.

Copy link
Member

@mleguen mleguen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! Ready to merge once engines requirements in package.json is in line with tsc output.

package.json Outdated
"index.js"
"index.js",
"build",
"lib/**/*.js"
],
"engines": {
"node": ">=6"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do want to continue supporting node 6 in yargs-parser, then the target needs to be changed in tsconfig.json (and in .eslintrc too, I think), as node 6 supports only 17% of es2017 (see https://node.green/#ES2017).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But as I see we no longer do tests on node 6, I suggest upgrading the engines requirements to "node": ">=8" here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mleguen @QmarkC since we're going to need to take on a breaking change any ways, let's just make it >=10, but I'll do this in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but opening #278 does not change the issue here: tsconfig.json and .eslintc are still incoherent with package.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mleguen engines.node in package.json has been updated to >=10, ESLint parserOptions.ecmaVersion updated to 2018, and updated tsconfig.json to remove es2017 so it will use es2018 from gts/tsconfig-google.json.

@bcoe
Copy link
Member

bcoe commented Jun 6, 2020

@QmarkC master has the latest decamelize and camelcase now 👍

Resolve conflicts, make sure you've addressed @mleguen's feedback, and we'll merge this?

@QmarkC
Copy link
Contributor Author

QmarkC commented Jun 11, 2020

Thanks a lot! Ready to merge once engines requirements in package.json is in line with tsc output.

@bcoe @mleguen this should be resolved now with the update to node >=10, and es2018 for ESLint and tsconfig.json.

I also created another branch, 265-tsify-yargs-parser, that has this first step, second step to tsify yargs-parser, and cleans up the commits. Let me know if you want to close this PR in favor of that branch.

@bcoe bcoe merged commit 5c17398 into yargs:master Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants