feat(typings): add typings to support TypeScript #646

alan-agius4 · 2017-06-24T08:43:08Z

Add typings to support TypeScript

…pes/commander`

michael-yx-wu · 2017-07-05T21:43:52Z

package.json

@@ -16,10 +16,12 @@
  },
  "devDependencies": {
    "should": ">= 0.0.1 <9.0.0",
-    "sinon": ">=1.17.1"
+    "sinon": ">=1.17.1",
+    "typescript": "^2.3.4"


unlikely to make a different to this PR, but 2.4.1 is available now 😄

alan-agius4 · 2017-07-05T21:55:11Z

With the carot it will take 2.4.1 anyways

…

On Wed, 05 Jul 2017 at 23:43, Michael Wu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In package.json <#646 (comment)>: > @@ -16,10 +16,12 @@ }, "devDependencies": { "should": ">= 0.0.1 <9.0.0", - "sinon": ">=1.17.1" + "sinon": ">=1.17.1", + "typescript": "^2.3.4" unlikely to make a different to this PR, but 2.4.1 is available now 😄 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#646 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQv-WnZfMuSZN3IHDeKaE0F-HlKyRjBTks5sLAObgaJpZM4OERJx> .

roman-vanesyan · 2017-07-18T18:53:54Z

Can you resolve the pr on top of master, please :)

alan-agius4 · 2017-07-18T18:56:03Z

What do you mean by 'resolve' please? I am current away and ill be able to do it on Friday

roman-vanesyan · 2017-07-18T18:59:10Z

Sorry, mean rebase, mistyped :)

alan-agius4 · 2017-07-18T19:06:39Z

Ah okay, will do that on friday :)

…

On Tue, 18 Jul 2017 at 20:59, Roman Vanesyan ***@***.***> wrote: Sorry, mean rebase, mistyped :) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#646 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQv-WmvuFXHUmRMr5LP2bCJOphDJs6YUks5sPQCDgaJpZM4OERJx> .

# Conflicts: # package.json

alan-agius4 · 2017-07-21T10:35:08Z

@vanesyan rebase done :)

roman-vanesyan · 2017-07-22T19:08:25Z

package.json

@@ -28,8 +23,12 @@
    "index.js"
  ],
  "dependencies": {
-    "@types/node": "^8.0.2",
-    "graceful-readlink": ">= 1.0.0"
+    "@types/node": "^8.0.2"


Shouldn't this go in peerDependencies?

alan-agius4 · 2017-07-22T19:13:39Z

Can go, if you prefer it that way. Though its not neccessary. Not a bog fan of peers unless it's neccessery. I'll do it as you prefer though. Just let me know :)

…

On Sat, 22 Jul 2017 at 21:08, Roman Vanesyan ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In package.json <#646 (comment)>: > @@ -28,8 +23,12 @@ "index.js" ], "dependencies": { - ***@***.***/node": "^8.0.2", - "graceful-readlink": ">= 1.0.0" + ***@***.***/node": "^8.0.2" Shouldn't this go in peerDependencies? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#646 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQv-WnxTg9i3QV20u965QaikE-i_2HA5ks5sQkirgaJpZM4OERJx> .

alan-agius4 · 2017-07-22T19:17:19Z

The simple reason why its dependency is that it that node is requeried by commander.js because of the 'eventEmitter'

…

On Sat, 22 Jul 2017 at 21:13, Alan Agius ***@***.***> wrote: Can go, if you prefer it that way. Though its not neccessary. Not a bog fan of peers unless it's neccessery. I'll do it as you prefer though. Just let me know :) On Sat, 22 Jul 2017 at 21:08, Roman Vanesyan ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In package.json > <#646 (comment)>: > > > @@ -28,8 +23,12 @@ > "index.js" > ], > "dependencies": { > - ***@***.***/node": "^8.0.2", > - "graceful-readlink": ">= 1.0.0" > + ***@***.***/node": "^8.0.2" > > Shouldn't this go in peerDependencies? > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#646 (review)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AQv-WnxTg9i3QV20u965QaikE-i_2HA5ks5sQkirgaJpZM4OERJx> > . >

roman-vanesyan · 2017-07-22T19:26:57Z

Just worry about people that don't need typing for commander, they'll get extra deps.

alan-agius4 · 2017-07-22T19:55:21Z

Thats trie, but its a very small deps. Though i can change it if you wish

…

On Sat, 22 Jul 2017 at 21:27, Roman Vanesyan ***@***.***> wrote: Just worry about people that don't need typing for commander, they'll get extra deps. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#646 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQv-Wun8EGhzM0Y7nMpMUEANQNgmqxaHks5sQk0DgaJpZM4OERJx> .

roman-vanesyan · 2017-07-22T20:24:09Z

I'm not too familiar with TypeScript, but what do other publishers do?

alan-agius4 · 2017-07-22T20:35:33Z

Usually they are set as dependencies even with the typyings packages themselves. https://www.npmjs.com/package/@types/commander

…

On Sat, 22 Jul 2017 at 22:24, Roman Vanesyan ***@***.***> wrote: I'm not too familiar with TypeScript, but what do other publishers do? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#646 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQv-WiYns2lIPM4Hi6HfxyzK8KsOsePwks5sQlptgaJpZM4OERJx> .

roman-vanesyan · 2017-07-22T20:44:50Z

Sorry me, if it's a bit stupid question, but what do we want to include typings within package, when typings are already released in DefinitelyTyped for? :)

alan-agius4 · 2017-07-22T20:47:41Z

Having typyings in DefinitlyTyped is a 'last' resort for the community to have typings of a library that the owner doesn't want to have within his library. TypeScript recommand having them within the library and not within DefinitlyTyped.

…

On Sat, 22 Jul 2017 at 22:44, Roman Vanesyan ***@***.***> wrote: Sorry me, if it's a bit stupid question, but what do we want to include typings within package, when typings are already released in DefinitelyTyped? :) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#646 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQv-WqOsw2suOSOeDKwS4u2f94lfaW2kks5sQl9FgaJpZM4OERJx> .

roman-vanesyan · 2017-07-22T20:56:12Z

Sorry me, but I haven't found such suggestion by TypeScript. Furthermore as I understand they heavily rely on @types npm namespace, which from my understanding is referring to DefinitelyTyped.

alan-agius4 · 2017-07-22T20:59:55Z

https://github.com/Microsoft/TypeScript-Handbook/blob/master/pages/declaration%20files/Publishing.md

…

On Sat, 22 Jul 2017 at 22:56, Roman Vanesyan ***@***.***> wrote: Sorry me, but I haven't found such suggestion by TypeScript. Furthermore as I understand they heavily rely on @types npm namespace, which from my understanding is referring to DefinitelyTyped. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#646 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQv-Wg6_TD8R6x7na6aoZ7Y6r2EIImRCks5sQmHvgaJpZM4OERJx> .

alan-agius4 · 2017-07-22T21:07:05Z

@types is indeed for DefinitlyTyped which is used only for non typescript libaries, as typically typescript libraries always have typing inside the library itself and now andays even js libraries have typings in them such as 'moment' Reason being is for maintability and no need for addition packages. TypeScript doesn't realy on DefinitlyTyped at all, its the consumer that realies on them if he needs non TypeScript ready libraries.

…

On Sat, 22 Jul 2017 at 22:59, Alan Agius ***@***.***> wrote: https://github.com/Microsoft/TypeScript-Handbook/blob/master/pages/declaration%20files/Publishing.md On Sat, 22 Jul 2017 at 22:56, Roman Vanesyan ***@***.***> wrote: > Sorry me, but I haven't found such suggestion by TypeScript. Furthermore > as I understand they heavily rely on @types npm namespace, which from my > understanding is referring to DefinitelyTyped. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#646 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AQv-Wg6_TD8R6x7na6aoZ7Y6r2EIImRCks5sQmHvgaJpZM4OERJx> > . >

roman-vanesyan · 2017-07-22T21:17:06Z

Ok, sorry me again, but previously I was able to use Node.js api out of the box when used typescript last time in the past. But sometimes not so far, I tried it again and now only way to use node.js api was to install 'em from DefinitelyTypes.

michael-yx-wu · 2017-07-23T00:23:24Z

Including node typings as a peer dependency is fine since it's a declaration that commander works with a minimum version of node, but you'll run into problems with:

projects not using typescript (npm / yarn will warn that the peer dependency is not met and will be annoying)
typescript projects relying on an older version of node: I believe commander works with at least node 7+ and possibly earlier versions?

I think we should keep it as an actual dependency to avoid 1.. We should also use the actual minimum version of node typings that commander is compatible with although 2. is not an immediate concern unless we change the typings dependency to a peer dependency.

alan-agius4 · 2017-07-23T09:37:20Z

So, what do you guys want me to do?

michael-yx-wu · 2017-07-23T13:10:31Z

My preference is to keep it as a dependency but use the version that commander is minimally compatible with.

roman-vanesyan · 2017-07-23T19:23:19Z

Thanks 👍

Marak · 2017-09-13T21:51:08Z

Hrmm.

By adding @types/node dependency, commander project is no longer zero-dependencies.

The total size of the installed code-base seems to be increased by over 10x.

Project was previously 57kb now is 503kb. That seems wrong to increase package size by 10x in order to support one specific framework ( TypeScript ).

Not sure what the project maintainers are thinking here. Please review this again @michael-yx-wu.

ERROR in [at-loader] ./app/examples/conference-agenda/conference-agenda-example.ts:63:5 TS2322: Type 'Timer' is not assignable to type 'number'. ERROR in [at-loader] ./app/examples/conference-agenda/conference-agenda-example.ts:71:9 TS2322: Type 'Timer' is not assignable to type 'number'. ERROR in [at-loader] ./app/examples/user-profile/user-profile-example.ts:89:5 TS2322: Type 'Timer' is not assignable to type 'number'. ERROR in [at-loader] ./app/examples/user-profile/user-profile-example.ts:97:9 TS2322: Type 'Timer' is not assignable to type 'number'. Caused by tj/commander.js#646. Related to tj/commander.js#719.

xavdid · 2017-11-28T00:29:54Z

There's a couple of gotchas happening here:

the package.json files array doesn't include the index.d.ts, so the typings aren't shipped to npm. TS complains when you try to import commander, since the types aren't installed. That's an easy fix.
The more involved piece is the dependencies. The bundle size increase is probably mostly due to the Typescript devDep, which commander doesn't really depend on. The more harmonious way to do this is to include the index.d.ts file, but not introduce any dependencies for it. You can include the /// <reference types="node"> flag to access EventEmitter and anything else needed.
Semi-related, after cloning, I couldn't get the type tests to fail. I changed a .option line to .asdf and tests still passed fine. That could super be a configuration issue on my end, but if they're not actually catching anything, they're adding complexity. It's easy enough to have maintainers edit the index.d.ts file whenever there are API changes (usually semver major & minor).

So! That said, if I were to make a PR now, I'd remove deps, add a reference to the index, and edit files. That should get the bundle back down, because shipped files would go up by exactly 1 and we'd be back to the deps we had before.

How does that sound?

alan-agius4 · 2017-11-28T06:03:38Z

@xavdid the bundle size is still the same considering the fact that TypeScript is only for devDeps, to be honest though, finding solutions to keep the devDeps down by removing TS, for me it's not a good idea as like this you will remove the tests will keeps the sanity of the definition file.

Why is it such a problem to have an extra devDeps which is actual used? Just for an example would you consider removing sinon?

Re the test point. however, does the Types Definitions, know that they should write tests on them, as that's the standard.

The test didn't fail, because commander argument can be accessed such as;

program.cheese()

Thus, when you changed .option to .asdf it assumed that .asdf is an argument

xavdid · 2017-11-28T06:46:47Z

I don't care about the bundle size, but other commenters seem to take issue with it. Since the API for a big project like this changes so infrequently, it's pretty easy to keep the typings up-to-date. I don't see a ton of value in the tests, but it's not a big deal.

That's a good point about the tests, I'm still pretty new with commander.

Nevertheless, the files issue remains.

alan-agius4 · 2017-11-28T06:48:53Z

The files issue is fixed #725 (review)

…

On Tue, 28 Nov 2017 at 07:46, David Brownman ***@***.***> wrote: I don't care about the bundle size, but other commenters seem to take issue with it. Since the API for a big project like this changes so infrequently, it's pretty easy to keep the typings up-to-date. I don't see a ton of value in the tests, but it's not a big deal. That's a good point about the tests, I'm still pretty new with commander. Nevertheless, the files issue remains. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#646 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQv-WoK2mbzhIJCSc1s3nP5kkl6G20nHks5s66xbgaJpZM4OERJx> .

feat(typings): add typings to support for TypeScript in favor of `@ty…

