[WIP] 🌟 New: Adds TypeScript definition generation

[aendra-rininsland]

trafficstars

As per https://github.com/michael/github/issues/241#issuecomment-250444749 I've been working on a TypeScript definition.

The d.ts included in this PR is 95% automated, 5% by hand. tsd-jsdoc currently has a bug that moves types into class definitions, where they fail. They need to be in a namespace as per the file included herein.

I'm leaving this as WIP for the moment so people can troubleshoot the issue with tsd-jsdoc; the goal is to have the d.ts file generated on every PR so it remains constant as the API all changes.

[pascalwhoop]

I like the PR but don't quiet understand why all the JSDoc needs to be touched to add typing support. Also when I copy the files into my node_modules folder (manually since its not yet in the repo), the axios. namespace gives me errors all over the place. How did it look like for you @aendrew ?

[mohsen1]

I looked at this but it's a lot more work that I expected. I resolved the merge conflict anyways.

@aendrew please merge my PR to your fork so if someone wants to pick it up they can: https://github.com/aendrew/github/pull/1

[aendra-rininsland]

Sorry for the really slow response to this issue!

@pascalwhoop I'm using JSDoc to generate the TypeScript definitions, and Axios returns a different Promise interface from the standard NodeJS promise. There are a few others that have been changed to allow tsd-jsdoc to properly generate interfaces.

@mohsen1 Cheers, thanks for doing that! I'll try to review and merge that sometime soon (ideally tomorrow but have been rather busy as of late).

tsd-jsdoc has released a new version and I plan to pick this up again shortly. Hopefully the new version will resolve some of the issues with interface generation that resulted in me having to tweak the JSDoc more than I'd have liked.

[akfish]

I was trying to use the generated .d.ts file directly in my project. It seems that all references to axios.Promise that are marked as 'the Promise for the http request' should be axios.AxiosPromise, according to axios/index.d.ts.

Those axios related typings are broken since [email protected], which is the version I got when I installed [email protected] with yarn.

[akfish]

Here is my standalone .d.ts file that works with webpack/ts-loader toolchain: https://gist.github.com/akfish/b9a42945f1e681ed173b0b06365303a7

[aendra-rininsland]

Given the codebase has moved along quite a lot and there's a new version of tsd-jsdoc, I've redone the TypeScript generation and made it so that no code changes are actually necessary. It's not quite automated yet, but the definition in the above commit should work a lot better. See above commit, or the definition here.

The steps needed to generate a d.ts file upon release are:

1. Generate d.ts via npm run make-ts-def
2. Add import {AxiosPromise} from 'axios' at top of newly-created github-api.d.ts
3. Replace all instances of Promise with AxiosPromise
4. Move the Requestable class into the Requestable module; export default it.
5. Replace all instances of callback with Requestable.callback (except in Requestable module itself)
6. Replace all instances of auth with Requestable.auth (except in Requestable module itself)
7. Fix any line that includes nested options values, i.e.: options.author?: any, options.commiter?: any, options.encode?: boolean
8. Change any Params type to any
9. Fix errors re: optional arguments (add ?)
10. Change Requestable.auth to two overloaded interfaces, ensuring both Requestable.auth and Requestable.callback are exported.

This admittedly isn't the best TypeScript def; it has far too many any types in it. But it's definitely a start, and so far this is the only GitHub API lib around that has anything resembling a TypeScript def.

To improve this in the future:

• Replace Promise with AxiosPromise in JSDoc. This will help a lot as it will get rid of a lot of the any types.
• Monitor this bug, which will fix the issue with parsing options interfaces
• Probably worth somehow creating generic interfaces for options and params — having these documented as part of the definition greatly increases its usability.
• Figure out why Requestable isn't exported as part of Requestable module

The remaining steps can probably be accomplished using codemods or somesuch.

I don't know why tests are failing; am doubting it has anything to do with me given I don't touch the actual codebase in this PR.

@akfish Would you mind taking a look at this again? I'd normally just add the excellent def you wrote to the repo and close this PR, but I feel some sort of automatic generation is necessary to keep everything up-to-date.

[akfish]

Actually my typing was based on the automatic generation. So I can't take credit for that. :)

I took a look at the typing. It would be easier if some automatic tests are included. Here's how I set it up based on DefinitelyTyped's method:

I changed the tsconfig.json to:

{
    "compilerOptions": {
        "module": "commonjs",
        "lib": [
            "es6"
        ],
        "noImplicitAny": true,
        "noImplicitThis": true,
        "strictNullChecks": false,
        "baseUrl": "./",
        "typeRoots": [
            "./"
        ],
        "types": [],
        "noEmit": true,
        "forceConsistentCasingInFileNames": true
    },
    "files": [
        "github-api.d.ts",
        "tests.ts"
    ]
}

And added this line to package.json:

{
  // other lines
  "types": "github-api.d.ts",
  // other lines
}

And added tests.ts:

// Write tests here
// This file will be compiled by tsc to check typings
// It will not be executed
import GitHub = require('github-api')

let gh = new GitHub()

Then you just run tsc as the test.

Now here're some issues I spotted:

• I believe you will need this line at the beginning of the generated d.ts file:

  /// <reference types="axios" />
  

• Compiler error:

  tests.ts(3,10): error TS2351: Cannot use 'new' with an expression whose type lacks a call or construct signature.

  This suggests the exported signatures are not correct. Adding:

  exports = GitHub
  

  seems to fix this issue. But I'm not familiar with the code base so I'm not sure if this is a correct fix.

[akfish]

See https://github.com/akfish/github/tree/d.ts

[aendra-rininsland]

@akfish It might be the tsconfig.json that's causing the issue, IIRC setting types and/or typeRoots will prevent node_modules/ and thus the Axios defs won't be picked up (from what I understand, using triple-slash directives in defs is considered a bad practice).

Will take a closer look sometime tomorrow! Thanks for picking this up and adding some tests!

[akfish]

No problem.

/// <reference types="..." /> is used for declaring type dependencies. It's widely used in @types packages. Though a second look at the document seems to suggest that it's only for @types/ packages. I'll verify this later.

[akfish]

I just verified that the /// <reference types="..." /> is not necessary for npm package typings. There's no need to change tsconfig.json too. It must be a problem with my global TypeScript version yesterday.

[dvargas92495]

What's the status here?

I'm interested in this enhancement. Could I take over if no one's working on it?

[aendra-rininsland]

@dvargas92495 Feel free! I don't expect I'll ever finish it tbth.