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

Remaining work for yargs v17 release #1899

Closed
7 tasks done
bcoe opened this issue Apr 4, 2021 · 12 comments
Closed
7 tasks done

Remaining work for yargs v17 release #1899

bcoe opened this issue Apr 4, 2021 · 12 comments

Comments

@bcoe
Copy link
Member

bcoe commented Apr 4, 2021

v17 is a large release, in which I've tried to address many long outstanding oddities related to middleware, async handlers, check functions, coerce functions, help output from commands.

There are still a few more tasks I would like to complete before we push it out the door:


If you would like to help QA, npm i yargs@next.

@seivan
Copy link
Contributor

seivan commented Apr 7, 2021

@bcoe I can help with update TypeScript docs. Or at the very least make some examples, unless you meant like generated jsdoc?

@bcoe
Copy link
Member Author

bcoe commented Apr 7, 2021

@seivan I would appreciate help with the TypeScript documentation, my intention has been to dig deep into the tests here:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/yargs/yargs-tests.ts

And try to make sure we reflect as much as possible in the docs.

I would also like to start contributing upstream to @types/yargs, for shortcomings.

@seivan
Copy link
Contributor

seivan commented Apr 7, 2021

@bcoe That sounds like a great idea, though I'd argue in order to keep things succinct, maybe omit certain approaches that aren't taking advantage of the type system .

I did some tests of my own and cross-posting here:

export function createAdd(arg: Yargs.Argv): Yargs.Argv {
  return arg.command("add <name> [url]", "sample stuff", (addYarg) => {
    return addYarg
      .positional("name", { type: "string" }, demandOption: true)
      .positional("url", { type: "string" })
  }, (args) => {
     //args.name & args.url will be typed ✅
  })
}

While it does not work (out of the box) with CommandModule

const sourceAddCommand: Yargs.CommandModule = {
  command: ["add <name> [url]"],
  builder: (args) => {
    return args
      .positional("name", { type: "string", demandOption: true })
      .positional("url", { type: "string" })
  },

  handler: (args) => {
  //args & args.url ARE NOT typed ❌
  },
}

However there is partial support for just options and not positional.
But you can't have a builder and you lose descriptions, it literally skips over them when printing usage.

const sourceListCommand: Yargs.CommandModule<{}, {
  "available": {
    boolean: true,
    conflicts: "outdated",
   description: "Not visible ❌" 
   desc: "Not visible ❌",
  }, "outdated": {
    boolean: true,
    conflicts: "available",
   description: "Not visible ❌" 
   desc: "Not visible ❌" 
  }  
}> = {
  describe: "List Command",

  command: "list",
  handler: (args) => {
   // args.available & args.outdated are typed ✅
  },
}

To get typed arguments and avoid polluting the argument interface from other sub-commands in the same parent you need to wrap the entire sub-command in a function.
To rephrase: sub-command siblings will get access to each other arguments and positional if they are not wrapped in a function.

   Yargs
    .default(YargsHelper.hideBin(process.argv))
    .scriptName(ProjectPackage.name)
    .command("mother <command>", "One of the main commands", (yargs) => {

      return [yargs]
        .map(makeFirstSubCommand)
        .map(createAdd)
      [0]
})
   . command("father <command>", "Other main command with its own subs", (yargs) => {

      return [yargs]
        .map(createAnotherSubCommandUnderFather)
        .map(someMoreUnderFather)
      [0]
})
.argv

@bcoe
Copy link
Member Author

bcoe commented Apr 8, 2021

@seivan if perhaps make a candidate PR with the behavior you're seeing added to the examples? I'd also like to add a section on how to init a TypeScript project to use with yargs.

@seivan
Copy link
Contributor

seivan commented Apr 8, 2021

@bcoe I am just fixing things as I go along
DefinitelyTyped/DefinitelyTyped#52265

Honestly the types should be in here and generated by in the repo since this is a Typescript project after all, but more importantly should be true to the source code more than anything.
Maintaining types externally seems like an accident waiting to happen.

More so, it's painful to see so much potential for better types and API.

For instance a command should't take more than a single middleware instead of an array and its return type would be combined with the types defined in builder before being passed to a handler
That way a builder defines input's that are expected and a middleware can then process those before moving to a handler

An example would be that a command that expects --path: string to some file and a middleware would use that to read the content of said file and return the updated arg.

The handler would then get {path: string, fileContent: string | undefined}.

@bcoe
Copy link
Member Author

bcoe commented Apr 11, 2021

Honestly the types should be in here and generated by in the repo since this is a Typescript project after all, but more importantly should be true to the source code more than anything.

