Add types - #661

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

This is definitely not the first stab at this, see #492

As for request being passed to itself, it was added as part of the following commit f10aa37

Switching away from Bottleneck would really be appreciated, but I don't know of any alternatives that offer the same functionality

This is definitely not the first stab at this, see #492

Sorry, yes, I only noticed that after I opened this PR. Would you be happy to proceed with/review this PR or do you prefer updating/merging #492?

As for request being passed to itself, it was added as part of the following commit f10aa37

Looks like the specific commit added all of requestWithGraphqlErrorHandling and there was never a version of it with just request(options). I'll have another look at this later to see if including request as a first argument makes a difference. If it does, I'll try to add a test for it, since it seems like the tests are currently passing without it.

Broadly, we are using this plugin in https://github.com/github/codeql-action and have run into some limitations for our usage that we would like to make some improvements for here, rather than just implementing something for ourselves.

I'm fine either way. Thank you for the PR.

These repositories are in dire need of some love.

It looks good to me, I'll test it further a little later

While adding types for requestWithGraphqlErrorHandling, I was surprised to see that request is called with itself as an argument (await request(request, options)) which doesn't seem to line up with the type that request gets from octokit.hook.wrap("request", ...). I removed it and it didn't seem to break anything in the tests, but maybe I am missing some good reason why that was there before.

I have investigated this a bit and traced this down to a bug in the before-after-hook library. Fundamentally, the request(request, options) call is wrong, since request only expects options as argument, so the version in this PR is correct. However, currently it doesn't matter what we have here because of the bug in before-after-hook. The TL;DR: is that the same options object is passed to each wrapper. For example:

const collection = new Hooks.Collection();

async function wrappee(options) {
    console.info("In wrappee");
    console.log(options);
}

collection.wrap("test", (fn, opts) => {
    console.info("In first wrapper");
    console.log(opts);
    fn({...opts, first: true });
});

collection.wrap("test", (fn, opts) => {
    console.info("In second wrapper");
    console.log(opts);
    fn({...opts, second: true });
});

await collection("test", wrappee, {});

outputs:

In second wrapper
{}
In first wrapper
{}
In wrappee
{ first: true }

In our case, that means that it doesn't matter what we call request with because, as long as it is another wrapper, the arguments are ignored and the wrapper is called with the initial options. The responsible code in before-after-hook is https://github.com/gr2m/before-after-hook/blob/main/lib/register.js#L23-L25

This binds the initial options to every wrapper call. Only the first wrapper that was installed, which calls the initial function directly, passes its argument to it.

I have opened a PR there to fix this: gr2m/before-after-hook#125

🎉 This PR is included in version 8.1.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