4b75c37

…pes/commander`

alan-agius4 changed the title ~~feat(typings): add typings to support for TypeScript in favor of `@ty…~~ feat(typings): add typings to support for TypeScript Jun 24, 2017

alan-agius4 mentioned this pull request Jun 24, 2017

Support for TypeScript #616

Closed

alan-agius4 changed the title ~~feat(typings): add typings to support for TypeScript~~ feat(typings): add typings to support TypeScript Jun 24, 2017

alan-agius4 added 2 commits June 24, 2017 10:48

packages: add @types/node

0822eb9

typings: remove definitions statement

43362c9

abetomo requested a review from roman-vanesyan June 24, 2017 13:54

michael-yx-wu reviewed Jul 5, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feature/ts-typings

b40ab0d

# Conflicts: # package.json

roman-vanesyan reviewed Jul 22, 2017

View reviewed changes

packages: use @types/node 7.0.39

57e043e

michael-yx-wu approved these changes Jul 23, 2017

View reviewed changes

roman-vanesyan merged commit 66dfb8c into tj:master Jul 23, 2017

alan-agius4 deleted the feature/ts-typings branch July 23, 2017 20:13

abetomo mentioned this pull request Sep 20, 2017

No longer zero-dependencies. In addition the package size has become 10x. #699

