fix(typescript): adapt for @octokit/[email protected]

[wolfy1339]

This patch fixes #212, however it currently breaks the automatic type inferance for event payloads, thus making the tests fail

[G-Rath]

@wolfy1339 it'd be good if you could just enable strict :)

[wolfy1339]

strict is enabled (from the root tsconfig.json, which is extended in tests/tsconfig.json)

---

The following lines of code in the tests are erroring (see octokit/webhooks.js#490 as well)

app.js/test/readme-examples.test.ts

Lines 181 to 191 in b1ab5f7

	app.webhooks.on("issues.opened", async ({ octokit, payload }) => {
	await octokit.request(
	"POST /repos/{owner}/{repo}/issues/{issue_number}/comments",
	{
	owner: payload.repository.owner.login,
	repo: payload.repository.name,
	issue_number: payload.issue.number,
	body: "Hello World!",
	}
	);
	});

Error message:

Property 'repository' does not exist on type 'CheckRunCompletedEvent | CheckRunCreatedEvent | CheckRunRequestedActionEvent | CheckRunRerequestedEvent | ... 313 more ... | WorkflowDispatchEvent'.
  Property 'repository' does not exist on type 'never'.ts(2339)

[G-Rath]

Then strictNullChecks is already enabled, as that's covered by strict.

[wolfy1339]

Do you have any idea how to fix the error i mentioned above?

[G-Rath]

I can tell you why it's happening: the generic is being destroyed by this:

Webhooks<
  Required<Options>["webhooks"] & {
    path: "/api/github/webhooks";
    transform: (
      event: EmitterWebhookEvent
    ) => Promise<EmitterWebhookEvent & { octokit: Octokit }>;
  }
>

I honestly don't think it can be solved in its current form - the problem is that transform can return anything (vs requring returning <event> & T), otherwise it'd be easy to preserve the generics since we'd just be doing <event> & T rather than T.

[gr2m]

I honestly don't think it can be solved in its current form - the problem is that transform can return anything (vs requring returning <event> & T), otherwise it'd be easy to preserve the generics since we'd just be doing <event> & T rather than T.

I'd be okay with enforcing that transform can only amend the current event type, not change or replace it. It would make things more predictable anyway. Would that help?

[G-Rath]

That's be my preference as well as it should make things a lot easier to manage.

[gr2m]

as it should make things a lot easier to manage

that's music in my ears :) let's do it

[oscard0m]

Would this be the approach you were looking for? octokit/webhooks.js#492
I can use fix-212 as base branch if you want @wolfy1339

[wolfy1339]

Does that work? Does it keep the proper event typing?
ie:

webhooks.on("push", event => {
  // event should have type EmitterWebhookEvent<"push"> & { octokit: Octokit }
});

[wolfy1339]

I have tried a variance of this patch, setting the types via the generic of the Webhooks class. That didn't work

diff --git a/src/webhooks.ts b/src/webhooks.ts
index bd9d823..461615b 100644
--- a/src/webhooks.ts
+++ b/src/webhooks.ts
@@ -10,15 +10,15 @@ export function webhooks(
   options: Required<Options>["webhooks"]
   // Explict return type for better debugability and performance,
   // see https://github.com/octokit/app.js/pull/201
-): Webhooks<
-  Required<Options>["webhooks"] & {
-    path: "/api/github/webhooks";
-    transform: (
-      event: EmitterWebhookEvent
-    ) => Promise<EmitterWebhookEvent & { octokit: Octokit }>;
-  }
-> {
-  return new Webhooks({
+) {
+  return new Webhooks<
+    Required<Options>["webhooks"] & {
+      path: "/api/github/webhooks";
+      transform: (
+        event: EmitterWebhookEvent
+      ) => Promise<EmitterWebhookEvent & { octokit: Octokit }>;
+    }
+  >({
     secret: options.secret,
     path: "/api/github/webhooks",
     transform: async (event) => {

---

So then I tried removing all the explicit types, and then that didn't work either.
However, in the tests for @octokit/webhooks we have a test that uses a transform method, and it works just fine for the added property but not the event argument of the handler function.
The types are broken in @octokit/webhooks currently

[gr2m]

I really appreciate you looking into this. Thank you so much!

[wolfy1339]

This PR will not be necessary once octokit/webhooks.js#494 is merged