Middleware changes

[tg44]

fixes #682

There are three changes in this commit.

• I added an option to match paths with regex
• I pass down the original request to the handlers
• I typed the request and the response (as close as the code uses it)

The third mod could be opinionated. It matches the http interface and can be implemented in other HTTP frameworks too. Quick and dirty example for sunder;

import { Context } from "sunder";
import { ResponseData } from "sunder/util/response";
import { HeadersShorthands } from "sunder/context";

class WrappedRequest {
  request: Request & HeadersShorthands;
  headers: Record<string, string | string[] | undefined>;
  body: any;
  finished: Promise<unknown>;
  url: string;
  method: string;
  ctx: Context;
  setEncoding(enc: string) {};
  on(event: string, callback: (data: any) => void) {};

  constructor(ctx: Context) {
    const request = ctx.request;
    this.request = request;
    this.headers = Object.fromEntries(Array.from(request.headers.entries()));
    this.finished = request.json().then(async (d) => {
        this.body = d;
    });
    this.url = request.url;
    this.method = request.method;
    this.ctx = ctx;
  }
}

class WrappedResponse {
  response: ResponseData;

  set statusCode(value: number) {
    this.response.status = value;
  }

  end(body: string) {
    this.response.body = body;
  }

  writeHead(status: number, headers: Record<string, string>) {
    this.response.status = status;
    Object.entries(headers).map(([k, v]) => {
      this.response.headers.append(k, v);
    });
  }

  constructor(response: ResponseData) {
    this.response = response;
  }
}

export async function githubWebhooksMiddleware(ctx: Context, next: Function) {
  try {
    const request = new WrappedRequest(ctx);
    await request.finished;
    return webhooksMiddleware(
      request,
      new WrappedResponse(ctx.response),
      next
    );
  } catch (e) {
    console.log(e);
  }
}

[gr2m]

can you update the README please?

[tg44]

@gr2m now?

[timrogers]

Nice work on this! I appreciate you taking the time to make webhooks.js better ❤️

[timrogers]

What do you think of accepting a path and allowing it to be either a string or a RegExp? That interface feels nicer to me - although I'm not an expert in idiomatic JavaScript. It avoids the "override" complexity you have here, which could lead to surprises.

[tg44]

I'm not a js pro either, but something like a string | RegExp and later on a type matching to them seems to be more dangerous to me. But I can try it, a typeof string should split them correctly.

[timrogers]

Let’s wait and see what @gr2m or other maintainers think.

If we keep it this way, we should enforce that exactly one of the path options is provided.

[wolfy1339]

I'm not a js pro either, but something like a string | RegExp and later on a type matching to them seems to be more dangerous to me.

Can you explain why you think so?

I don't see the need to add another argument that has the exact same purpose

[tg44]

I fixed this also.

[timrogers]

Despite this any, we did clearly have some implicit assumptions before about what properties the IncomingMessage would have. Is the interface you define below common across many JavaScript libraries/frameworks?

[tg44]

I don't know if it is common or not, I just made those "implicit assumptions" to be explicit, so others could write a bridge more easily.

[wolfy1339]

They aren't really "implicit assumptions", the middleware was designed for NodeJS HTTP servers of the likes of express, and raw http.Server implementations.

We had to comment out the imports due to Deno compatibility (The link in the comment is out of date)
See octokit/octokit.js#2075 (comment) for context.
That's why they are currently set to any

[timrogers]

@wolfy1339 Thanks for the context! Do you think we should stick with any? Or can we define our own interface as @tg44 has?

[wolfy1339]

I am not sure. I have no clue when it comes to Deno, or other environments.

It would probably be best to write new middleware designed with other platforms at mind

[tg44]

I had this in mind, but that would mean that we need to publish multiple small packages (like middleware-node, middleware-deno, middleware-sunder), and I didn't want to go this deep.

BTW I did nothing "strange" here, I just made an interface from the typedef, wrote the used params/functions into it, and cross-checked with the original node types (so the headers could be string | string[] | undefined instead of just string and the URL and method could be undefined too). In theory this should not broke anything.

[wolfy1339]

This is needed for Node, so it exits even with a timeout running
octokit/octokit.js#2079

[tg44]

I read what unref is and why it is used, and I'm almost certain that we don't "need" that unref (we have a small timeout and also we clear in every paths). Also, v8 does not have unref, so cloudflare workers die BCS of it.

Probably we should use the below check instead of just adding/deleting it;

if (typeof timeout.unref === "function") {
  timeout.unref();
}

[gr2m]

Also, v8 does not have unref, so cloudflare workers die BCS of it