Closed

timocov mentioned this pull request Nov 23, 2017

Move @types/node to devDependencies #719

Closed

vchimev mentioned this pull request Nov 23, 2017

fix(ts): cast setTimeout() to avoid TS errors NativeScript/nativescript-marketplace-demo#282

Merged

shadowspawn mentioned this pull request Mar 5, 2019

Add autocomplete based on omelette library #907

Closed

shadowspawn mentioned this pull request Jun 12, 2019

fix: Add node types to typings #974

Closed

shadowspawn mentioned this pull request Oct 28, 2019

Switch from class to class-like to include all types in commander namespace #1081

Merged

shadowspawn mentioned this pull request Jan 10, 2020

Remove EventEmitter from TypeScript definition file #1146

Merged

shadowspawn mentioned this pull request Aug 16, 2023

Refactor type-checking setup #1969

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(typings): add typings to support TypeScript #646

feat(typings): add typings to support TypeScript #646

alan-agius4 commented Jun 24, 2017

michael-yx-wu Jul 5, 2017

alan-agius4 commented Jul 5, 2017 via email

roman-vanesyan commented Jul 18, 2017

alan-agius4 commented Jul 18, 2017

roman-vanesyan commented Jul 18, 2017 •

edited

Loading

alan-agius4 commented Jul 18, 2017 via email

