Repository: octokit/webhooks.js
Pull request #494 (2021-03-14 18:17:49)

fix(typescript): revert #459 & #464
## 📝 Summary
<!--- Provide a general summary of your changes, and if you feel that the commit comments are not descriptive enough, give more detail of your changes -->
Reverts the following PR's: 
* https://github.com/octokit/webhooks.js/pull/464
* https://github.com/octokit/webhooks.js/pull/459

## ⛱ Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
Types for webhooks are broken when a `transform` function is passed in the options: 
https://github.com/octokit/app.js/pull/213#issuecomment-798928569

Follow up conversation:
https://github.com/octokit/webhooks.js/pull/492#issuecomment-798938503

wolfy1339: This fixes the problem.

What about the scenario brought up in #464? Since we only allow to append to the event and not change it completely.
oscard0m: > This fixes the problem.

I would like to add a test in `ypescript.validate.ts` to cover the case we are fixing so we avoid regressions in the future

> What about the scenario brought up in #464? Since we only allow to append to the event and not change it completely.

I would create an issue (I think there was not one before) so we keep discussing this. What do you think?

wolfy1339: > I would like to add a test in `typescript.validate.ts` to cover the case we are fixing so we avoid regressions in the future

That should be as easy, just add an event handler for a certain event (ex: `issues`), and then try to access a property that exists only on that event (ex in the `issues` event: `issue`)

> I would create an issue (I think there was not one before) so we keep discussing this. What do you think?

Yes, we should create an issue.
oscard0m: > > I would like to add a test in `typescript.validate.ts` to cover the case we are fixing so we avoid regressions in the future
> 
> That should be as easy, just add an event handler for a certain event (ex: `issues`), and then try to access a property that exists only on that event (ex in the `issues` event: `issue`)

We have this already, and seems it did not prevent us to break types:

https://github.com/octokit/webhooks.js/blob/01c7655b556fea9ed0b89aeb7a9afdf8f2acc2cc/test/typescript-validate.ts#L164-L166

This is the test example you were suggesting right?
wolfy1339: That doesn't have the `transform` method, but it is the exact sort of test that needs to be added. Look further down, there is another section of the tests where we have the `transform` method

https://github.com/octokit/webhooks.js/blob/01c7655b556fea9ed0b89aeb7a9afdf8f2acc2cc/test/typescript-validate.ts#L179-L182
oscard0m: > That doesn't have the `transform` method, but it is the exact sort of test that needs to be added. Look further down, there is another section of the tests where we have the `transform` method
> 
> https://github.com/octokit/webhooks.js/blob/01c7655b556fea9ed0b89aeb7a9afdf8f2acc2cc/test/typescript-validate.ts#L179-L182

Sorry, I think I'm not understanding. `webhooks` is initialized with 

> That doesn't have the `transform` method, but it is the exact sort of test that needs to be added. Look further down, there is another section of the tests where we have the `transform` method
> 
> https://github.com/octokit/webhooks.js/blob/01c7655b556fea9ed0b89aeb7a9afdf8f2acc2cc/test/typescript-validate.ts#L179-L182

