fix(types): improvements to internal types and options parameter (#642

)

* fix(types): improvements to internal types and `options` parameter
* fix: remove unneeded `ts-expext-errror` directive
* chore: remove most `ts-expect-error` annotations
* Add internal `State` type
* Type the `wrapRequest()` function
* Add type augmentation of `OctokitResponse`

* fix: improve options type

* style: prettier

* fix: add missing type import

* fix: move `ts-expext-error` directive

* test: add `ts-expect-error`

* build(deps): update `@octokit/types`

* chore: remove more `ts-expect-error`
* chore: move `ts-expect-error` directive directly before function
* build: workaround typescript

Build the project first, then import from the build and then do the type check

package.json

@@ -14,13 +14,13 @@
     "update-endpoints": "npm-run-all update-endpoints:*",
     "update-endpoints:fetch-json": "node scripts/update-endpoints/fetch-json",
     "update-endpoints:code": "node scripts/update-endpoints/code",
-    "validate:ts": "tsc --noEmit --noImplicitAny --target es2020 --esModuleInterop --moduleResolution node test/typescript-validate.ts"
+    "validate:ts": "npm run build && tsc --noEmit --noImplicitAny --target es2020 --esModuleInterop --moduleResolution node test/typescript-validate.ts"
   },
   "repository": "github:octokit/plugin-throttling.js",
   "author": "Simon Grondin (http://github.com/SGrondin)",
   "license": "MIT",
   "dependencies": {
-    "@octokit/types": "^12.0.0",
+    "@octokit/types": "^12.2.0",
     "bottleneck": "^2.15.3"
   },
   "peerDependencies": {

src/index.ts

@@ -1,22 +1,32 @@
 // @ts-expect-error
 import BottleneckLight from "bottleneck/light";
+import type TBottleneck from "bottleneck";
 import { Octokit } from "@octokit/core";
 import type { OctokitOptions } from "@octokit/core/dist-types/types.d";
-import type { Groups, ThrottlingOptions } from "./types";
+import type { Groups, State, ThrottlingOptions } from "./types";
 import { VERSION } from "./version";
 
 import { wrapRequest } from "./wrap-request";
 import triggersNotificationPaths from "./generated/triggers-notification-paths";
 import { routeMatcher } from "./route-matcher";
+import type { EndpointDefaults, OctokitResponse } from "@octokit/types";
 
 // Workaround to allow tests to directly access the triggersNotification function.
 const regex = routeMatcher(triggersNotificationPaths);
 const triggersNotification = regex.test.bind(regex);
 
 const groups: Groups = {};
 
-// @ts-expect-error
-const createGroups = function (Bottleneck, common) {
+const createGroups = function (
+  Bottleneck: typeof TBottleneck,
+  common: {
+    connection:
+      | TBottleneck.RedisConnection
+      | TBottleneck.IORedisConnection
+      | undefined;
+    timeout: number;
+  },
+) {
   groups.global = new Bottleneck.Group({
     id: "octokit-global",
     maxConcurrent: 10,
@@ -45,7 +55,7 @@ const createGroups = function (Bottleneck, common) {
 export function throttling(octokit: Octokit, octokitOptions: OctokitOptions) {
   const {
     enabled = true,
-    Bottleneck = BottleneckLight,
+    Bottleneck = BottleneckLight as typeof TBottleneck,
     id = "no-id",
     timeout = 1000 * 60 * 2, // Redis TTL: 2 minutes
     connection,
@@ -59,15 +69,15 @@ export function throttling(octokit: Octokit, octokitOptions: OctokitOptions) {
     createGroups(Bottleneck, common);
   }
 
-  const state = Object.assign(
+  const state: State = Object.assign(
     {
       clustering: connection != null,
       triggersNotification,
       fallbackSecondaryRateRetryAfter: 60,
       retryAfterBaseValue: 1000,
       retryLimiter: new Bottleneck(),
       id,
-      ...groups,
+      ...(groups as Required<Groups>),
     },
     octokitOptions.throttle,
   );
@@ -100,9 +110,12 @@ export function throttling(octokit: Octokit, octokitOptions: OctokitOptions) {
     octokit.log.warn("Error in throttling-plugin limit handler", e),
   );
 
-  // @ts-expect-error
   state.retryLimiter.on("failed", async function (error, info) {
-    const [state, request, options] = info.args;
+    const [state, request, options] = info.args as [
+      State,
+      OctokitResponse<any>,
+      Required<EndpointDefaults>,
+    ];
     const { pathname } = new URL(options.url, "http://github.test");
     const shouldRetryGraphQL =
       pathname.startsWith("/graphql") && error.status !== 401;
@@ -174,6 +187,11 @@ export function throttling(octokit: Octokit, octokitOptions: OctokitOptions) {
     }
   });
 
+  // The types for `before-after-hook` do not let us only pass through a Promise return value
+  // the types expect that the function can return either a Promise of the response, or diectly return the response.
+  // This is due to the fact that `@octokit/request` uses aysnc functions
+  // Also, since we add the custom `retryCount` property to the request argument, the types are not compatible.
+  // @ts-expect-error
   octokit.hook.wrap("request", wrapRequest.bind(null, state));
 
   return {};
@@ -187,4 +205,10 @@ declare module "@octokit/core/dist-types/types.d" {
   }
 }
 
+declare module "@octokit/types" {
+  interface OctokitResponse<T, S extends number = number> {
+    retryCount: number;
+  }
+}
+
 export type { ThrottlingOptions };

src/types.ts

@@ -1,9 +1,10 @@
 import { Octokit } from "@octokit/core";
+import type { EndpointDefaults } from "@octokit/types";
 import Bottleneck from "bottleneck";
 
 type LimitHandler = (
   retryAfter: number,
-  options: object,
+  options: Required<EndpointDefaults>,
   octokit: Octokit,
   retryCount: number,
 ) => void;
@@ -42,3 +43,13 @@ export type Groups = {
   search?: Bottleneck.Group;
   notifications?: Bottleneck.Group;
 };
+
+export type State = {
+  clustering: boolean;
+  triggersNotification: (pathname: string) => boolean;
+  fallbackSecondaryRateRetryAfter: number;
+  retryAfterBaseValue: number;
+  retryLimiter: Bottleneck;
+  id: string;
+} & Required<Groups> &
+  ThrottlingOptions;

src/wrap-request.ts

@@ -1,12 +1,25 @@
+import type { EndpointDefaults, OctokitResponse } from "@octokit/types";
+import type { State } from "./types";
+
 const noop = () => Promise.resolve();
 
-// @ts-expect-error
-export function wrapRequest(state, request, options) {
+export function wrapRequest(
+  state: State,
+  request: ((
+    options: Required<EndpointDefaults>,
+  ) => Promise<OctokitResponse<any>>) & { retryCount: number },
+  options: Required<EndpointDefaults>,
+) {
   return state.retryLimiter.schedule(doRequest, state, request, options);
 }
 
-// @ts-expect-error
-async function doRequest(state, request, options) {
+async function doRequest(
+  state: State,
+  request: ((
+    options: Required<EndpointDefaults>,
+  ) => Promise<OctokitResponse<any>>) & { retryCount: number },
+  options: Required<EndpointDefaults>,
+) {
   const isWrite = options.method !== "GET" && options.method !== "HEAD";
   const { pathname } = new URL(options.url, "http://github.test");
   const isSearch = options.method === "GET" && pathname.startsWith("/search/");
@@ -37,14 +50,19 @@ async function doRequest(state, request, options) {
     await state.search.key(state.id).schedule(jobOptions, noop);
   }
 
-  const req = state.global.key(state.id).schedule(jobOptions, request, options);
+  const req = state.global
+    .key(state.id)
+    .schedule<OctokitResponse<any>, Required<EndpointDefaults>>(
+      jobOptions,
+      request,
+      options,
+    );
   if (isGraphQL) {
     const res = await req;
 
     if (
       res.data.errors != null &&
-      // @ts-expect-error
-      res.data.errors.some((error) => error.type === "RATE_LIMITED")
+      res.data.errors.some((error: any) => error.type === "RATE_LIMITED")
     ) {
       const error = Object.assign(new Error("GraphQL Rate Limit Exceeded"), {
         response: res,

test/exports.test.ts

@@ -10,7 +10,9 @@ describe("Exports", function () {
     };
 
     options.enabled = false;
+    // @ts-expect-error
     options.onRateLimit(10, {}, {} as Octokit, 0);
+    // @ts-expect-error
     options.onSecondaryRateLimit(10, {}, {} as Octokit, 0);
   });
 });

test/typescript-validate.ts

@@ -1,5 +1,5 @@
 import { Octokit } from "@octokit/core";
-import { throttling } from "../src/index";
+import { throttling } from "../pkg";
 // ************************************************************
 // THIS CODE IS NOT EXECUTED. IT IS FOR TYPECHECKING ONLY
 // ************************************************************