webhooks.js

webhooks.js copied to clipboard

What’s missing?

Add an optional function param to middleware that can add down additional data to the handlers.

Why?

I wanted to write an aggregator/relayer service, which can get multiple hooks from multiple repos and relay them to various endpoints. For this, I need the called URL in the handler. Later on, we find out, that if we want to use Cloudflare-workers + durable-storage we need the whole request context in the handler too.

Alternatives you tried

I have a working fork, but it is not really consistent with the type orders and things like that; https://github.com/tg44/octokit-webhooks.js

I can work on this to make it mergeable, but at least I need a review or guidance.

(Also, it should handle the security as an optional thing, and I needed to remove the .ref() from the setTimeout BCS some platforms has no .ref on setTimeout...)

Can you please explain what you need using code?

If you look at the diff of their changes, it could help understand: https://github.com/octokit/webhooks.js/compare/master...tg44:master

okay you want to add a additionalDataExtractor constructor option. When set, whatever data it returns will be passed to the event handlers?

Can you start a pull request that only updates the README and we discuss based on that?

Some code from a live project;

export interface AdditionalData {
  ctx: Context;
}

const additinalDataFromRequest = (request: any): AdditionalData => {
  const ctx = request.ctx as Context;
  return { ctx };
};

const webhooksMiddleware = createNodeMiddleware(webhooks, {
  path,
  additionalDataExtractor: additinalDataFromRequest,
});

webhooks.on("ping", handlePing);

export const handlePing = async ({
   payload,
   additionalData,
}: {
  payload: PingEvent;
  additionalData?: AdditionalData;
}) => {
  if (additionalData) {
    const creator = payload.sender.login;
    //this function needs the request context to get access to other things (for example additional headers that some proxy added, or the call url)
    const users = await enhanceUsernames([creator], additionalData.ctx);
     ...
   }
}

Also, I change the path matcher to a "starts with" in the middleware, so it now can match to "/mypath/generatedUrl1" and "/mypath/generatedUrl2", and I can reach the "generatedUrl1" string inside the handler.

EDIT: I also added a test so you can check a dummy use-case inside the code too.

@tg44 Do you think it would be enough just to pass the request (i.e. IncomingMessage) to the event handler as another argument? I think that might be a simpler design as you wouldn't need the additionalDataExtractor.

It would be something like this:

webhooks.on("ping", ({ payload, request }) => {
  console.log('Received request', request);
});

Do you think a regular expression could work for the path matching too, instead of using String.prototype.startsWith?

Both are generally a good idea. I started with the extractor, bcs I wanted it to be typed, and I could minimize the any cast with it (but I felt that its not really worth it).

The regexp is a more general option than a startsWith, but the startsWith was more general than an exact path match.

I started the issue for these comments exactly :)

One more thing; it would be really good if we would get an interface for te request and for the response. I needed to write a wrapper for sundler, and it was not easy bcs of the not typed interface. (I can share that code too if it helps.)

I like the idea of just passing the request to the event handler - with types, of course ✨ I'm not sure how soon I or someone else at GitHub will be able to work on it, so you'd be welcome to. Otherwise, we can keep this issue here.

I will prepare a PR then!

Wonderful! Happy to review 😊