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

Make the Octokit constructor derive types for octokit.request based on the version option passed to the constructor #3

Closed
7 tasks done
Tracked by #2127
gr2m opened this issue Jul 19, 2021 · 0 comments
Assignees
Labels
Type: Feature New feature or request typescript
Projects

Comments

@gr2m
Copy link
Contributor

gr2m commented Jul 19, 2021

part of octokit/octokit.js#2127

Todos

Differences between platforms

There are several differences between the REST API at https://api.github.com/ ("dotcom", GItHub Enterprise Server ("GHES") instances and GitHub AE ("GHAE").

  • Some endpoints will always only exist on one platform but not the other
  • New endpoints are always released on dotcom first, for example GET /app/hook/deliveries, which is not present in GHES 3.1 or older versions
  • Sometimes new parameters, response keys, or response headers are added to dotcom which do not propagate immediately to the other platforms
  • GHES includes a X-GitHub-Enterprise-Version response header

However there are no cases when an existing REST API endpoint behaves differently in a breaking way, the changes are only additive, e.g. when dotcom graduated a preview header which was not yet graduated on a GHES version.

Also if a script needs to be compatible with multiple GHES versions, it is enough to be compatible with the lowest version. What works in GHES 3.0 will also always work in GHES 3.1, 3.2, etc. At least within the range of officially supported GHES versions.

What we have today

Today we have types for all the 600+ REST API endpoints in dotcom. We do track the OpenAPI specifications for the other platforms in https://github.com/octokit/openapi and https://github.com/octokit/openapi-types.ts, but we don't use the other types in any @octokit modules yet.

We do have a plugin for GHES versions: https://github.com/octokit/plugin-enterprise-server.js. When loading the plugins, only the REST API methods are loaded that exist in the respective GHES version, but the types are incorrect. For example developers get TypeScript IntelliSense for non-existing parameters and response keys, while e.g. the GHES-only response headers are missing. And there are no types at all for the .admin.* endpoint methods, which makes the package unusable with TypeScript, as they build is failing. The TypeScript source code relies on REST API routes that do not exist in @octokit/types, so TypeScript will complain when attempting to build it. We have a long-staning open issue for this problem: octokit/plugin-enterprise-server.js#84

What we want

By default, The Ocotkit SDK should behave just as it does today:

const octokit = new Octokit({
  auth: env.GITHUB_TOKEN
})

const response = octokit.request("GET /repos/{owner}/{repo}/hooks/{hook_id}/deliveries", {
  owner,
  repo,
  hook_id
})

Now in order to write script targeting a minimal GHES version, an optional version parameter can be set

const octokit = new Octokit({
  auth: env.GITHUB_TOKEN,
  version: "ghes-3.0"
})

const response = octokit.request("GET /repos/{owner}/{repo}/hooks/{hook_id}/deliveries", {
  owner,
  repo,
  hook_id
})

The above code should not show a TypeScript error, explaining that the GET /repos/{owner}/{repo}/hooks/{hook_id}/deliveries endpoint is not present in GHES 3.1 and below.

On the other side, the response.headers type should now include { 'x-github-enterprise-version': string } in this example

const octokit = new Octokit({
  auth: env.GITHUB_TOKEN,
  version: "ghes-3.0"
})

const response = octokit.request("GET /")

while it wouldn't be set if the version option was set to "github.com" or not set at all.

So instead of creating entirely separate types for all the different platforms/versions, I think we should have the types built upon each other. The github.com version will include all the types we have today, then the ghes-3.1 endpoint types will extend the github.com endpoints add add its differences, e.g. sent some of the routes to never with a comment explaining that this endpoint is not availabil in GHES 3.1 and below, then ghes-3.0 will depend on ghes-3.1 and implement its differences, and so on.

Only the github.com types should be loaded by default. If a developer wants to build against ghes-3.0, they will have to install the respective types module and import it explicitly, e.g.

import { Octokit } from "octokit";
import "@octokit/types-endpoints-ghes-3.0";

const octokit = new Octokit({
  auth: env.GITHUB_TOKEN,
  version: "ghes-3.0"
})

const response = octokit.request("GET /")

That will import all necessary type differences between dotcom and GHES 3.1 as well as the differences between GHES 3.0 and 3.1

Compatibility versions

A common use case is to create code that is supposed to work against both dotcom and a minimal GHES version. For these cases we should create virtual compatibility versions, such as ghes-3.1-compatible, which would include types from both dotcom and GHES 3.1, but will flag the types that that do not exist in both as optional. So e.g. this code should throw errors

const octokit = new Octokit({
  auth: env.GITHUB_TOKEN,
  version: "ghes-3.0"
})

// error: `GET /repos/{owner}/{repo}/hooks/{hook_id}/deliveries` does not exist on GHES 3.1`
octokit.request("GET /repos/{owner}/{repo}/hooks/{hook_id}/deliveries", {
  owner,
  repo,
  hook_id
})
// error: `x-github-enterprise-version` header does not exist on `dotcom`
const response = await octokit.request("GET /")
console.log(response.headers["x-github-enterprise-version"])

To resolve the above error, the version must be permitted to be set explicitly on the request

octokit.request("GET /repos/{owner}/{repo}/hooks/{hook_id}/deliveries", {
  owner,
  repo,
  hook_id,
  request: { version: "github.com" },
})

const response = await octokit.request("GET /", { request: { version: "ghes-3.1" } })
console.log(response.headers["x-github-enterprise-version"])

Bonus: It would be nice if we could make the type code smart enough that e.g. checking for the presence of the x-github-enterprise-version header would work to set the version implicitly, so the code below wouldn't throw any errors.

const response = await octokit.request("GET /")
if ("x-github-enterprise-version" in response.headers) {
  // version is now implicitly set to `ghes-3.1`
  octokit.request("GET /repos/{owner}/{repo}/hooks/{hook_id}/deliveries", { owner, repo, hook_id })
}

Plugins

Plugin developers are a special use case, as they want their plugins to be compatible explicitly with specific version. I'm not sure what the best approach would be, maybe it would suffice to add automated tests which would set version to all supported platform type versions, and then set a compatibility matrix as meta information on the plugin.

But either way, the least we need is to be able to explicitly set version on each request. That means that plugins might have their own dependency on endpoints type modules such as @octokit/types-endpoints-ghes-3.0. Not ideal as depending on one plugin which depends on an old version of @octokit/types-endpoints-ghes-* might result in a big amount of types-only dependencies, and even result loading outdated versions, so the app ends up having multiple versions of the types packages.

@gr2m gr2m self-assigned this Jul 19, 2021
@gr2m gr2m transferred this issue from octokit/octokit.js Jul 19, 2021
@ghost ghost moved this from In progress to Inbox in JS Jul 19, 2021
@gr2m gr2m added the Type: Feature New feature or request label Jul 19, 2021
@ghost ghost moved this from Inbox to In progress in JS Jul 19, 2021
@gr2m gr2m changed the title Create a demo for a GHES 3.1 Octokit constructor which accepts a version option and uses endpoint types for octokit.request that are specific to GHES 3.1. It should also extend response types to include headers specific to GHES versions Make the Octokit constructor derive types for octokit.request based on the version option passed to the constructor Aug 12, 2021
@gr2m gr2m closed this as completed Aug 12, 2021
JS automation moved this from In progress to Done Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request typescript
Projects
No open projects
JS
  
Done
Development

No branches or pull requests

1 participant