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): tsify satellite modules #1596

Merged
merged 15 commits into from
Mar 25, 2020
Merged

chore(ts): tsify satellite modules #1596

merged 15 commits into from
Mar 25, 2020

Conversation

mleguen
Copy link
Member

@mleguen mleguen commented Mar 20, 2020

Part of the 2nd step of #1586: this PR tsifies all "satellite" modules of yargs, leaving all "core" modules still in js (yet).

Although it touches 21 files:

  • its review should be pretty straightforward as the satellite modules are small, and not much changes in most files
  • modules are handled one by one, in different commits (with a 1st commit for renaming, and a 2nd one for tsifying)

Points of attention, however:

  • I had to switch from standard to standardx for linting, as there are some known false positives with standard and typescript (see https://standardjs.com/#typescript)
  • 1st benefits of switching to typescript: it detected a not-handled null in apply-extends. I added an explicit exception to cover this case (though it was already implicitely handled, as it made fs throw a few lines below)

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.

again, once we're a ways along with the conversion, I would be excited to see us drop eslint for gts, but this is looking awesome (and I appreciate the incremental approach).

Even though none of these changes are breaking, I would like to make TypeScript part of the next major of yargs (this will be a big announcement).

@mleguen
Copy link
Member Author

mleguen commented Mar 20, 2020

I would be excited to see us drop eslint for gts

Do we really have to wait for yargs not to support node 8 anymore for this?

Linting tools are only dev dependencies, so couldn't we just remove linting checks from node 8 tests? And move to gts 2.x for other node versions?

@bcoe
Copy link
Member

bcoe commented Mar 24, 2020

@mleguen I would like to wait because many (probably thousands) of people using yargs are relying on @types/yargs, once we start moving towards publishing our own types, I believe it's an improvement, but it will be a breaking API change for all of our TypeScript users (I'm of the opinion that we should treat breaking TypeScript changes as breaking changes, it's miserable when upstream TypeScript dependencies break their interface).

@mleguen
Copy link
Member Author

mleguen commented Mar 24, 2020

@bcoe I agree with you on the fact that publishing our own types should be considered as a breaking change (this would impact me as a yargs user), but as long as we do not tsify index.js or yargs.js at the project root yet, there should be no impact on ts users.

So IMHO, this current PR should not be considered as a breaking change, nor switching to gts 2.x and disabling linting in node 8 test (which was what I was suggesting in my previous message).

Would this be OK for you?

@bcoe
Copy link
Member

bcoe commented Mar 24, 2020

@mleguen this makes sense to me, as long as we keep the Node 8 tests for the time being for the suite, and make it a breaking change once yargs and index are exported.

@mleguen mleguen merged commit 16ee006 into master Mar 25, 2020
@mleguen mleguen deleted the 1586-ts-step-2 branch March 25, 2020 09:46
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

2 participants