feat: update @octokit/webhooks to v9

[wolfy1339]

This (tries to) resolves some type incompatibilities with @octokit/webhooks due to the change in the way we generate payload types, and some deprecated code that got removed.

/ref #1472
/cc @G-Rath @gr2m

Breaking Changes

• remove '*' event
• app.webhooks.middleware has been removed
• removes the webhookPath option on new Probot({}) for the webhooks middleware

Closes #1558

[wolfy1339]

Hey @G-Rath @oscard0m,

Could you guys help me out here (especially for the tests), make sure my changes will work as expected ?

[oscard0m]

I use probot as user but I'm not familiar with the code. Cloning the repo and taking a look into your PR. Additionally on my comments, are there any checks or tries you would like us to do? Generating a local probot and try something?

[wolfy1339]

Additionally on my comments, are there any checks or tries you would like us to do? Generating a local probot and try something?

Generating a local probot in typescript would be helpful, fixing up tests.

One thing I'm really not sure is the Context class, before it took the Webhook payload type, and the types didn't work very well whenever you had an Array of events, it would default to any. That would be awesome if we could land this in
probot with the update to @octokit/webhooks v8

[wolfy1339]

I've gotten things to work mostly, but it's not a great position atm.

I'd want input from @gr2m and @G-Rath for prettying things up and improvements on efficiency

[gr2m]

I won't have time to look into this for another 2-3 weeks, but I have it in my backlog

[wolfy1339]

This one is a breaking change, just pointing it out so you know to expect it

[gr2m]

is this change necessary because of the new types? Anyway to avoid it? It will be quite impactful. I want to make this change eventually anyway, but if we do it now we at least need to add a deprecation to the current version

[wolfy1339]

Yes, the change is required because of the new types because support for the *' event was removed in @octokit/webhooks v8

It would require rewriting a bunch of the types from @octokit/webhooks just to make this change backwards compatible
I don't think there is any way to avoid it

[wolfy1339]