alan-agius4 commented Jul 21, 2017

roman-vanesyan Jul 22, 2017

alan-agius4 commented Jul 22, 2017 via email

alan-agius4 commented Jul 22, 2017 via email

roman-vanesyan commented Jul 22, 2017

alan-agius4 commented Jul 22, 2017 via email

roman-vanesyan commented Jul 22, 2017

alan-agius4 commented Jul 22, 2017 via email •

edited

Loading

roman-vanesyan commented Jul 22, 2017 •

edited

Loading

alan-agius4 commented Jul 22, 2017 via email

roman-vanesyan commented Jul 22, 2017

alan-agius4 commented Jul 22, 2017 via email

alan-agius4 commented Jul 22, 2017 via email

roman-vanesyan commented Jul 22, 2017

michael-yx-wu commented Jul 23, 2017

alan-agius4 commented Jul 23, 2017

michael-yx-wu commented Jul 23, 2017

roman-vanesyan commented Jul 23, 2017

Marak commented Sep 13, 2017

xavdid commented Nov 28, 2017

alan-agius4 commented Nov 28, 2017 •

edited

Loading

xavdid commented Nov 28, 2017

alan-agius4 commented Nov 28, 2017 via email

feat(typings): add typings to support TypeScript #646

feat(typings): add typings to support TypeScript #646

Conversation

alan-agius4 commented Jun 24, 2017

michael-yx-wu Jul 5, 2017

Choose a reason for hiding this comment

alan-agius4 commented Jul 5, 2017 via email

roman-vanesyan commented Jul 18, 2017

alan-agius4 commented Jul 18, 2017

roman-vanesyan commented Jul 18, 2017 • edited Loading

alan-agius4 commented Jul 18, 2017 via email

alan-agius4 commented Jul 21, 2017

roman-vanesyan Jul 22, 2017

Choose a reason for hiding this comment

alan-agius4 commented Jul 22, 2017 via email

alan-agius4 commented Jul 22, 2017 via email

roman-vanesyan commented Jul 22, 2017

alan-agius4 commented Jul 22, 2017 via email

roman-vanesyan commented Jul 22, 2017

alan-agius4 commented Jul 22, 2017 via email • edited Loading

roman-vanesyan commented Jul 22, 2017 • edited Loading

alan-agius4 commented Jul 22, 2017 via email

roman-vanesyan commented Jul 22, 2017

alan-agius4 commented Jul 22, 2017 via email

alan-agius4 commented Jul 22, 2017 via email

roman-vanesyan commented Jul 22, 2017

michael-yx-wu commented Jul 23, 2017

alan-agius4 commented Jul 23, 2017

michael-yx-wu commented Jul 23, 2017

roman-vanesyan commented Jul 23, 2017

Marak commented Sep 13, 2017

xavdid commented Nov 28, 2017

alan-agius4 commented Nov 28, 2017 • edited Loading

xavdid commented Nov 28, 2017

alan-agius4 commented Nov 28, 2017 via email

roman-vanesyan commented Jul 18, 2017 •

edited

Loading

alan-agius4 commented Jul 22, 2017 via email •

edited

Loading

roman-vanesyan commented Jul 22, 2017 •

edited

Loading

alan-agius4 commented Nov 28, 2017 •

edited

Loading