Additional data from requests to handlers

[tg44]

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...)

[gr2m]

Can you please explain what you need using code?

[wolfy1339]

If you look at the diff of their changes, it could help understand:
master...tg44:master

[gr2m]

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?

[tg44]

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.

[timrogers]

@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?

[tg44]

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.)

[timrogers]

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.

[tg44]

I will prepare a PR then!

[timrogers]

Wonderful! Happy to review 😊