Open tg44 opened 2 years ago
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 😊
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...)