are you sure? Node is built on v8

[tg44]

I faced the "unref is not a function" error when I tried to run it on cloudflare workers so I'm sure. But a fixup commit is on the way.

[tg44]

I added this back with an if, and I left out the else from the branch coverage, to keep it 100%. I don't think it is worth the effort to mock the setInterval out.

[gr2m]

I don't think this will work. If I recall correctly, we made the APIs in a way to work independent of its environment. Node's standard request/response middleware is the most common, but we also want to support others such as Cloudflare Workers and Deno.

So I think this would need to be something abstract, like extraData, or similar.

The node middleware could then pass the extraData.request reference by default

[timrogers]

That would work, but it’s sad to lose the request type that makes this pattern a bit friendlier to work with.

Could we keep the current pattern, and then say that the originalRequest is not guaranteed to be provided? That way, you have the Node middleware case where the request is passed to the handler, whereas if you call receive directly, it’s missing.

[tg44]

The originalRequest already an originalRequest?: IncomingMessage so it is optional. You need to check if it is set or not, and you can possibly use the whole thing without it.

[gr2m]

I don't think the webhooks.* API is the right place for it. When using the node middleware then extraData.request could be typed correctly as IncomingMessage.

The Webhooks constructor would need a generic to which you can pass types for the .extraData, so even without using the node middleware, you'll be able to get all the types you want, without us needing to import the types for all users.

[tg44]

I think we are speaking about different things, so I will WOT down all of my gathered ideas.

• Problem 1; I need data from the request in the handlers.
• Problem 2; We want to handle multiple "http frameworks"

For problem 1;

• one option is to simply pass the whole request in "as-is", and we can read it in the hooks (current PR)
  • we can wrap this data, or type it better (like I did in this PR or like I will describe later)
• we define an extractor function, which will allow as to type the "worthy data", call this function in the middleware, and pass the "additional data" down to the hooks (my original idea, but it still needs a lot of generic types so from the two options I would choose the first one)

With problem 2, I started out a solution (which is probably not the best in retro, see later), which does nothing special just type the request and response as we currently use them (making a minimal needed subset). This is better than the previous type IncomingMessage = any but still not really good to call this IncomingMessage (bcs it is not necessarily an IM from node, and it is a bit misleading, but I did not introduce this misleading behavior to the code, it was already there).

After I slept on it I would solve this whole middleware problem with TypeClasses. Instead of extending the original implementation. We could generic-type out a middleware<I, O>(helper: MiddlewareIOHelper<I, O>, request: I, response: O, ...) and get a helper which looks something like this;

interface MiddlewareIOHelper<I, O> {
  getHeader(i: I, name: string): string;
  getBodyAsJson(i: I): Promise<unknown>;
  getUrlPath(i: I): string;
  getMethod(i: I): "GET" | "POST" | "DELETE" | "OPTION" | "PUT" | "UNKNOWN";
  
  respondWith(o: O, body?: string, status?: number, headers?: Record<string, string>): void;
}

From the interface above, we can easily implement an instance for <IncomingMessage, ServerResponse> or <Context, Context> or we could even type the <I, O> to a single type so less typeparams will be dragged in the code.

I think this PR is focusing on problem 1 solution 1. And bcs of the area, it touches problem 2 too.

I think the "solution" would be sth like;

• renaming the IncomingMessage bcs it is misleading
• merging this PR
• agreeing on the typeclass solution (I can draft out one), and handling it in an another PR

[timrogers]

My gut feeling is that we should probably simplify this proposal and go back to something similar to what you proposed originally: just allow the handler to accept an extraData object (which can be any object; no specific type!), and then have a convention that the built-in Node middleware passes a request key inside that extraData with the IncomingMessage.

What do you think?

[tg44]

Okay, I reverted all changes with IncomingMessage, and renamed the originalRequest to extraData.

[tg44]

@timrogers @gr2m ping, I think I fixed/reverted all the concerns.

[amitgilad3]

Hey @tg44 , im using fastify and was wondering if this pr could help me integrate with it??

[tg44]

@amitgilad3 this is the relevant WOT about what this PR does and what it doesn't. I think it would be nice to provide the described MiddlewareIOHelper and if we would implement that, probably fastify would be easy to add too. As you can see, we didn't merged this into, so I stopped working on this repo (bcs I already had the hack I needed for my app. I just wanted to improve the lib so others should not thinker with this when they want the same thing as I).

I think the current implementation of this lib uses the interfaces described here. If you can mock/wrap these functions to fastify specific, you can probably write a middleware yourself.