Initial README

[gr2m]

This is going to be the replacement for the current https://github.com/octokit/webhooks.js

It implements some features that are currently built into probot (e.g. accepting eventNames as array for event handlers), it exposes a receiver module so folks can use it in server frameworks that don’t follow the function (request, response, next) {} middleware pattern (like hapi) and it implements asynchronous hooks so servers can respond with errors if anything goes wrong in their handlers.

This is only the README for discussion, I’m working on the implementation and tests but wanted to get your input as soon as possible :)

Open questions

• .on / .removeListener and/or .hook / .removeHook: Initial README #1 (comment) – I tend to just .on / .removeListener and make them work async
  -> e97371b
• Enforce usage of secret: yes/no? Initial README #1 (comment)
  -> 5f1b6b2
• Expose the .handle() method from the middleware’s receiver instance: Initial README #1 (comment)
  -> 🤔 differentiating webhook events from github.com and GHE #2

Preview 👀

ping @bkeepers, @anglinb, @JasonEtco, @hiimbex, @zeke

[zeke]

Is this going to be published to npm under the octokit scope? Do we own that?

Might also be good to add installation instructions to the README.

[JasonEtco]

This is looking awesome. My only bit of feedback: I wonder if there's a way to organize the Events and Actions in a different way, so that it isn't one tall list. Since actions are all "children" of events, maybe they could be organized like that? For example:

---

• pull_request
  • .opened
  • .closed
• issues
  • .opened
  • .closed

---

Event	Actions
pull_request	.opened, .closed
issues	.opened, .closed

[gr2m]

Is this going to be published to npm under the octokit scope? Do we own that?

Yes, that’s ours :)

Might also be good to add installation instructions to the README.

Good point :)

I wonder if there's a way to organize the Events and Actions in a different way, so that it isn't one tall list

Yes, I’m not happy with it myself yet. I wanted to make clear that both "issues" and "issues.opened" are valid events. I like your idea with the table better, I’ll add a sentence above it to explain that the two can be combined, and that "issues" gets triggered for all actions. UPDATE: done

[bkeepers]

💥 This is really great! I love how modular it is. A few minor comments below, but no major gripes.

[bkeepers]

Does async support need to be a separate method? Could we make on behave properly if a promise is returned or an error is thrown?

[gr2m]

We could but the Node EventEmitter is used so widely, it would break a widely used convention, so I’d rather not do that. I think event handlers are still great and they are more performent. Making the asynchronous handlers with a dedicated method makes it clear.

[bkeepers]

the Node EventEmitter is used so widely, it would break a widely used convention

What would the consequences be for those that expecting this to behave exactly as an EventEmitter? If they treat it as an EventEmitter, it should work exactly as they expect. The only difference being that if they return a promise or throw an exception, the receiver.handle (which already doesn't look like an EventEmitter) would be able to find out about it. Most people won't be exposed to that because they'll be using the middleware that abstracts this.

We're already breaking conventions with on accepting an array and the emitter being called handle instead of emit. As for performance, I'd have to see the implementation, but I'm skeptical that there will be a difference.

Making the asynchronous handlers with a dedicated method makes it clear.

Personally, I have a strong preference for libraries that just do the right thing with promises. Being forced to use a different method based on the return semantics of your method has a smell to me.

I'm fine with settling for two separate methods if we have to, but I would only do that if there was significant trade-offs to make on work with both sync and async handlers.

[gr2m]

The biggest risk I see is that people do something like this

middleware.on('installation', async (payload) => {
  await doSomethingThatTakesTooLong(payload)
})

I would not expect my .on handler to be blocking the response to the GitHub webhook. That’s probably the only relevant downside that I see.

How about we get rid of the .on / .removeListener methods instead? It will be easy to add them later if it turns out we need both

[gr2m]

🤔

I forgot to implement an "error" event which is something we should do. And with an error event I would definitely want to use the .on('error', handleError) API.

Now I’m tending towards just the .on() API as Brandon originally suggested 😁 We can my concern by documenting that it does not behave like a EventEmitter event handler if the passed function is async or if the function returns a promise

[bkeepers]

It'd be handy if .receiver was prefixed with create so it works with shorthand:

const {createReceiver} = require('@octokit/webhooks')

Same goes for all the examples below.

[gr2m]

you can do const createReceiver = require('@octokit/webhooks/receiver') :)

But yeah, I was thinking about that, too. I’m a friend of verbose methods, I’ll do the change

[bkeepers]

