feat: deprecate `new Application({ throttleOptions })` (#1365)

src/application.ts

@@ -10,7 +10,6 @@ import { Context } from "./context";
 import { getAuthenticatedOctokit } from "./octokit/get-authenticated-octokit";
 import { ProbotOctokit } from "./octokit/probot-octokit";
 import { getLog } from "./helpers/get-log";
-import { getOctokitThrottleOptions } from "./octokit/get-octokit-throttle-options";
 import { getProbotOctokitWithDefaults } from "./octokit/get-probot-octokit-with-defaults";
 import { DeprecatedLogger, ProbotWebhooks, State } from "./types";
 import { webhookEventCheck } from "./helpers/webhook-event-check";
@@ -31,8 +30,12 @@ export interface Options {
   // Application class specific options
   cache?: LRUCache<number, string>;
   octokit?: InstanceType<typeof ProbotOctokit>;
-  throttleOptions?: any;
   webhooks?: Webhooks;
+
+  /**
+   * @deprecated set `Octokit` to `ProbotOctokit.defaults({ throttle })` instead
+   */
+  throttleOptions?: any;
 }
 
 export type OnCallback<T> = (context: Context<T>) => Promise<void>;
@@ -70,21 +73,23 @@ export class Application {
       appId: options.id,
       privateKey: options.privateKey,
       cache,
+      log: this.log,
+      redisConfig: options.redisConfig,
+      throttleOptions: options.throttleOptions,
     });
 
