Make Octokit.ApiVersions interface merging work with GHES additions in TypeScript

[gr2m]

This is a follow up to #7

Here is a video summarizing the problem. The file I'm showing is tests/ts/ghes-3.1/test.ts

2021-08-09.12-51-01.mp4

I also created a TypeScript playground with a very much simplified illustration of the problem I'm running into.

The @octokit-next/* packages are published from the packages/ folders. They are written in JavaScript and hand-written d.ts files.

When installing @octokit-next/core, then the version constructor parameter can only be set to "github.com" and the only declared REST API endpoint is the root endpoint GET /.

---

---

When installing @octokit-next/types-rest-api-github.com and importing it, then the additional endpoints show up

---

---

However, when installing and importing @octokit-next/types-rest-api-ghes-3.1, then the version constructor option cannot be set to "ghes-3.1", although it is added as key to the Octokit.ApiVersions interface:

---

---

octokit-next.js/packages/types-rest-api-ghes-3.1/index.d.ts

Lines 52 to 63 in f376a96

	declare module "@octokit-next/types" {
	namespace Octokit {
	interface ApiVersions {
	"ghes-3.1": {
	ResponseHeaders: GHES31ResponseHeaders;
	Endpoints: Octokit.ApiVersions["github.com"]["Endpoints"] &
	GHES30EndpointsDiff;
	};
	}
	}
	}

---

I added tests for both JavaScript (using tsd) and TypeScript (using tsc) examples in tests/js and tests/ts respectively.

The JS/DTS tests are passing, but the TypeScript test for GHES-3.1 is failing. And I'm not sure why that is turns out the the JS/DTS tests used a custom class where I explicitly set the versions. I now added a new tests/js/ghes-3.1 test that fails just like its TypeScript counterpart

[gr2m]

I found out that what does work is if I set the Octokit<TVersion> type argument explicitly

But that should be inferred from the version option by default 🤔

I also realized that the GHES-3.1 tests for JS create a custom class which sets the version type argument explicitly

octokit-next.js/tests/js/ghes-3.1/index.d.ts

Lines 1 to 15 in f376a96

	import { ExtendOctokitWith, Octokit } from "@octokit-next/core";
	import "@octokit-next/types-rest-api-ghes-3.1";
	export const OctokitGhes31: ExtendOctokitWith<
	Octokit<"ghes-3.1">,
	{
	defaults: {
	baseUrl: string;
	};
	}
	>;
	// support import to be used as a class instance type
	export type OctokitGhes31 = typeof OctokitGhes31;

While the TS tests does not do that

octokit-next.js/tests/ts/ghes-3.1/test.ts

Lines 1 to 18 in f376a96

	import { Octokit } from "@octokit-next/core";
	import "@octokit-next/types-rest-api-ghes-3.1";
	function expectType<T>(value: T): void {}
	export async function test() {
	const octokit = new Octokit({
	version: "ghes-3.1",
	});
	const dotcomOnlyResponse = await octokit.request("GET /dotcom-only");
	expectType<never>(dotcomOnlyResponse);
	const ghesOnlyResponse = await octokit.request("GET /ghes-only");
	expectType<boolean>(ghesOnlyResponse.data.ok);
	expectType<never>(ghesOnlyResponse.headers["x-dotcom-only"]);
	}

[gr2m]

I think I figured it out. I'll have to remove version from Octokit.Options and treat it entirely separate. That way it can be derived from the version options passed to the constructor.

playground

[github-actions]

🎉 This issue has been resolved in version 1.3.1 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

[nmkataoka]

@gr2m Hey I saw your post on the TS discord after you closed it but it looked interesting so I took a crack at it. If you're still looking at this, would something along these lines work?

playground

There's something I don't understand going on with TVersion not being inferred from TOptions so I just specified TVersion explicitly in the SDK constructor. Also request needed a generic on the route parameter.

[gr2m]

Thanks a lot Nolan for the follow up! Setting options: { version: TVersion } & TOptions in the constructor is exactly the solution I came up with as well, that way TVersion can be derived:

https://github.com/octokit/octokit-next.js/pull/10/files#diff-144904d408ec513806681518c894e534492562fdd352bba77ab8e7643851d978R209

I also removed version from API.Options to make it explicit everywhere I use it.

The next challenge I see is to make it work when creating custom classes using the static .withDefaults({ version }) method. Ideally I'd like this to work

import { Octokit } from "@octokit-next/core"
import "@octokit-next/types-rest-api-ghes-3.1"

const MyOctokit = Octokit.withDefaults({
  version: "ghes-3.1"
})

const octokit = new MyOctokit()
octokit.request("GET /ghes-only")

But that's another rabbit hole 🐰🕳️ I created a follow up issue for now #11