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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Jul 23, 2017
Merged

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

merged 5 commits into from
Jul 23, 2017

Conversation

alan-agius4
Copy link
Contributor

Add typings to support TypeScript

@alan-agius4 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 alan-agius4 mentioned this pull request Jun 24, 2017
Closed
@alan-agius4 alan-agius4 changed the title feat(typings): add typings to support for TypeScript feat(typings): add typings to support TypeScript Jun 24, 2017
@abetomo abetomo requested a review from roman-vanesyan June 24, 2017 13:54
michael-yx-wu
michael-yx-wu reviewed Jul 5, 2017
View reviewed changes
package.json Outdated
@@ -16,10 +16,12 @@
},
"devDependencies": {
"should": ">= 0.0.1 <9.0.0",
"sinon": ">=1.17.1"
"sinon": ">=1.17.1",
"typescript": "^2.3.4"
Copy link

Choose a reason for hiding this comment

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

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

@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Jul 5, 2017 via email

@roman-vanesyan
Copy link
Collaborator

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

@alan-agius4
Copy link
Contributor Author

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

@roman-vanesyan
Copy link
Collaborator

roman-vanesyan commented Jul 18, 2017 •
edited
Loading

Sorry, mean rebase, mistyped :)

@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Jul 18, 2017 via email

@alan-agius4
Copy link
Contributor Author

@vanesyan rebase done :)

roman-vanesyan
roman-vanesyan reviewed Jul 22, 2017
View reviewed changes
package.json Outdated
@@ -28,8 +23,12 @@
"index.js"
],
"dependencies": {
"@types/node": "^8.0.2",
"graceful-readlink": ">= 1.0.0"
"@types/node": "^8.0.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this go in peerDependencies?

@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Jul 22, 2017 via email

@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Jul 22, 2017 via email

@roman-vanesyan
Copy link
Collaborator

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

@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Jul 22, 2017 via email

@roman-vanesyan
Copy link
Collaborator

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

@alan-agius4
Copy link
Contributor Author

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

@roman-vanesyan
Copy link
Collaborator

roman-vanesyan commented Jul 22, 2017 •
edited
Loading

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
Copy link
Contributor Author

alan-agius4 commented Jul 22, 2017 via email

@roman-vanesyan
Copy link
Collaborator

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
Copy link
Contributor Author

alan-agius4 commented Jul 22, 2017 via email

@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Jul 22, 2017 via email

@roman-vanesyan
Copy link
Collaborator

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
Copy link

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:

  1. projects not using typescript (npm / yarn will warn that the peer dependency is not met and will be annoying)
  2. 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
Copy link
Contributor Author

So, what do you guys want me to do?

@michael-yx-wu
Copy link

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

@roman-vanesyan roman-vanesyan merged commit 66dfb8c into tj:master Jul 23, 2017
@roman-vanesyan
Copy link
Collaborator

Thanks 👍

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

Marak commented Sep 13, 2017

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.

@abetomo abetomo mentioned this pull request Sep 20, 2017
Closed
@timocov timocov mentioned this pull request Nov 23, 2017
Closed
vchimev pushed a commit to NativeScript/nativescript-marketplace-demo that referenced this pull request Nov 23, 2017
fix(ts): cast setTimeout() to avoid TS errors
9936924 
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.
@vchimev vchimev mentioned this pull request Nov 23, 2017
Merged
vchimev pushed a commit to NativeScript/nativescript-marketplace-demo that referenced this pull request Nov 23, 2017
fix(ts): cast setTimeout() to avoid TS errors (#282)
f3f0f35 
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
Copy link

xavdid commented Nov 28, 2017

There's a couple of gotchas happening here:

  1. 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.
  2. 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.
  3. 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
Copy link
Contributor Author

alan-agius4 commented Nov 28, 2017 •
edited
Loading

@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
Copy link

xavdid commented Nov 28, 2017

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
Copy link
Contributor Author

alan-agius4 commented Nov 28, 2017 via email

@shadowspawn shadowspawn mentioned this pull request Mar 5, 2019
Closed
@shadowspawn shadowspawn mentioned this pull request Jun 12, 2019
Closed
@shadowspawn shadowspawn mentioned this pull request Oct 28, 2019
Merged
@shadowspawn shadowspawn mentioned this pull request Jan 10, 2020
Merged
@shadowspawn shadowspawn mentioned this pull request Aug 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-vanesyan roman-vanesyan roman-vanesyan left review comments

@michael-yx-wu michael-yx-wu michael-yx-wu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alan-agius4 @roman-vanesyan @michael-yx-wu @Marak @xavdid