I think the most common use case will be people using the webhook handler as middleware, so my preference would be to have this first example be a version of the middleware example below.

[gr2m]

One thing that makes me think: should we support usage without setting a secret at all? 🤔 Is there a valid use case where people could not set a secret?

I think for the sake of simplicity and also to nudge people towards doing the right think, I would enforce the usage of a secret.

What do you all think?

[gr2m]

The current implementation would only pass the event payload to the event handlers, while the github-webhook-handler module emits an object with the properties {event, id, payload, protocol, host, url}

https://github.com/octokit/webhooks.js/blob/9e0d6364e0e3e0b07fc51237e25bdffafdd9174d/github-webhook-handler.js#L103-L110

I don’t see a use case for protocol, host or url, but I think it does make sense to pass the event name, request id and the payload as separate properties.

[JasonEtco]

I can see a use-case for usage with GitHub Enterprise to provide the host or url, if only to be able to explicitly verify that webhooks are coming from the correct GHE url. Don't know about protocol though.

[gr2m]

This would only be relevant if a secret would not be configured, right? I can imagine a case where the same webhook URL would handle events from both github.com and GHE and then distinguish them based on the host. But if we were to enforce the usage of secret – which I think we should – this becomes a non issue, right?

I’d like to make the decision on wether to enforce secret or not first. I had a discussion with Carter the other day at Offline Camp. He said that one of the biggest trouble they had was to explain what the secret is and why it’s important. Folks would build webhook handlers ignoring it. I think one way to avoid this is to provide tooling that enforces the best practise. Unless someone has a good use case for why it should not

[JasonEtco]

Totally fair - the use of secrets is a valid point of why my proposed use case isn't right. However, new use case: if my app writes comments that link to other issues or repos, those URLs will need to have the host associated so that one app can be installed against both GHE and GitHub.com, but still point URLs to the appropriate resource.

[gr2m]

The host, protocal and url are not for the GitHub instance sending the webhook, but for the receiving webhook event server. They only thing this could be helpful is if you have the same server instance receiving events from different GitHub instances at different domains, I think?

I think your use case is interesting though, for that to work we would need a referer header though. And ideally a header which tells us the GHE version ..., maybe let’s create a follow up issue for that and raise it with the GitHub platform folks?

[JasonEtco]

The host, protocal and url are not for the GitHub instance sending the webhook, but for the receiving webhook event server.

Oooh I see what you mean, that makes sense. Sorry about the confusion.

a referer header though

That's a good idea.

[gr2m]

created a follow up issue to get back to it later: #2

[gr2m]

I think we also need to expose the .handle() method from the middleware’s receiver instance. This make testing easier, and we currently expose a .receive function on Probot’s robot instance here: https://github.com/probot/probot/blob/83b7454f74160d81193e447012e3c507c0092c4e/lib/robot.js#L22-L26

I’m not sure if we should call the method middleware.handle() though, or something that makes more clear that it’s meant for testing or at least make folks think twice before using it, as it works around some safety measurements that the middleware implements, e.g. middleware.inject(). Any thoughts on that?

[gr2m]

Another possibility would be to support an API like this

const Webhooks = require('@octokit/webhooks')
const webhooks = new Webhooks({secret: 'mysecret', middleware: {path: '/custom/webhooks'})

// webhooks has pre-initialised .middleware & .receiver. 
// webhooks.middleware is using the webhooks.reicever instance internally

const express = require('express')
const app = express()
app.use(options.webhookPath || '/', webhooks.middleware)

const createRobot = require('./lib/robot')
const robot = createRobot({app, webhooks})

// the createRobot method sets
// robot.on = webhooks.receiver.on
// robot.removeListener = webhooks.receiver.removeListener
// robot.inject = webhooks.receiver.handle

^^ This makes a lot of sense to me. Instead of exposing the same static methods at require('@octokit/webhooks').middleware and require('@octokit/webhooks/middleware'), I would get rid of require('@octokit/webhooks').middleware, make Webhooks = require('@octokit/webhooks') a factory or a class .middleware and the other modules and methods pre-initialised with the secret passed to new Webhooks(options)

That would let folks use e.g. '@octokit/webhooks/middleware' standalone. But if they want to inject an event or use the .sign / .verify method, they would use the @octokit/webhooks module which puts it all together.

I will give this some more thought and will try this out on probot. Happy to hear what y’all think.

[gr2m]

ugh accidentally pushed to master and this PR got auto-merged. Is there no way I can re-open it? :(