Remove ts-ignores annotations

[copperwall]

This removes existing ts-ignore annotations from plugin-retry.js. I did my best to use existing types from @octokit/types and @octokit/core instead of creating new types or interfaces in this project.

I did have some trouble getting rid of the type errors in wrap_request.ts on the bottleneck/light import. The bottleneck package does specify typings, but it looks like by using the light import, TypeScript can't relate those types to the exports from bottleneck/light.

I also tried placing

import type Bottlneck from 'bottleneck'
declare module "bottleneck/light" {
   export default Bottleneck
}

in a local bottleneck.d.ts file to import the Bottleneck namespace from the module and export it under the bottleneck/light module to appease TypeScript, but I got an error about how those types couldn't be applied to bottleneck/light import. The only thing that seemed to work was copying the type definitions into the library.

I'm open to looking for more solutions other than copying 1000+ lines of another modules types into this one, but this is the solution I've found so far.

Closes #43

[copperwall]

Oops, it looks like I'm going to have to put a little more time into this.

[gr2m]

@SGrondin I wonder if you could export the types for bottleneck/light, too, so we don't have to add them?

[copperwall]

@gr2m @SGrondin I made a PR in bottleneck to export types for bottleneck/light as well! SGrondin/bottleneck#160

[gr2m]

@copperwall as SGrondin/bottleneck#160 shipped now, could you update the PR?

[copperwall]

Oh sure thing, I'll get back on this

[copperwall]

@gr2m I've pushed up a commit that removes the extra types I added to this repo, but it looks like the changes in bottleneck haven't made it into a release yet.

[gr2m]

Ah sorry, I I'll ping @SGrondin ☝🏼

[wolfy1339]

This would be really nice to merge.

Since the bottleneck repo hasn't seen activity in a while, I propose that we just don't apply typings for bottleneck, or we vendor in the file ourselves in the repo.

What are your thoughts? @octokit/js

[G-Rath]

I'm -1 on vendoring if we can avoid it, which if the issue is missing types we 100% should be able to by providing the types ourselves here.

I agree it's less than ideal to be using a library that isn't being actively maintained anymore - does anyone know of a possible alternative?

[gr2m]

I'm -1 on vendoring if we can avoid it, which if the issue is missing types we 100% should be able to by providing the types ourselves here.

Yeah I'd add the types ourselves.

I agree it's less than ideal to be using a library that isn't being actively maintained anymore - does anyone know of a possible alternative?

I do not know of any. But things do tend to pop up if a library like bottleneck becomes stale.

I'd get this pr unblocked in the simplest way possible, and then look for a bottleneck alternative.

[gr2m]

Add this point I suggest we cut our own release of bottleneck. It doesn't look like any new changes are coming, and it's been nearly 3 years. I would release our own @octokit/bottleneck based on the latest code, just not advertise it anywhere. We will have to find an alternative to Bottleneck eventually. It's kind of a ticking bomb. And as we move everything to ESM, we would want our dependencies to be ESM as well.

[ollie-iterators]

https://js.libhunt.com/bottleneck-alternatives

[gr2m]

@ollie-iterators We would need to find an alternative that does check our boxes. We need a library that works in all modern JS environments and optionally supports clustering using Redis. I did some research but couldn't find one that would work yet.

[ollie-iterators]

@ollie-iterators We would need to find an alternative that does check our boxes. We need a library that works in all modern JS environments and optionally supports clustering using Redis. I did some research but couldn't find one that would work yet.

Ok, good luck.