Ok, but with changes introduced by this PR it passes the `transform` method in [L75](https://github.com/octokit/webhooks.js/pull/494/files#diff-f0e1dffba6fc83b2e2bfe81aeab3e340f2b50b5cbf6aac225bfc5dc2536ea4eaR75) to all following lines so we are covering the use case now, right?
wolfy1339: After a second look, I can say that that should do the trick for the purposes of this PR
oscard0m: > After a second look, I can say that that should do the trick for the purposes of this PR

Added a comment nlinking to this PR for that line to give context for other developers before modifying it in the future
wolfy1339: Closes octokit/app.js#213

Patch:
diff --git a/src/event-handler/index.ts b/src/event-handler/index.ts
index 754081b..24393e6 100644
--- a/src/event-handler/index.ts
+++ b/src/event-handler/index.ts
@@ -15,23 +15,23 @@ import {
 import { receiverHandle as receive } from "./receive";
 import { removeListener } from "./remove-listener";
 
-interface EventHandler<T extends Options> {
+interface EventHandler<TTransformed> {
   on<E extends EmitterWebhookEventName>(
     event: E | E[],
-    callback: HandlerFunction<E, T["transform"]>
+    callback: HandlerFunction<E, TTransformed>
   ): void;
   onAny(handler: (event: EmitterWebhookEvent) => any): void;
   onError(handler: (event: WebhookEventHandlerError) => any): void;
   removeListener<E extends EmitterWebhookEventName>(
     event: E | E[],
-    callback: HandlerFunction<E, T["transform"]>
+    callback: HandlerFunction<E, TTransformed>
   ): void;
   receive(event: EmitterWebhookEvent): Promise<void>;
 }
 
-export function createEventHandler<T extends Options>(
-  options?: T
-): EventHandler<T> {
+export function createEventHandler<TTransformed>(
+  options: Options<TTransformed>
+): EventHandler<TTransformed> {
   const state: State = {
     hooks: {},
     log: createLogger(options && options.log),
diff --git a/src/index.ts b/src/index.ts
index 08c2bf0..12ac19d 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -17,18 +17,18 @@ import {
 import { verify } from "./verify/index";
 
 // U holds the return value of `transform` function in Options
-class Webhooks<T extends Options> {
+class Webhooks<TTransformed> {
   public sign: (payload: string | object) => string;
   public verify: (eventPayload: string | object, signature: string) => boolean;
   public on: <E extends EmitterWebhookEventName>(
     event: E | E[],
-    callback: HandlerFunction<E, T["transform"]>
+    callback: HandlerFunction<E, TTransformed>
   ) => void;
   public onAny: (callback: (event: EmitterWebhookEvent) => any) => void;
   public onError: (callback: (event: WebhookEventHandlerError) => any) => void;
   public removeListener: <E extends EmitterWebhookEventName>(
     event: E | E[],
-    callback: HandlerFunction<E, T["transform"]>
+    callback: HandlerFunction<E, TTransformed>
   ) => void;
   public receive: (event: EmitterWebhookEvent) => Promise<void>;
   public middleware: (
@@ -40,7 +40,7 @@ class Webhooks<T extends Options> {
     options: EmitterWebhookEvent & { signature: string }
   ) => Promise<void>;
 
-  constructor(options: T) {
+  constructor(options: Options<TTransformed>) {
     if (!options || !options.secret) {
       throw new Error("[@octokit/webhooks] options.secret required");
     }
diff --git a/src/types.ts b/src/types.ts
index b25b3ff..7e3bd72 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -32,12 +32,8 @@ type TransformMethod<T> = (event: EmitterWebhookEvent) => T | PromiseLike<T>;
 
 export type HandlerFunction<
   TName extends EmitterWebhookEventName,
-  TTransform
-> = (
-  event: TTransform extends TransformMethod<infer T>
-    ? T
-    : EmitterWebhookEvent<TName>
-) => any;
+  TTransformed
+> = (event: EmitterWebhookEvent<TName> & TTransformed) => any;
 
 type Hooks = {
   [key: string]: Function[];


Test patch:
diff --git a/test/integration/event-handler-test.ts b/test/integration/event-handler-test.ts
index 278e171..8b5e15b 100644
--- a/test/integration/event-handler-test.ts
+++ b/test/integration/event-handler-test.ts
@@ -135,7 +135,7 @@ test("options.transform", (done) => {
     },
   });
 
-  eventHandler.on("push", (event: string) => {
+  eventHandler.on("push", (event: EmitterWebhookEvent) => {
     expect(event).toBe("funky");
 
     done();
@@ -155,7 +155,7 @@ test("async options.transform", (done) => {
     },
   });
 
-  eventHandler.on("push", (event: string) => {
+  eventHandler.on("push", (event: EmitterWebhookEvent) => {
     expect(event).toBe("funky");
     done();
   });
diff --git a/test/typescript-validate.ts b/test/typescript-validate.ts
index ba46570..8a9e3fb 100644
--- a/test/typescript-validate.ts
+++ b/test/typescript-validate.ts
@@ -72,6 +72,10 @@ export default async function () {
   const webhooks = new Webhooks({
     secret: "blah",
     path: "/webhooks",
+    transform: (event) => {
+      console.log(event.payload);
+      return Object.assign(event, { foo: "bar" });
+    },
   });
 
   // Check named exports of new API work
@@ -162,19 +166,9 @@ export default async function () {
   });
 
   webhooks.on("issues", (event) => {
+    // ⚠️ This test is for assuring 'transform' method is preserving event.payload
     console.log(event.payload.issue);
   });
-}
-
-{
-  const webhooks = new Webhooks({
-    secret: "blah",
-    path: "/webhooks",
-    transform: (event) => {
-      console.log(event.payload);
-      return Object.assign(event, { foo: "bar" });
-    },
-  });
 
   webhooks.on("issues", (event) => {
     // foo is set by options.transform