The major issue is that the types in this codebase aren't as feature rich as the well supported types in DefinitelyTyped. We haven't made much use of generics, to ensure that the types defined when configuring yargs carry over to the parsed Arguments object. Whereas, @types/yargs has put thought into this.

I think shipping the types we have in yargs would be a step backwards for usability, and would also be a breaking change for the community.

I'm also not convinced that we want to put the work in to add complex generics to yargs itself, I'm concerned this could significantly increase the complexity of the codebase, and make it more difficult to contribute to (especially for people who aren't TypeScript experts).


Regardless of whether we consider shipping types from yargs itself in the future, I wouldn't like to take on this change in the v17 release -- perhaps it's something we consider in v18.

@seivan
Copy link
Contributor

seivan commented Apr 11, 2021

The major issue is that the types in this codebase aren't as feature rich as the well supported types in DefinitelyTyped. We haven't made much use of generics, to ensure that the types defined when configuring yargs carry over to the parsed Arguments object. Whereas, @types/yargs has put thought into this.
I think shipping the types we have in yargs would be a step backwards for usability, and would also be a breaking change for the community.

Yeah I don't mean to it to be a regression, if that is the case, I understand why you haven't.

I'm also not convinced that we want to put the work in to add complex generics to yargs itself, I'm concerned this could significantly increase the complexity of the codebase, and make it more difficult to contribute to (especially for people who aren't TypeScript experts).

Hmm, I somewhat disagree on this, or technically it's two points:

First off is the complexity of types themselves, I genuinely think it would be better if they were part of the code base as it would tied to the code and be less broken, I do find runtime errors and issues that the type checker would have found.

You saw the ticket with terminalWidth() which the compiler autocompleted but fails at runtime.

There are other issues, where certain methods not available or should not be called on top level are available and doesn't throw a compiler error when they should.

and make it more difficult to contribute to (especially for people who aren't TypeScript experts).

I disagree on the premise that the compiler would guide contributors to what should or should not be possible, and to honest conditional types and string literal types are still new to me, but I find myself using them more often because of the possibility they open up.

Being an expert isn't necessary since the compiler would (try) guide you.

Regardless of whether we consider shipping types from yargs itself in the future, I wouldn't like to take on this change in the v17 release -- perhaps it's something we consider in v18.

I get that, I and I agree, but it's something I would like to check at at some point.
Inherently I would love an API that would use associated types to generate options and positionals.

@seivan
Copy link
Contributor

seivan commented Apr 11, 2021

Also I got a few questions that would help me try to make an alternative approach if you would be open to it, but I need to know the priorities here.
One of the issues is the concept of the function based builder.

The reason that's an issue is that it allows for defining arguments and positional at runtime, if yarg could abandon that notion that those would be typed or even better just get rid of the idea completely it would be a lot easier to design said API. That being said, if it would be possible to do that without too much trouble I'd try it.
This combined with the handler getting a combination of types from returned values of MiddlewareFunction and builder for
So a middleware can modify content of args before being passed to handler, and said handler would get everything typed.

So for the sake brevity, going forward explaining what I had in mind assume that anything returned from a builder () => () is untyped. In fact I'd just omit that entirely.

This API that didn't work with everything would be the nicest approach.

const sourceListCommand: Yargs.CommandModule<{
 options: {
  "available": {
    boolean: true,
    conflicts: "outdated",
   description: "Not visible ❌" 
   desc: "Not visible ❌",
  }, "outdated": {
    boolean: true,
    conflicts: "available",
   description: "Not visible ❌" 
   desc: "Not visible ❌" 
  }  
}
}> = {
  describe: "List Command",

  command: "list",
  handler: (args) => {
   // args.available & args.outdated are typed ✅
  },
}

The problem here is that the types aren't available at runtime, so things like description wouldn't be visible as it's part of the type. But I know there is a way o make it work, I just haven't figured out how at this moment.

@bcoe
Copy link
Member Author

bcoe commented Apr 12, 2021

@seivan I'm close to calling it a night and watching some 📺, just wanted to leave a note that I'll respond to your last messages in detail early this week.

@bcoe
Copy link
Member Author

bcoe commented May 2, 2021

@seivan I felt I'd checked enough boxes, with regards to goals for a v17 release, that i would publish v17.

TypeScript improvements can be an ongoing project.

@bcoe
Copy link
Member Author

bcoe commented May 2, 2021

released 🎉

@bcoe bcoe closed this as completed May 2, 2021
@bcoe bcoe unpinned this issue May 2, 2021
@seivan
Copy link
Contributor

seivan commented May 31, 2021

Sorry for the delayed response: Congratulations!
@bcoe Could you take a look at DefinitelyTyped/DefinitelyTyped#52265

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

2 participants