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

Add a TypeScript definition #241

Open
aendra-rininsland opened this issue Oct 21, 2015 · 8 comments
Open

Add a TypeScript definition #241

aendra-rininsland opened this issue Oct 21, 2015 · 8 comments
Assignees

Comments

@aendra-rininsland
Copy link
Member

This will help adoption by people who use TypeScript and doesn't impact the library any other way.

I'll do this because I want it and have experience with TS.

@aendra-rininsland aendra-rininsland self-assigned this Oct 21, 2015
@aendra-rininsland aendra-rininsland added this to the v0.10.8 milestone Oct 21, 2015
@aendra-rininsland
Copy link
Member Author

Anyone interested, I've started work at aendrew/github#typescript_def. It's completely unfinished and I'm struggling with the syntax, so any help is quite welcome.

@aendra-rininsland
Copy link
Member Author

Apparently you can generate these via jsDoc. I'd really prefer to write these all in one place and then have tooling (Possibly this) generate any copies, so will add these to the definition for the moment and then migrate that documentation into the actual code itself sometime around 0.11.0 (I'd do so now but it'd change the shape of the code enough that it'd likely cause a large number of existent PRs to conflict).

@AurelioDeRosa AurelioDeRosa modified the milestones: 0.12.0, v0.10.8 Jan 24, 2016
@AurelioDeRosa AurelioDeRosa removed this from the 0.12.0 milestone Feb 21, 2016
@AurelioDeRosa
Copy link
Member

@Aendrew Are you still interested in this?

@aendra-rininsland
Copy link
Member Author

@AurelioDeRosa Yep, absolutely, just a bit delayed. Probably a good thing though; ambient definitions have changed a bit since I posted this. Currently researching the best way to add a def to a lib.

@clayreimann
Copy link
Member

@Aendrew We're all jsdoc'd up so integrating this shouldn't be too hard if you want to.

@smakinson
Copy link

@Aendrew Have you made any progress on your research?

@aendra-rininsland
Copy link
Member Author

aendra-rininsland commented Sep 29, 2016

Here's a first pass, I've merely generated the d.ts from jsDoc and haven't had time to play with the definition beyond that: https://github.com/aendrew/github/tree/d.ts

The declaration itself is at:

https://github.com/aendrew/github/blob/d.ts/github-api.d.ts

There are a bunch of errors being thrown, given that tsd-jsdoc doesn't validate jsDoc comments.

jsDoc comments that will need to be fixed:

  • ln. 1078, "Requestable.callback"
    @param {(Object|true)} needs to be something like @param {GitHubAPIResponse|boolean} or any
    @param {Object} needs to be something like a GitHubAPIRequest interface or any
    This is probably one of the more difficult ones to resolve correctly. It's possible that we could piggyback on Axios' TypeScript defs for this; instead of Object, we could use Axios.AxiosResponse. The biggest problem with this is it's generated within the class definition, which is a known bug; see Issues in generated files englercj/tsd-jsdoc#6.
  • ln. 1135, "Search.Params"
    Object is not a valid TypeScript type. Needs an interface.
  • lns. 544, 553, 809, 819, 1015, "variable-name" conflict
    Argument should be renamed to something not reflecting a primitive.
  • ln. 789 , "Repository.createBranch"
    For some reason it's inferring this as an optional param, which can't go in front of required parameters. I think removing [oldBranch=master] from the doccomment will fix this.
  • ln. 935, "Repository.writeFile"
    cb should be in square brackets if it's optional.
  • ln. 1109, "Requestable._requestAllPages"
    results should either be optional, or private methods shouldn't have a declaration generated.
  • ln. 1404, "interface Iauth"
    token is not a valid type.
    Just change the type to string.
  • Throughout "Generic type Promise requires 1 type argument(s)"
    Instead of just returning a promise, what the promise resolves must also be included.
    Replacing Promise with axios.Promise solves this.

On a whole, though, y'all done a crazy-good job of documenting everything, am semi-surprised everything worked this well!

I'll probably write a mass PR in the next few days that resolves all of the above and lets tsd-jsdoc generate the TS declaration cleanly, followed by one that resolves this issue by adding the build setup I use in that branch.

@aendra-rininsland
Copy link
Member Author

See above PR if you want to test the definition I've been working on. Automatic generation is about 95% of the way there; once tsd-jsdoc is fixed upstream it'll be worth adding d.ts generation as a build step.

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

4 participants