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 from class to class-like to include all types in commander namespace #1081

Merged
merged 1 commit into from
Oct 30, 2019

Conversation

shadowspawn
Copy link
Collaborator

Pull Request

Problem

TypeScript errors in some client use cases because not all types are exported. See #1037

Solution

Rework to use class-like interfaces and constructors rather than explicitly using class so all the types can be included in the commander namespace. (This is made hard by the existing default export of the global Command object so can not use multiple exports or export explicit classes.)

This is beyond my TypeScript comfort zone! We might need a TypeScript expert to confirm this is a reasonable approach.

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Oct 28, 2019

@alan-agius4 (#646)
@jsamr (#713)
Thanks for your past contributions to the Commander TypeScript declaration file.

In case you are interested, I have a PR in flight and would welcome any feedback from TypeScript contributors.

I have done a refactor to get the Command and Option types fully into the commander namespace to fix a reported bug. I did a lot of reading and trial and error, but I seem to have it working nicely now with a manual class-like declaration (interface + constructor).

All my attempts to use a true class failed and my understanding is declaring a class adds a value (the constructor) to the otherwise type-only namespace, which then conflicts with the export of the global command object. I expect this same problem is why Command and Option were originally outside the namespace, then moved into a separate namespace to allow aliasing the types.

Copy link
Contributor

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

this LGTM

@shadowspawn
Copy link
Collaborator Author

Thanks @alan-agius4 !

@jsamr
Copy link
Contributor

jsamr commented Oct 28, 2019

@shadowspawn I think grouping all the declarations in the same namespace is a good idea, and I was a bit skeptical with the former local namespace, and I would guess it would fix #1037 . I just wonder however why you had to get rid of class declarations in favor of instance interface + constructor interface + constant declaration? It's OK, but a bit over-engineered.

@shadowspawn
Copy link
Collaborator Author

With the single export =, adding a class to the namespace causes an error. Stripped down example:

declare namespace commander {
  class Foo {
  }
}
declare const commander: object
export = commander;

error TS2300: Duplicate identifier 'commander'.

@jsamr
Copy link
Contributor

jsamr commented Oct 28, 2019

@shadowspawn This is expected :-) The namespace is a value! A POJO which fields are the names of declared values.

So you should get rid of declare const commander: object and you'll be all good.

EDIT You can read that nice page to get what I mean: Definition File Theory: A Deep Dive

Also, I can submit a PR to your fork if you don't grasp it, but I would recommend trying as it is a great exercise to grow your typing muscles 💪 .

@jsamr
Copy link
Contributor

jsamr commented Oct 28, 2019

@shadowspawn see my edited answer above

@shadowspawn
Copy link
Collaborator Author

Thanks @jsamr. I did consult the TypeScript documentation, especially the Declaration Files pages, and the Deep Dive was indeed one of the most relevant pages.

My understanding is the previous and PR solution allow a single export to include both meanings of commander, the "value" which is the global program object, and the (non-value) "namespace" which includes the supporting "types". The shape of the global program object is the Command class/interface with a few additions.

I am not sure how to declare the global/default program object without exporting a "value" for it. Did you have a pattern in mind?

For example, client code could look like:

import * as commander from 'commander';
commander
   .name('global-program');
const myProgram = new commander.Command();
myProgram
   .name('local-program');

@jsamr
Copy link
Contributor

jsamr commented Oct 29, 2019

@shadowspawn Now I get it. Typescript cannot merge a constant value with a namespace which has values within, such as classes. So your trick is perfectly legitimate 🙂

LGTM! 👍

@shadowspawn shadowspawn self-assigned this Oct 29, 2019
@shadowspawn shadowspawn added this to the v4.0.0 milestone Oct 29, 2019
@shadowspawn
Copy link
Collaborator Author

Thanks @alan-agius4 and @jsamr

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.

3 participants