Add Types to Responses

[philschatz]

This adds type information to responses (useful for probot/probot#372 and fixes #563). Currently they are of the form Github.AnyResponse.

This changes them to be of the form (depending on the method that is called):

• Github.AnyResponse<void> : a method that removes an object
• Github.AnyResponse<Github.Response.Gist> : a method that yields a single object
• Github.AnyResponse<Array<Github.Response.RepoEvent>>> : a method that yields multiple objects

It also creates a the response types like this:

declare namespace Github {
	...
    namespace Response {
        export type RepoEvent = {
            id: string;
            type: string;
            actor: EventActor;
            repo: RepoSlug;
            payload: RepoEventPayload;
            public: boolean;
            created_at: string;
        };
    }
}

This work was originally done in philschatz/octokat.js#176

Screenshots

Responses now have type information:

Includes the method documentation in the index.d.ts file so it shows up in TypeScript editors:

TODO

• Finish porting over the "yields" fields from philschatz/octokat#176 routes.json
• Verify the Callback does not yield an AnyResponse type
• Test that the type signatures are correct. This includes the Response.Boolean (which is an '' when true and an err when false), AnyResponse<Array<...>> ones

Possible TODO

• Infer the method yields an Array<...> when the JSON contains per_page as one of the parameters
• Infer all DELETE methods yield a void
• Simplify method signatures to be get: MethodWithParams<Github.PullRequestGetParams, Github.Response.RepoPullRequest> rather than get(params: Github.PullRequestsGetParams, callback?: Github.Callback<Github.Response.RepoPullRequest>): Promise<Github.AnyResponse<Github.Response.RepoPullRequest>>;

[gr2m]

Hey Phil, thanks for all the extensive pull request, love to see work being merged with octokat :) I'm heads down in finishing up other work right now but let me know if you have any blockers.

One question that I have is: how will we keep scripts/response-types.json updated? Can we automate it somehow? How do we make sure it remains in sync with the code?

[philschatz]

Thanks for responding! I originally generated scripts/response-types.json by autogenerating tests (using routes.json) and collecting the JSON (which I had cached in VCR cassettes/fixtures).

I think that something similar could be done to at least validate that the response-types.json file is still valid.

Possible Blocker: I pushed up an example that generates these tests but since octokit might need fixtures to run, I am not sure where those fixtures should be referenced (maybe in routes.json?)

If you run scripts/generate-response-types-tests.js it will generate test/autogenerated-response-types-test.js which contains the following:

// Frontmatter. See scripts/templates/generate-response-types-tests.tpl for the frontmatter

it('Responds with a "RepoEvent" when calling octokit.activity.getEvents(...)', async () => {
  const expectedEmpty = false
  const expectedArray = true
  const expectedType = "RepoEvent"
  const params = reifyParams({"page":{"type":"number","description":"Page number of the results to fetch."},"per_page":{"type":"number","default":"30","description":"A custom page size up to 100. Default is 30."}})
  const octokit = newGitHub()
  const {data} = await octokit.activity.getEvents(params)
  validateType(`(${expectedType})`, data, expectedType, expectedArray)
})

[gr2m]

sounds good! I'll be looking into auto generated & updating the routes.json file soon, I think that'll help

[tcbyrd]

@philschatz Heads up that I'm getting this now when I try to compile a typescript project using the newest version:

src/robot.ts(149,53): error TS2345: Argument of type '{ debug: boolean; host: string; pathPrefix: string; logger: LoggerWithTarget; }' is not assignable to parameter of type 'Options'.
  Property 'agent' is missing in type '{ debug: boolean; host: string; pathPrefix: string; logger: LoggerWithTarget; }'.

Writing the option as agent?: http.Agent in the type definitions takes this warning away. I wasn't sure what the appropriate method was to address this, but thought I'd report it here, since the generated definitions didn't make it optional. Looks like the agent option was added here: https://github.com/octokit/rest.js/pull/783

[gr2m]

Hey follow up on my last comment, the script to generate something like routes.json is now happening at octokit/routes#1. Hope that will be helpful to generate much more useful Typescript definitions, too.

[philschatz]

@tcbyrd Thank you for catching that! agent: http.Agent was introduced in https://github.com/octokit/rest.js/pull/741#issuecomment-366481703 but #803 Should fix that oversight 😊 .

[tcbyrd]

I think we need to change host to baseUrl in the types as well, right? https://github.com/octokit/rest.js/pull/807

When I use baseUrl, I get

Argument of type '{ debug: boolean; host: string | undefined; logger: LoggerWithTarget; }' is not assignable to parameter of type 'Options'.
  Object literal may only specify known properties, and 'host' does not exist in type 'Options'.

But if I use host, I get a deprecation warning:

console.warn node_modules/@octokit/rest/lib/parse-client-options.js:30
      DEPRECATED: host option is no longer supported

[philschatz]

This Pull Request now uses a JSON Schema to describe the Response Types, and @JasonEtco's suggestion of using json-schema-to-typescript to convert the schema into TypeScript definitions (per discussion in octokit/routes#20 ). You can paste the schema file into https://transform.now.sh/json-schema-to-ts/ to see the TypeScript.

Validating the schema might be automatically possible by using the existing tests and adding a octokit.hook.after(...) before the tests run. The only additional information that would be needed is to provide the octokit.hook.after callback which API call was made so that the expected response type can be looked up in routes.json.

It seems like octokit/routes might be a better place for the Schema to live (since it is language agnostic) but I included it here because:

• this repository does not use octokit/routes yet
• the routes.json contains an additional field, the responseType which is not automatically generated (see Could we Include Return Types? routes#20 (comment))

[gr2m]

I started a PR to use @ocotkit/routes at #822. The basic setup works, now there are lots and lots smaller todos to work off. I created some issues at https://github.com/octokit/routes/issues and will probably create a whole bunch more. @ocotkit/routes has a cron job and updates itself, so once we resolve the open issue this should be manageable to keep up-to-date moving forward. I have to focus on this right now to get it out of the way before I can help with the type definitions.

I’m not yet sure about how we achieve the same with response-type-defs.json? Do you know how to handle different responses based on different media type / preview headers?

[orta]

I’m not yet sure about how we achieve the same with response-type-defs.json? Do you know how to handle different responses based on different media type / preview headers?

1. Use differently named functions from octokit side (e.g. getRawContent, getDiffContent)
2. Use or types for the response e.g: string | number

[gr2m]

Use differently named functions from octokit side (e.g. getRawContent, getDiffContent)

I would prefer not to use different method names in order to stay as close to the REST API as possible.

Use or types for the response e.g: string | number

Sounds like a plan!

[gr2m]

#822 is merged, happy to move forward with this now.

#822 introduced aliasing of method names and parameters, does Typescript support that? I want to stay close to the API documentation so some parameters like :column_id has been renamed to :id and deprecated. Now id is required instead of column_id, so I think for people using typescript it would fall apart if they use the deprecated parameter in their code.

[gr2m]

oh also if you could help me double check the new version, especially if you use typescript, I'd appreciate it: #844

[laughedelic]

Would it make sense now to rename it to Response<T>? An alias AnyResponse = Response<any> could be kept too if needed for compatibility.

[julien-c]

You can also do Response<T = any>

[gr2m]

Hey friends, would you still like to finish this up? Anything I can help with?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[gr2m]

I’m sorry, I’m going to close this. I don’t want to add the response-type-defs.json file that would needed be maintained manually, for api.github.com and the GHE versions. We are discussing alternative ways to get schema information, I’ll keep you posted in #620