+    if (options.throttleOptions) {
+      this.log.warn(
+        `[probot] "new Application({ throttleOptions })" is deprecated. Use "new Application({Octokit: ProbotOctokit.defaults({ throttle }) })" instead`
+      );
+    }
+
     this.state = {
       cache,
       githubToken: options.githubToken,
       log: this.log,
       Octokit,
       octokit: options.octokit || new Octokit(),
-      throttleOptions:
-        options.throttleOptions ||
-        getOctokitThrottleOptions({
-          log: this.log,
-          throttleOptions: options.throttleOptions,
-          redisConfig: options.redisConfig,
-        }),
       webhooks: {
         path: options.webhookPath,
         secret: options.secret,

src/index.ts

@@ -22,7 +22,6 @@ import { createServer } from "./server/create-server";
 import { createWebhookProxy } from "./helpers/webhook-proxy";
 import { getErrorHandler } from "./helpers/get-error-handler";
 import { DeprecatedLogger, ProbotWebhooks, State } from "./types";
-import { getOctokitThrottleOptions } from "./octokit/get-octokit-throttle-options";
 import { getProbotOctokitWithDefaults } from "./octokit/get-probot-octokit-with-defaults";
 import { aliasLog } from "./helpers/alias-log";
 import { logWarningsForObsoleteEnvironmentVariables } from "./helpers/log-warnings-for-obsolete-environment-variables";
@@ -199,9 +198,6 @@ export class Probot {
 
     this.apps = [];
 
-    // TODO: Refactor tests so we don't need to make this public
-    this.options = options;
-
     // TODO: support redis backend for access token cache if `options.redisConfig || process.env.REDIS_URL`
     const cache = new LRUCache<number, string>({
       // cache max. 15000 tokens, that will use less than 10mb memory
@@ -216,13 +212,10 @@ export class Probot {
       appId: options.id,
       privateKey: options.privateKey,
       cache,
-    });
-    const octokit = new Octokit();
-
-    this.throttleOptions = getOctokitThrottleOptions({
       log: this.log,
       redisConfig: options.redisConfig,
     });
+    const octokit = new Octokit();
 
     this.state = {
       id: options.id,
@@ -232,7 +225,6 @@ export class Probot {
       log: this.log,
       Octokit,
       octokit,
-      throttleOptions: this.throttleOptions,
       webhooks: {
         path: options.webhookPath,
         secret: options.secret,
@@ -248,6 +240,9 @@ export class Probot {
 
     const { version } = require("../package.json");
     this.version = version;
+
+    // TODO: Refactor tests so we we can remove these
+    this.options = options;
   }
 
   /**

src/octokit/get-authenticated-octokit.ts

@@ -4,21 +4,13 @@ export async function getAuthenticatedOctokit(
   state: State,
   installationId?: number
 ) {
-  const { githubToken, log, Octokit, octokit, throttleOptions } = state;
+  const { githubToken, log, Octokit, octokit } = state;
 
   if (!installationId) return octokit;
 
   const constructorAuthOptions = githubToken
     ? {}
     : { auth: { installationId: installationId } };
-  const constructorThrottleOptions = throttleOptions
-    ? {
-        throttle: {
-          id: installationId,
-          ...throttleOptions,
-        },
-      }
-    : {};
 
   const pinoLog = log.child({ name: "github" });
   const options = {
@@ -30,8 +22,10 @@ export async function getAuthenticatedOctokit(
       debug: pinoLog.debug.bind(pinoLog),
       trace: pinoLog.trace.bind(pinoLog),
     },
+    throttle: {
+      id: installationId,
+    },
     ...constructorAuthOptions,
-    ...constructorThrottleOptions,
   };
 
   return new Octokit(options);

src/octokit/get-octokit-throttle-options.ts

@@ -4,7 +4,6 @@ import type { Logger } from "pino";
 
 type Options = {
   log: Logger;
-  throttleOptions?: any;
   redisConfig?: Redis.RedisOptions;
 };
 

src/octokit/get-probot-octokit-with-defaults.ts

@@ -1,13 +1,21 @@
 import { createAppAuth } from "@octokit/auth-app";
 import LRUCache from "lru-cache";
 import { ProbotOctokit } from "./probot-octokit";
+import Redis from "ioredis";
+
+import { getOctokitThrottleOptions } from "./get-octokit-throttle-options";
+
+import type { Logger } from "pino";
 
 type Options = {
   cache: LRUCache<number, string>;
   Octokit: typeof ProbotOctokit;
+  log: Logger;
   githubToken?: string;
   appId?: number;
   privateKey?: string;
+  redisConfig?: Redis.RedisOptions;
+  throttleOptions?: any;
 };
 
 /**
@@ -35,15 +43,33 @@ export function getProbotOctokitWithDefaults(options: Options) {
     baseUrl:
       process.env.GHE_HOST &&
       `${process.env.GHE_PROTOCOL || "https"}://${process.env.GHE_HOST}/api/v3`,
+    throttle: Object.assign(
+      {},
+      options.throttleOptions,
+      getOctokitThrottleOptions({
+        log: options.log,
+        redisConfig: options.redisConfig,
+      })
+    ),
     ...authOptions,
   };
 
   return options.Octokit.defaults((instanceOptions: any) => {
-    const options = Object.assign({}, defaultOptions, instanceOptions, {
+    const options = Object.assign(
+      {},
+      defaultOptions,
+      instanceOptions,
+      {
         auth: instanceOptions.auth
           ? Object.assign({}, defaultOptions.auth, instanceOptions.auth)
           : defaultOptions.auth,
-    });
+      },
+      {
+        throttle: instanceOptions.throttle
+          ? Object.assign({}, defaultOptions.throttle, instanceOptions.throttle)
+          : defaultOptions.throttle,
+      }
+    );
 
     return options;
   });

src/types.ts

@@ -13,7 +13,6 @@ export type State = {
   log: Logger;
   Octokit: typeof ProbotOctokit;
   octokit: InstanceType<typeof ProbotOctokit>;
-  throttleOptions: any;
   cache?: LRUCache<number, string>;
   webhooks: {
     path?: string;

test/deprecations.test.ts

@@ -3,7 +3,7 @@ import Stream from "stream";
 import { Webhooks } from "@octokit/webhooks";
 import pino from "pino";
 
-import { createProbot, Probot, ProbotOctokit } from "../src";
+import { Application, createProbot, Probot, ProbotOctokit } from "../src";
 
 describe("Deprecations", () => {
   let output: any;
@@ -87,4 +87,49 @@ describe("Deprecations", () => {
       `octokit.repos.createStatus() has been renamed to octokit.repos.createCommitStatus()`
     );
   });
+
+  it("throttleOptions", async () => {
+    expect.assertions(4);
+
+    const testLog = pino(streamLogsToOutput);
+    const log = ({
+      fatal: testLog.fatal.bind(testLog),
+      error: testLog.error.bind(testLog),
+      warn: testLog.warn.bind(testLog),
+      info: testLog.info.bind(testLog),
+      debug: testLog.debug.bind(testLog),
+      trace: testLog.trace.bind(testLog),
+      child: () => log,
+    } as unknown) as pino.Logger;
+    const Octokit = ProbotOctokit.plugin((octokit: any, options: any) => {
+      return {
+        pluginLoaded: true,
+        test() {
+          expect(options.throttle.id).toBe(1);
+          expect(options.throttle.foo).toBe("bar");
+        },
+      };
+    }).defaults({ log });
+
+    const app = new Application({
+      Octokit,
+      id: 1,
+      privateKey: "private key",
+      secret: "secret",
+      throttleOptions: {
+        foo: "bar",
+        onAbuseLimit: () => true,
+        onRateLimit: () => true,
+      },
+      log,
+    });
+
+    const installationOctokit = await app.auth(1);
+    installationOctokit.test();
+
+    expect(output.length).toEqual(1);
+    expect(output[0].msg).toContain(
+      `[probot] "new Application({ throttleOptions })" is deprecated. Use "new Application({Octokit: ProbotOctokit.defaults({ throttle }) })" instead`
+    );
+  });
 });

test/index.test.ts

@@ -66,34 +66,27 @@ describe("Probot", () => {
       process.env = env;
     });
 
-    it("runs with a function as argument", () => {
+    it("runs with a function as argument", async () => {
       let initialized = false;
 
-      return new Promise(async (resolve) => {
       probot = await Probot.run((app) => {
         initialized = true;
       });
       expect(probot.options).toMatchSnapshot();
       expect(initialized).toBeTruthy();
       probot.stop();
-
-        resolve();
-      });
     });
 
-    it("runs with an array of strings", () => {
-      return new Promise(async (resolve) => {
+    it("runs with an array of strings", async () => {
       probot = await Probot.run(["run", "file.js"]);
       expect(probot.options).toMatchSnapshot();
       probot.stop();
-        resolve();
-      });
     });
 
-    it("works with REDIS_URL configuration", () => {
+    it("works with REDIS_URL configuration", async () => {
       process.env.REDIS_URL = "redis://test:test@localhost:6379";
 
-      return new Promise(async (resolve, reject) => {
+      await new Promise(async (resolve, reject) => {
         const probot = await Probot.run((app) => {
           app.auth(1).then(resolve, reject);
         });
@@ -403,33 +396,39 @@ describe("Probot", () => {
     });
 
     it("sets throttleOptions", async () => {
+      expect.assertions(2);
+
       probot = new Probot({
         webhookPath: "/webhook",
         githubToken: "faketoken",
-      });
-
-      expect(probot.throttleOptions.Bottleneck).toBe(Bottleneck);
-      expect(probot.throttleOptions.connection).toBeInstanceOf(
+        Octokit: ProbotOctokit.plugin((octokit, options) => {
+          expect(options.throttle.Bottleneck).toBe(Bottleneck);
+          expect(options.throttle.connection).toBeInstanceOf(
             Bottleneck.IORedisConnection
           );
+        }),
+      });
     });
   });
 
   describe("redis configuration object", () => {
     it("sets throttleOptions", async () => {
+      expect.assertions(2);
       const redisConfig = {
         host: "test",
       };
+
       probot = new Probot({
         webhookPath: "/webhook",
         githubToken: "faketoken",
         redisConfig,
-      });
-
-      expect(probot.throttleOptions.Bottleneck).toBe(Bottleneck);
-      expect(probot.throttleOptions.connection).toBeInstanceOf(
+        Octokit: ProbotOctokit.plugin((octokit, options) => {
+          expect(options.throttle.Bottleneck).toBe(Bottleneck);
+          expect(options.throttle.connection).toBeInstanceOf(
             Bottleneck.IORedisConnection
           );
+        }),
+      });
     });
   });
 

test/integration/webhook.test.ts

@@ -56,7 +56,7 @@ describe("webhooks", () => {
     );
   });
 
-  test.only("logs webhook error exactly once", async () => {
+  test("logs webhook error exactly once", async () => {
     probot.load(() => {});
 
     await request(probot.server)