These are the jest logs, and they are failing.

 npx jest

 FAIL  test/context.test.ts

  ● Context › config › gets a valid configuration

    context.repo() is not supported for this webhook event.

      94 |
      95 |     if (!repo) {
    > 96 |       throw new Error(
         |             ^
      97 |         "context.repo() is not supported for this webhook event."
      98 |       );
      99 |     }

      at Context.repo (src/context.ts:96:13)
      at Context.config (src/context.ts:216:25)
      at Object.<anonymous> (test/context.test.ts:191:36)

  ● Context › config › returns null when the file and base repository are missing

    context.repo() is not supported for this webhook event.

      94 |
      95 |     if (!repo) {
    > 96 |       throw new Error(
         |             ^
      97 |         "context.repo() is not supported for this webhook event."
      98 |       );
      99 |     }

      at Context.repo (src/context.ts:96:13)
      at Context.config (src/context.ts:216:25)
      at Object.<anonymous> (test/context.test.ts:207:28)

  ● Context › config › accepts deepmerge options

    context.repo() is not supported for this webhook event.

      94 |
      95 |     if (!repo) {
    > 96 |       throw new Error(
         |             ^
      97 |         "context.repo() is not supported for this webhook event."
      98 |       );
      99 |     }

      at Context.repo (src/context.ts:96:13)
      at Context.config (src/context.ts:216:25)
      at Object.<anonymous> (test/context.test.ts:227:21)

 FAIL  test/server.test.ts
  ● Server › webhook handler (POST /) › should return 200 and run event handlers in app function

    expect(received).toContain(expected) // indexOf

    Expected substring: "POST / 200 -"
    Received string:    "POST / 400 - 1ms"

      94 |
      95 |       expect(output.length).toEqual(1);
    > 96 |       expect(output[0].msg).toContain("POST / 200 -");
         |                             ^
      97 |     });
      98 |
      99 |     test("shows a friendly error when x-hub-signature is missing", async () => {

      at Object.<anonymous> (test/server.test.ts:96:29)

  ● Server › webhook handler (POST /) › should return 200 and run event handlers in app function

    expect.assertions(3)

    Expected three assertions to be called but received two assertion calls.

      66 |   describe("webhook handler (POST /)", () => {
      67 |     it("should return 200 and run event handlers in app function", async () => {
    > 68 |       expect.assertions(3);
         |              ^
      69 |
      70 |       server = new Server({
      71 |         Probot: Probot.defaults({

      at Object.<anonymous> (test/server.test.ts:68:14)

  ● Server › webhook handler (POST /) › shows a friendly error when x-hub-signature is missing

    expect(received).toEqual(expected) // deep equality

    Expected: ObjectContaining {"msg": "Go to https://github.com/settings/apps/YOUR_APP and verify that the Webhook secret matches the value of the WEBHOOK_SECRET environment variable."}
    Received: {"hostname": "LENOVO-X1E2", "level": 30, "msg": "POST / 400 - 1ms", "name": "http", "pid": 402664, "req": {"headers": {"accept-encoding": "gzip, deflate", "connection": "close", "content-length": "6654", "content-type": "application/x-www-form-urlencoded", "host": "127.0.0.1:32991", "x-github-delivery": "3sw4d5f6g7h8", "x-github-event": "push"}, "id": "3sw4d5f6g7h8", "method": "POST", "remoteAddress": "::ffff:127.0.0.1", "remotePort": 43100, "url": "/"}, "res": {"headers": {"content-type": "application/json", "x-powered-by": "Express"}, "statusCode": 400}, "responseTime": 1, "time": 1619630683927}

      108 |         .expect(400);
      109 |
    > 110 |       expect(output[0]).toEqual(
          |                         ^
      111 |         expect.objectContaining({
      112 |           msg:
      113 |             "Go to https://github.com/settings/apps/YOUR_APP and verify that the Webhook secret matches the value of the WEBHOOK_SECRET environment variable.",

      at Object.<anonymous> (test/server.test.ts:110:25)


node:internal/process/promises:246
          triggerUncaughtException(err, true /* fromPromise */);
          ^

[UnhandledPromiseRejection: This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason "TypeError: response.writeHead is not a function".] {
  code: 'ERR_UNHANDLED_REJECTION'
}

node:internal/process/promises:246
          triggerUncaughtException(err, true /* fromPromise */);
          ^

[UnhandledPromiseRejection: This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason "TypeError: response.writeHead is not a function".] {
  code: 'ERR_UNHANDLED_REJECTION'
}
 FAIL  test/e2e/e2e.test.ts
  ● end-to-end-tests › hello-world app

    Command failed with exit code 1: bin/probot.js run ./test/e2e/hello-world.js

      114 |     } catch (error) {
      115 |       probotProcess.cancel();
    > 116 |       console.log((await probotProcess).stdout);
          |                    ^
      117 |     }
      118 |
      119 |     expect(httpMock).toHaveBeenCalledTimes(2);

      at makeError (node_modules/execa/lib/error.js:59:11)
      at handlePromise (node_modules/execa/index.js:114:26)
      at Object.<anonymous> (test/e2e/e2e.test.ts:116:20)

node:internal/process/promises:246
          triggerUncaughtException(err, true /* fromPromise */);
          ^

[UnhandledPromiseRejection: This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason "TypeError: response.writeHead is not a function".] {
  code: 'ERR_UNHANDLED_REJECTION'
}
node:internal/process/promises:246
          triggerUncaughtException(err, true /* fromPromise */);
          ^

[UnhandledPromiseRejection: This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason "TypeError: response.writeHead is not a function".] {
  code: 'ERR_UNHANDLED_REJECTION'
}
 FAIL  test/create-node-middleware.test.ts
  ● Test suite failed to run

    Call retries were exceeded

      at ChildProcessWorker.initialize (node_modules/jest-worker/build/workers/ChildProcessWorker.js:193:21)

Test Suites: 4 failed, 11 passed, 15 total
Tests:       6 failed, 6 skipped, 94 passed, 106 total
Snapshots:   7 passed, 7 total
Time:        6.174 s
Ran all test suites.

---

• The HTTP server isn't working as expected, related to the webhooks middleware and it timing out
• context.config() isn't working as expected
• The e2e test fails with no error given

@gr2m Can you take a look?

[gr2m]

@gr2m Can you take a look?

will do!

[gr2m]

after checking out your branch and running npm install, I get these TypeScript errors:

npm test   

> probot@0.0.0-development pretest /Users/gregor/Projects/probot/probot
> tsc --noEmit -p test

test/context.test.ts:12:26 - error TS7053: Element implicitly has an 'any' type because expression of type '0' can't be used to index type 'PushEvent'.
  Property '0' does not exist on type 'PushEvent'.

12 const pushEventPayload = (WebhookExamples.filter(
                            ~~~~~~~~~~~~~~~~~~~~~~~~
13   (event) => event.name === "push"
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
14 )[0] as WebhookDefinition<"push">).examples[0];
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

test/context.test.ts:15:28 - error TS7053: Element implicitly has an 'any' type because expression of type '0' can't be used to index type 'IssuesEvent'.
  Property '0' does not exist on type 'IssuesEvent'.

15 const issuesEventPayload = (WebhookExamples.filter(
                              ~~~~~~~~~~~~~~~~~~~~~~~~
16   (event) => event.name === "issues"
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
17 )[0] as WebhookDefinition<"issues">).examples[0];
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

test/context.test.ts:18:33 - error TS7053: Element implicitly has an 'any' type because expression of type '0' can't be used to index type 'PullRequestEvent'.
  Property '0' does not exist on type 'PullRequestEvent'.

18 const pullRequestEventPayload = (WebhookExamples.filter(
                                   ~~~~~~~~~~~~~~~~~~~~~~~~
19   (event) => event.name === "pull_request"
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
20 )[0] as WebhookDefinition<"pull_request">).examples[0];
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

test/probot.test.ts:30:10 - error TS2352: Conversion of type 'PushEvent | IssuesEvent | PullRequestEvent | StatusEvent | CheckRunEvent | CheckSuiteEvent | ... 46 more ... | WorkflowRunEvent' to type 'EmitterWebhookEvent<TName>["payload"][]' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  Type 'WorkflowRunRequestedEvent' is missing the following properties from type 'EmitterWebhookEvent<TName>["payload"][]': length, pop, push, concat, and 28 more.

30   return webhookExamples.filter((event) => event.name === name.split(".")[0])[0]
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
31     .examples as EmitterWebhookEvent<TName>["payload"][];
   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Any idea?

[wolfy1339]

That is actually an oversight of mine in octokit/webhooks#466
The type of examples should be WebhookEventMap[TName][] notice that it's an array vs non-array in the types

[wolfy1339]

In the mean time, you can apply this diff

--- a/node_modules/@octokit/webhooks-examples/index.d.ts
+++ b/node_modules/@octokit/webhooks-examples/index.d.ts
@@ -5,7 +5,7 @@ export type WebhookDefinition<
   name: TName;
   actions: string[];
   description: string;
-  examples: WebhookEventMap[TName];
+  examples: WebhookEventMap[TName][];
   properties: Record<
     string,
     {

[gr2m]

thanks, that helped 👍🏼

[gr2m]

I gotta run now, but just a reminder that we have to include the relevant breaking changes in the release notes for probot: https://github.com/octokit/webhooks.js/releases/tag/v9.0.0

the node middleware tests fail because the x-hub-signature-256 header is missing, which is now required. Probaly it's the reason why all the remaining tests are failing, can you check? I can check again tomorrow

GitHub

Release v9.0.0 · octokit/webhooks.js

BREAKING CHANGES

createWebhooksApi() has been removed. Use new Webhooks() instead

webhooks.middleware has been removed. Use createNodeMiddleware() instead

createMiddleware has been removed. U...

[gr2m]

I've rebased on latest master. There is a problem with the WebhookPayloadWithRepository, at least when I run the tests locally? I wonder if we should remove the type altogether now, or at least improve it based on the types we have from @octokit/webhooks-types? What do you think?

[wolfy1339]

I think the rebase wasn't done properly. There's commits in the PR that don't belong

The beta branch isn't up to date with master iirc

[wolfy1339]

I don't think there is a nice way to do anything with WebhookPayloadWithRepository.

If we remove it, the functions of the Context class will complain that the property they are checking doesn't exist, and TS complains that the generated types are too complex. Discussed a bit here: octokit/webhooks#461

If we try to update it, then TS will complain that event.payload types are incompatible with WebhookPayloadWithRepository.
I tried casting the type of event.payload to unknown and then WebhookEventWithRepository, but then the tests fail for unrelated reasons