Could we Include Return Types?

[octokit]

Could we Include Return Types?

[gr2m]

For responses, my current idea is to hook it up with https://github.com/octokit/fixtures so there is no risk the responses get out of date. This is not related to return types, just wanted to mention it to share my plans moving forward.

I don’t mind a central file where we map response properties to types, with the fixtures and routes specification being monitored with daily cron jobs I think we would be notified very quickly if the response type specification file would have a problem?

What might be helpful is GitHub’s GraphQL API. As far as I know, they plan to back the REST API with GraphQL in the longer term, and maybe there is a way to make all the GraphQL queries that handle the REST APIs public, because then we could generate the types pretty nicely, right?

If GitHub does won’t do that any time soon, we could also make it ourselves, it’s a finite number of routes that don’t change too often. The benefit would be that we would get the descriptions of the response properties, too, and they would be always up-to-date.

[philschatz]

Yeah, I think that using the fixtures to generate the TypeScript definitions is a great idea!

@JasonEtco in probot/probot#372 (comment) mentioned transform-www which uses json-schema-to-typescript .

I like the idea of creating a language-neutral JSON Schema and then using that to generate TypeScript definitions because other languages could benefit from the schema. Would that be useful for other octokit libraries?

Also, any thoughts on how to de-duplicate types? For example, a Gist contains a "user": ..., a Commit contains a "committer": ..., and a Repository contains an "owner": ... which all have the same structure (a UserWithoutFullName) but might be named differently by json-schema-to-typescript.

[gr2m]

Yes, you got it right. Lots has changed since Phil's last comment, but we do still derive response types from the response examples in the documentation, which are not always complete.

[gr2m]

Interesting edge case: https://developer.github.com/v3/git/refs/#get-a-reference – can return an object or an array based on :ref

I wonder if we should split that into two methods, one to get a single reference, one to get multiple, and then handle the differences in the method itself, so that it always returns the same type...

[gr2m]

I like the idea of creating a language-neutral JSON Schema and then using that to generate TypeScript definitions because other languages could benefit from the schema. Would that be useful for other octokit libraries?

Yes, I think that’s a good plan.

Also, any thoughts on how to de-duplicate types? For example, a Gist contains a "user": ..., a Commit contains a "committer": ..., and a Repository contains an "owner": ... which all have the same structure (a UserWithoutFullName) but might be named differently by json-schema-to-typescript.

Not yet :/ I guess we will end up having some configuration files for mapping

[philschatz]

@gr2m Regarding the edge-case, that is interesting.

What if @octokit/rest.js had 2 methods: one that always returned an Array of refs, and one that always returned a single Ref? (or threw an Error rather than unexpectedly returning an Array)?

From the docs, it seems like someone could create a branch named fix at one point in time and get back a single Ref, and then later created branches named fix-hyperdrive and fix-regressions, they would unexpectedly get back an Array.

[gr2m]

What if @octokit/rest.js had 2 methods: one that always returned an Array of refs, and one that always returned a single Ref? (or threw an Error rather than unexpectedly returning an Array)?

Yes that is my current plan. The implementation is still quite in flux, but for edge case like these I implemented overrides, see https://github.com/gr2m/octokit-rest-routes/tree/master/lib/endpoint/overrides/v3/git/refs. This will require custom implementation of these methods in the Octokit rest, they cannot be just auto-generated like in @octokit/rest right now, they will need to have an additional check to only return an object/array respectively

From the docs, it seems like someone could create a branch named fix at one point in time and get back a single Ref, and then later created branches named fix-hyperdrive and fix-regressions, they would unexpectedly get back an Array.

Yes exactly, it’s a pretty complicated edge case, hence the fallback to the overrides. I wish they made two different endpoints for these, but I guess it’s too late for that :D

[philschatz]

This will require custom implementation of these methods in the Octokit rest, they cannot be just auto-generated like in @octokit/rest right now, they will need to have an additional check to only return an object/array respectively

Ah right, that is annoying.

Not yet :/ I guess we will end up having some configuration files for mapping

Unless you want to, I can take a look at converting https://github.com/octokit/rest.js/pull/732/files#diff-bbbe78fcae258c894e23515f5c803147 into a JSON schema over the next few days; at least then it will be a format that people recognize and can use tools with, rather than an ad-hoc format.

Where do you think that JSON Schema file should live? I can start by updating https://github.com/octokit/rest.js/pull/732 along with the scripts that generate the TypeScript files, or wait. That might not be an ideal place, but it might help get probot/probot#372 unblocked sooner.

[maticzav]

@gr2m is this still relevant? It seems like the generated specification now includes response types (

routes/openapi/api.github.com/operations/git/get-ref.json

Line 44 in 185c907

"description": "response",

).

On the other hand, it seems like octokit/rest.js relies on derivation of types from examples. Did I get that right? Couldn't we use return types in the spec to directly generate TypeScript typings?

[maticzav]

@gr2m I could work on a PR over the weekend which would change the source of truth for type derivation to octokit/routes.

Would it be possible that you check it out afterwards? Do you have any pointers?

[gr2m]

I don't think we want to manually maintain response types in octokit/routes at this point. If the response in the documentation is incomplete, then the best course of action right now is to contact [email protected] and let them know about it. If the documentation is improved, everyone benefits

[maticzav]

@gr2m hmm. I don't know if I understand you correctly. My suggestion was to use octokit/routes as the source of octokit/rest.js type definitions. octokit/routes already includes all the return types, while there are some missing in octokit/rest.js. Is it possible that the generator script isn't working properly? GET /trees endpoint, for example, has examples, but its type is any.

Example

[gr2m]

Ahh I see, sure, pull requests welcome to improve the types generated in octokit/rest.js. The same fix would need to be applied to https://github.com/octokit/plugin-rest-endpoint-methods.js/, I plan to migrate octokit/rest.js to use the latter internally soon, as part of migrating it to TypeScript like the rest of the octokit/*.js projects

[gr2m]

The response types for the endpoint methods in @octokit/rest are now automatically generated based on octokit/routes