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

On the road to typescript #1586

Closed
mleguen opened this issue Mar 11, 2020 · 13 comments
Closed

On the road to typescript #1586

mleguen opened this issue Mar 11, 2020 · 13 comments

Comments

@mleguen
Copy link
Member

mleguen commented Mar 11, 2020

Here is a proposal about how to migrate progressively to typescript, to:

  • avoid big-bang merge requests, impossible to review and difficult to merge without freezing other developpments for a while
  • limit - and avoid if possible - impacts on projects depending on yargs

Proposed steps:

  1. (done: see chore(ts): init typescript conf + tsify is-promise #1587) a first merge request to:
    • convert to typescript a first module, not depending on any other yargs' module (is-promise could be a good choice), renaming it from lib/module.js to lib/module.ts (compiled version: build/lib/module.js and build/lib/module.d.ts for its types)
    • adapt other modules depending on this one to look for it in build
    • and initialize typescript's configuration, dependencies and packaging scripts for this module to have only its compiled version in the final npm package (transparent for users)
  2. (done: see chore(ts): tsify satellite modules #1596, chore(ts): tsify lib/completion #1601, chore(ts): tsify usage #1606, chore(ts): tsify lib/argsert #1610, chore(ts): tsify lib/validation #1614, refactor(ts): group type definitions and helpers #1632, chore(ts): tsify lib/middleware #1636 and chore(ts): tsify lib/command #1654 ) several other merge requests to convert to typescript all other modules in lib
  3. (done: see chore(ts): move and tsify most of root yargs.js to lib/yargs #1670) move most of yargs.js to lib/yargs.ts to keep it as a nearly empty shell (see below)
  4. (done: see chore(ts): move and tsify most of root yargs.js to lib/yargs #1670) keep index.js and yargs.js at the root of the project, mostly as empty shells importing typescript-built modules, to keep yargs require calls untouched in calling js code: const yargs = require('yargs') or const yargs = require('yargs/yargs') (changing to const { yargs } = require('yargs') would be necessary if we exposed directly typescript-built modules)
  5. (doing) add index.d.ts and yargs.d.ts at the root of the project, for calling ts code to be able to access our types (mostly empty shells too)
    • start with the type definitions in @types/yargs
    • replace them one by one by type definitions imported from ts files
    • when the new type names diverge from the ones in @types/yargs, keep old names as aliases to avoid a breaking change for ts users
  6. submit a PR to @types/yargs to replace it by a warning that it is no longer necessary to use it as yargs is now providing its own types
  7. update typescript examples
  8. convert to typescript other yargs modules we are depending on (set-blocking, y18n and yargs-parser), or merge them into yargs code (require-main-filename, see Switch to typescript require-main-filename#48)
  9. contribute to @types to add module definitions for other dependencies

EDITS: fixed typo, updated progress, review type naming strategy

@mleguen
Copy link
Member Author

mleguen commented Mar 11, 2020

I'm currently working on the 1st step

@bcoe
Copy link
Member

bcoe commented Apr 12, 2020

@mleguen I have a thought about the final conversion of yargs.js and index.js to TypeScript, could we use @types/yargs as the foundation:

  1. it's approach to reflection is sophisticated; it's clever about making the response object from the parse match the configuration options you've configured.
  2. it's downloaded 9,000,000 times a month... wow. I want to be careful that we don't break that community of 1000s of projects when we ourselves start to cut over.

@cameronhunter I saw that you've contributed to @types/yargs most recently, do you have any thoughts about how we can make moving yargs itself to TypeScript as smooth as possible?

@cameronhunter
Copy link
Contributor

Some thoughts:

As a first step you could pull the type definitions intoyargs and publish them yourself; that way they're under your control. Continue to publish JS, add a types field to package.json.

As long as you conform to the type interface, the migration of the implementation to TS should just be patch releases.

Eventually when your migration is complete you can publish types generated from your TS instead of maintaining the type interface manually.

@mleguen
Copy link
Member Author

mleguen commented Apr 30, 2020

@bcoe @cameronhunter I modified the proposed type naming strategy to integrate your suggestions. Would that be OK for you?

@bcoe
Copy link
Member

bcoe commented May 1, 2020

@mleguen "5." looks great.

@bcoe
Copy link
Member

bcoe commented May 15, 2020

@mleguen I've added additional functionality to our publication that allows us to publish candidate releases (just add the label publish:candidate to the release PR).

Doing this I released a candidate release of yargs with your TypeScript work ... It's pretty darn close to the same size as our non-typescript release at this point:

npm notice === Tarball Details === 
npm notice name:          yargs                                   
npm notice version:       15.4.0-beta.0                           
npm notice filename:      yargs-15.4.0-beta.0.tgz                 
npm notice package size:  52.5 kB                                 
npm notice unpacked size: 203.4 kB                                
npm notice shasum:        5a740a632c5038aaedc8dbdf30904dba705c0fd6
npm notice integrity:     sha512-7wb4FKgipUjvO[...]633RDERrlwXvg==
npm notice total files:   63     

vs.,

npm notice name:          yargs                                   
npm notice version:       15.3.1                                  
npm notice filename:      yargs-15.3.1.tgz                        
npm notice package size:  47.9 kB                                 
npm notice unpacked size: 182.2 kB                                
npm notice shasum:        9505b472763963e54afe60148ad27a330818e98b
npm notice integrity:     sha512-92O1HWEjw27sB[...]CLRtAkScbTBQA==
npm notice total files:   43     

This makes me happy and is really promising


If any of us interested in this work have the cycles, I suggest trying out yargs@15.4.0-beta.0 so we can make sure the conversion work is going smoothly.

@mleguen
Copy link
Member Author

mleguen commented May 20, 2020

I'm roughly halfway now of converting yargs.js to lib/yargs.ts.

I knew it would be the hardest part, but on the way I am learning much on yargs inner workings, and honing my type inference skills too, so I enjoy the ride! :-)

@bcoe
Copy link
Member

bcoe commented May 30, 2020

@mleguen anything I can do to pitch in on the conversion of yargs.js? Perhaps you can push a branch.

@mleguen
Copy link
Member Author

mleguen commented May 31, 2020

@bcoe Sure. I finished debugging on Friday, and should be able to push a PR after a final review on my side by Tuesday or Wednesday.

This PR will not yet include type inference on parse arguments, as @types/yargs does, as it would increase the review delay and complexity. This will have to be done later when working on d.ts files.

@bcoe
Copy link
Member

bcoe commented Jun 12, 2020

Feature Request:

My colleague brought up the fact that if you have validation and coercion right now, you don't get good typing information.

Basically, he would like to coerce an option, after applying validation. You could do something like this with middleware but you don't get good types out the other end.

@mleguen
Copy link
Member Author

mleguen commented Jun 14, 2020

My colleague brought up the fact that if you have validation and coercion right now, you don't get good typing information.

Mind a small example?

@bcoe
Copy link
Member

bcoe commented Jun 15, 2020

@mleguen here's where my colleague was attempting to use coerce and choices in conjunction:

https://github.com/grpc/grpc-node/pull/1474/files#diff-30899751cdd83c7ea9d8e583f66504c6R528

The problem is that coerce runs prior to choices. Independently of the TypeScript types, this seems like a bug.

@bcoe
Copy link
Member

bcoe commented Sep 10, 2020

We ultimately decided to continue pointing the community towards @types/yargs, here's a post on the topic.

It's a significant benefit to the project to be written in TypeScript, for maintainers, but @types/yargs simply deviates too far from the existing API surface to easily consolidate it with the project.

tdlr; we're put in the position of either:

  1. having yargs ship with community type definitions (_which is certainly convenient to the community), and breaking 1000s of users.
  2. or, not breaking 1000s of users, but requiring folks to still install @types/yargs.

I could imagine revisiting this decision in the future, or perhaps taking an alternate approach like shipping types separately.

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