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

fix: Add node types to typings #974

Closed
wants to merge 1 commit into from
Closed

fix: Add node types to typings #974

wants to merge 1 commit into from

Conversation

ffflorian
Copy link
Contributor

No description provided.

@shadowspawn
Copy link
Collaborator

@ffflorian What problem is this Pull Request solving?

@ffflorian
Copy link
Contributor Author

@shadowspan
commander's typings use interfaces from Node so the reference should be included. When I wanted to create typings for commander-remaining-args (see DefinitelyTyped/DefinitelyTyped#36148) I needed to reference Node's typings explicitly. This could've been avoided by just referencing them here.

@shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn self-requested a review June 12, 2019 22:39
@ffflorian
Copy link
Contributor Author

@shadowspawn

From the comment:

You can include the /// <reference types="node"> flag to access EventEmitter and anything else needed

Does that mean you approve?

@shadowspawn
Copy link
Collaborator

No it does not mean I approve, it means I have started looking into it. If you have good links for the details of what reference types does, I'll be reading up on that too. (I review Pull Requests fairly carefully and add more comments than you might expect.)

@ffflorian
Copy link
Contributor Author

I review Pull Requests fairly carefully and add more comments than you might expect.

You don't know what I expect 🙂 I was just wondering what you meant by your comment, sorry. Take your time!

Here are the docs for reference types: https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html

In short:

Triple-slash references instruct the compiler to include additional files in the compilation process.

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

Happy with the triple slash, thanks. I think it is the Right Thing to do. As a small bonus it changes the tsc error messages when node types are not installed, and that may help people recognise the problem quicker.

In part because we are shipping definitions ourselves now, I do not wish to list new minor contributors in the "Definitions by" please. On DefinitelyTyped that had the benefit of getting email notifications about related changes but that is no longer the case. (We have git history and GitHub contributors for author credit.)

@ffflorian
Copy link
Contributor Author

@shadowspawn I removed the changes to the contributors line.

@shadowspawn
Copy link
Collaborator

Thanks @ffflorian

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

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

Thank you!

@shadowspawn shadowspawn added this to the v3.0.0 milestone Jun 22, 2019
@shadowspawn
Copy link
Collaborator

Merging into release/3.0.0 branch

shadowspawn added a commit that referenced this pull request Jun 23, 2019
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Jun 23, 2019
@shadowspawn
Copy link
Collaborator

Already had two approvals. Merged into v3 and will be included in that release. Closing to make it clear that should not be merged into master.

Thank you for your contributions.

@ffflorian
Copy link
Contributor Author

Thanks @shadowspawn @abetomo, looking forward to v3!

@ffflorian ffflorian deleted the fix/node-types branch June 24, 2019 08:01
@shadowspawn
Copy link
Collaborator

Available now as a prerelease. See #1001

@shadowspawn
Copy link
Collaborator

Shipped in v3: https://github.com/tj/commander.js/releases/tag/v3.0.0

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.

None yet

3 participants