type of Octokit is incorrect

[SleeplessByte]

TL;DR don't union void if you want TypeScript to work as expected.

My original findings in #2112 follow below:

---

@gr2m I checked your PR on @neenjaw repro and yes it solves his issue, but I did find the actually issue with the type.

The type of Octokit is, regardless of what Tim does in his repro:

Octokit 
  & void 
  & { paginate: PaginateInterface } 
  & Api

This results in your PR working because octokit.graphql reaches into Octokit as can be seen here:

So far, so good, right?

The problem is that void that's there. This makes it so that you can't narrow this value when doing a truthy (or falsy) check, which is what Tim is running into. Let me demonstrate:

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

if (octokit) {
  octokit.graphql // nope
}

This is understandable behaviour. Something that is void can never be truthy. We can further proof that this is what happens by inspecting the else case:

if (octokit) {
} else {
  octokit.graphql // yep!
}

The source octokit.d.ts has this faulty type:

export declare const Octokit: typeof OctokitCore & import("@octokit/core/dist-types/types").Constructor<void & {
    paginate: import("@octokit/plugin-paginate-rest").PaginateInterface;
} & import("@octokit/plugin-rest-endpoint-methods/dist-types/types").Api>;

@neenjaw the reason this worked with earlier versions is that void didn't trump other types in the union, and now it does. As far as I can tell this has been the change that broke the type, but the change is correct. It shouldn't be void & { shape } but just be { shape }.

Originally posted by @SleeplessByte in #2112 (comment)

[gr2m]

I've no idea where the void is coming from there. It's quite a deep rabbit hole. I also don't see the use case ochecking if octokit is truthy right after you instantiate it? Why would you do that?

I agree the void should be removed, but unless it has a bigger impact I won't give this a high priority

[SleeplessByte]

You would do that if you want to pass octokit in as a variable, like:

function myThing(ocktokit: Octokit): void
function myThing(ocktokit: undefined): void
function myThing(ocktokit: null): void
function myThing(): void

function myThing(ocktokit?: Octokit | null | undefined): void {

}

...among many more use cases. Right now we can't pass octokit around unless we assume that it's always there or cast the type manually.

This also means you can't safely type a conditional initializer or memoize it.

[gr2m]

The problem was a plugin returning void. I fixed that now, but will also look into preventing that from happening again in the future. Sorry for the trouble. A new release for octokit is on its way

[github-actions]

🎉 This issue has been resolved in version 1.0.6 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[gr2m]

The problem should not happen again once octokit/core.js#356 is merged an released. However, I couldn't figure out a way to test for octokit being an intersection with void. I added a test to test/typescript-validate.ts, see https://github.com/octokit/core.js/pull/356/files, but that test passed before already.

Do you have an idea how to add a typescript-only check whether octokit has the intersection with void? I'd like preventing that from happening again

[SleeplessByte]

You could use the narrowing trick I used:

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

if (octokit) {
  octokit // should be the correct type, so `expectType` here. Should NOT be never
}

But I don't know if that will work with the test suite you have set up. When I must force type checkes like this, I usually use what the DefinitelyTyped repo uses.

[gr2m]

Thanks! This did the trick

expectType<void>(octokitWithVoidAndNonVoidPlugins);

That worked before octokit/core.js#356 but is now throwing a TypeScript error 👍🏼