Add types

[mbg]

Adds typings throughout and resolves #43.

This should not change the behaviour of the plugin in any way.

A few notes:

• 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.
• The bottleneck library doesn't appear to be maintained and, while there is a commit in the repo there which adds typings for bottleneck/light, those are unreleased. I added some minimal typings for it in this PR, but in the long-term it would probably be good to move to something that is still maintained.

Closes #492
Closes #110

Pull request checklist

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• Yes
• No

---

[github-actions]

👋 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! 🚀

[wolfy1339]

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

[wolfy1339]

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

[mbg]

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.

[wolfy1339]

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

These repositories are in dire need of some love.

[wolfy1339]

Use interface declaration merging so that the types for the plugin can propogate to the Octokit constructor, and some other small points

[wolfy1339]

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

[mbg]

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

[wolfy1339]

After testing, everything seems to work just fine

[gr2m]

thanks for taking the time, looks great to me! I'm looking at your other PR as well

[gr2m]

love this 👍🏼

[github-actions]

🎉 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 📦🚀