Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concept Proposal: std/http/middleware #1295

Open
LionC opened this issue Sep 20, 2021 · 18 comments
Open

Concept Proposal: std/http/middleware #1295

LionC opened this issue Sep 20, 2021 · 18 comments
Labels
suggestion a suggestion yet to be agreed

Comments

@LionC
Copy link
Contributor

LionC commented Sep 20, 2021

There have been some discussions here and there (e.g. #1283 ) about middleware in std/http. I asked people for some days to post a concept idea and here it is :-)

std/http/middleware

Goals

  • Establish a middleware concept that enables std/http to be used for actual applications directly in the future. Once a pattern is established, there are already some modules in std that could easily be wrapped into out-of-the-box middleware
  • Allow middleware and composed middleware stacks to just be a function that takes some form of request and returns a response, optionally calling the next middleware. This ensures that we deal with normal call stacks, allows errors to bubble up as expected and reduces the amount of black magic happening at runtime
  • Be completely type safe, including middleware order and arbitrary middleware composition. This means I want the type checker to stop me from registering a handler on the server that assumes that certain information is available from previous middlewares (e.g. auth info, parsed and validated bodies...), even though that middleware is not present in that handler's chain. Just having a global state that can be typed and is assumed to always be present in every function is not good enough - we are dealing with a chain of functions here, we should leverage Typescript and make sure that that chain actually works type-wise.
  • The middleware signature should be compatible to the Servers Handler signature. Composing middleware should always just return a new middleware, so that compositions can be modularized and passed around opaquely

POC

Here is a branch in which I have built a small dirty POC fullfiling the goals above. This is just to show the idea. It is not fleshed out, very rough around a lot of edges, has subpar ergonomics and several straight up bugs. All of them are solvable in several ways and their solution is not vital to the concept, so I left them as they are for the sake of starting a conversation.

I stopped writing as soon as I was sure enough that this can be done reasonably. There are many ways to do this basic concept and a lot of them are viable - I did not want to invest into one of them, just have something to start talking.

API

The POC contains three components. Their actual runtime code is really small - most of the code around it (and most todos to fix the bugs / ergonomics issues) is just types.

The components are:

  • A Middleware function type with two important generic type parameters:

    • What type the middleware expects (e.g. it needs a semantic auth field on top of the normal request)
    • Optionally, what information the middleware adds to the request "context" (e.g. validating the body to be a valid Animal and adding an animal: Animal property) It could be used like this (lots of abstracted functions in here to show the idea):
    const validateFoo: Middleware<Request, { foo: Foo }> = async (req, con, next) => {
      const body = extractBody(req);
    
      if (!isFoo(body)) {
        return new Response(
          "Invalid Foo",
          { status: 422 },
        );
      }
    
      const nextReq = extend(req, { foo: body });
    
      return await next!(nextReq, con);
    };
  • A composeMiddleware function that takes two Middlewares and returns a new Middleware that is a composition of both in the given order. The resulting Middleware adds a union of what both arguments add and requires a union of what both arguments require, except the intersection between what the first one adds and the second one requires, as that has already been satisfied within the composition.

    It could be used like that:

    declare const authenticate: Middleware<Request, { auth: AuthInfo }>;
    declare const authorize: Middleware<Request & { auth: AuthInfo }>;
    
    const checkAccess = composeMiddleware(authenticate, authorize);
    
    assertType<Middleware<Request, { auth: AuthInfo }>>(checkAccess);

    composeMiddleware is the atomic composition and type checking step but not very ergonomic to use, as it can only handle two middlewares being combined.

  • A stack helper that wraps a given Middleware in an object thas has a chainable .add() method. This allows for nicer usage and follows the usual .use() idea in spirit. It can be used like this:

    declare const authenticate: Middleware<Request, { auth: AuthInfo }>;
    declare const authorize: Middleware<Request & { auth: AuthInfo }>;
    declare const validateFoo: Middleware<Request, { foo: Foo }>;
    
    const authAndValidate = stack(authenticate)
      .add(authorize)
      .add(validateFoo)
      .handler;
    
    assertType<Middleware<Request, { auth: AuthInfo; animal: Animal }>>(
      authAndValidate,
    );

    This essentially just wraps composeMiddleware to be chainable with correct typing.

    Notice the .handler at the end - this extracts the actual function again. There might be nicer ways to do it, but the concept works for the sake of discussion.

The components above fulfill the goals mentioned above:

  • Middleware is just a function, including the result of an arbitrary stack().add().add().add().handler chain
  • Middleware<Request> is assignable to std/http Handler - meaning there is no additional wrapping necessary
  • Middleware composition is completely type safe and order-aware. This means that all requirements that are present but not fulfilled by previous middleware "bubble up" and will type error when trying to register it on the Server, stating which properties are missing

To be fair, it makes some assumptions. It assumes that you always add the same type to your next call, so if you have conditional next calls with different types, you need to "flatten" the types. It also assumes that you do not throw away the previous request context. However, I think those are reasonable assumptions and they are also present (and a lot less safe) in other current TS middleware concepts e.g. in koa / oak.

Play around with it

To run a small server with some middleware from the POC branch, follow the steps below. The implemented middleware is just for presentation purposes, it's implementation is very bad, but it works to show the idea.

  1. Check out the branch, e.g. with

    git remote add lionc git@github.com:LionC/deno_std.git
    git fetch lionc
    git switch middleware-experiment
  2. Start the server with

    deno run --allow-net http/middleware/poc/server.ts

Now you can throw some requests at it, here are some httpie example commands:

  • Succeed

    http --json 0.0.0.0:5000/ name=My entryFee:=10 animals:='[{"name": "Kim", "kind": "Tiger"}, {"name": "Flippo", "kind": "Hippo"}, {"name": "Jasmin", "kind": "Tiger"}]'
  • Fail validation:

    $ http --json 0.0.0.0:5000/ name=My entryFee:=10
  • Fail JSON content type:

    $ http --form 0.0.0.0:5000/ name=My entryFee:=10

http/middleware/poc/server.ts is also a good place to play around with the type safe composition - try changing the order of middleware, leave a vital one out and see how LSP / tsc react.

What now?

There are two questions to answer here:

  • What have I missed? Is this something we want to go deeper on? I did not want to invest more time into figuring out all the details before there is some input on the core idea
  • How do we want the API for application and middleware authors to look like? See my take on Request below. The pattern above works either way, but I think we should take a look at that.

On Request and API ergonomic

While working on this and trying to write some middlewares, I really felt that the current Handler signature is quite...weird. I get why it looks that way, but from an API perspective, it does not make a lot of sense that two arbitrary fields about the incoming request are separated into their own argument. It also does not make a lot of sense that some arbitrary functionality that would be expected on the request parameter needs to be separately imported as a function and called on that object. There is also not really a nice way to add new things to a request in a type safe way.

Following Request makes a lot of sense, it being a Web standard and all. But I think it could make sense to extend Request in std/http to have one central API for everything concerning the incoming request - including connInfo, a simple helper to add to some kind of request context, helpers to get common info like parsed content types, get cookies etc while still following Request for everything it offers.

@keithamus
Copy link

keithamus commented Sep 20, 2021

A small question about the branch code: why extend request? Could stuff just be attached to con without using Request & everywhere? Coming from a principle where Request is immutable seems sensible to me because it can be used as in a WeakMap/Set for use cases outside of con.

export type Middleware<
  Needs = {}
  Adds = {},
> = (
  req: Request,
  con: Needs,
  next?: Middleware<Needs &Adds>,
) => Promise<Response>;

Edit: oh I see I've made a faulty assumption that con is the context/state when actually it is connection info...

Perhaps my point stands though: making Request immutable and perhaps having a context/state object passed to each middleware which is where "stuff" can be stored?

export type Middleware<
  Needs = {}
  Adds = {},
> = (
  req: Request,
  con: ConnInfo,
  state: Needs,
  next?: Middleware<Needs &Adds>,
) => Promise<Response>;

@LionC
Copy link
Contributor Author

LionC commented Sep 20, 2021

Perhaps my point stands though: making Request immutable and perhaps having a context/state object passed to each middleware which is where "stuff" can be stored?

export type Middleware<
  Needs = {}
  Adds = {},
> = (
  req: Request,
  con: ConnInfo,
  state: Needs,
  next?: Middleware<Needs &Adds>,
) => Promise<Response>;

I thought about that as well - there are several places we could put some context object. The problem with this approach is that as the "root" call is coming from Server directly, the context (or whatever name) object parameter would have to be optional. Which would mean that

  1. We would have 4 parameters for middleware with two of them being optional :-S

    export type Middleware<Requires = {}, Adds = {}> = (
      req: Request,
      con: ConnInfo,
      ctx?: Requires,
      next?: Middleware<Requires & Adds>,
    ) => Promise<Response>;
  2. Everytime middleware wants to add onto the context, it would need to do something like

    const newContext = {
      ...ctx ?? {}, // Even though obvious to some, this might be counterintuitive for others
      someNewProp: foo,
    }

I guess that would be ok? The whole (req, con, ctx, next) call just seems really clunky considering that we are mostly reinventing a proven wheel regarding the signature.

That is definitely one solution to the problem :-) I think I personally would push for thinking about extending Request with some class that wraps an actual immutable Request object and that offers some helpers (not just middleware related ones).

@keithamus
Copy link

2. Everytime middleware wants to add onto the context, it would need to do something like

const newContext = {
  ...ctx ?? {}, // Even though obvious to some, this might be counterintuitive for others
  someNewProp: foo,
}

TypeScript forgives passing optional parameters to spread operations so this wouldn't need the ?? {}. But it does mean ctx. would be referred to as ctx?. everywhere, which is disappointing.

@LionC
Copy link
Contributor Author

LionC commented Sep 21, 2021

  1. Everytime middleware wants to add onto the context, it would need to do something like
const newContext = {
  ...ctx ?? {}, // Even though obvious to some, this might be counterintuitive for others
  someNewProp: foo,
}

TypeScript forgives passing optional parameters to spread operations so this wouldn't need the ?? {}.

Good point!

But it does mean ctx. would be referred to as ctx?. everywhere, which is disappointing.

I don't think it would - if you type your middleware so it expects a certain context, that will apply to your argument type within the function, which means access is safe.

I would be happy to push this forward and finish a mergeable implementation - but I really think we should consider changing the Handler signature beforehand. If we do this now, we will forever be stuck with (req, con, ctx, next) => Response.

@keithamus
Copy link

I don't think it would - if you type your middleware so it expects a certain context, that will apply to your argument type within the function, which means access is safe.

If ctx is potentially undefined then any access to it will need to be guarded with either if (!ctx) return next(req, con, ctx) or you'd need ctx?..


I think my main concern with the existing Request definition is how subtle the syntax for needs vs provides is:

declare const a: Middleware<Request , { auth: AuthInfo }>;
declare const b: Middleware<Request & { auth: AuthInfo }>;

These do very different things but they're only 1 character different (& vs ,) which is easy to miss. I wonder if giving Request an optional mode of Request<T> where T is the given context would help?

interface Request<Ctx = {}> {
    ctx: Record<string|number|symbol, unknown> & Ctx
    withCtx: <NewCtx = {}>(newCtx: NewCtx) => Request<Ctx & NewCtx>
}

type Middleware<Needs = {}, Adds = {}> = (
  req: Request<Needs>,
  con: Request<Adds>,
  next?: Middleware<Needs & Adds>,
) => Response | Promise<Response>;


const foo: Middleware<{'a': number}, {'b': number}> = (req, con, next) => {
    const newReq = req.withCtx({b: req.ctx.a + 1})
    if (next) return next(newReq, con)
    return new Response()
}

withCtx could simply mutate ctx, keeping the same Request instance but refining the type.

@LionC
Copy link
Contributor Author

LionC commented Sep 21, 2021

I don't think it would - if you type your middleware so it expects a certain context, that will apply to your argument type within the function, which means access is safe.

If ctx is potentially undefined then any access to it will need to be guarded with either if (!ctx) return next(req, con, ctx) or you'd need ctx?..

But within your middleware implementation, the type is not potentially undefined anymore if you define Needs, see this reduced example . The rest of the type magic will make sure that a chain containing that middleware being registered as a Handler is actually safe, order aware and everything.

I don't think it would - if you type your middleware so it expects a certain context, that will apply to your argument type within the function, which means access is safe.

If ctx is potentially undefined then any access to it will need to be guarded with either if (!ctx) return next(req, con, ctx) or you'd need ctx?..

I think my main concern with the existing Request definition is how subtle the syntax for needs vs provides is:

declare const a: Middleware<Request , { auth: AuthInfo }>;
declare const b: Middleware<Request & { auth: AuthInfo }>;

These do very different things but they're only 1 character different (& vs ,) which is easy to miss. I wonder if giving Request an optional mode of Request<T> where T is the given context would help?

I agree, but that is an implementation detail - we can "just" separate Needs from Request altogether in the typing and attach it to whatever we want in the signature, so that it is Middleware<{}, {auth: string}> vs Middleware<{auth: string}>, which should be clear enough.

@LionC
Copy link
Contributor Author

LionC commented Sep 21, 2021

A minimal way to integrate some form of context and the other http server specific stuff into Request might be something like:

export class HttpRequest<C extends {} = {}> extends Request {
    constructor(
        readonly connInfo: ConnInfo,
        protected context: C,
        ...args: ConstructorParameters<typeof Request>,
    ) {
        super(...args)
    }

    addContext<N extends {}>(contextToAdd: N): HttpRequest<C & N> {
        this.context = { ...this.context, ...contextToAdd }

        //@ts-ignore Limitations of mutation and types, but we should mutate for performance
        return this as HttpRequest<C & N>
    }
}

which could then be used in a potential middleware signature like this:

const authorize: Middleware<{ auth: string }, { user: string }> = async (req, next) => {
  const { auth } = req.ctx
  const user = getUserForToken(auth)

  return await next!(
    req.addContext({ user })
  )  
}

I do not see any harm in mutating ctx, as TS will only expose the relevant properties in any subchain. All the Request fields stay untouched, we can integrate things like cookies and other potential helpers and still be Request compatible.

Our signature would be reduced to (req: HttpRequest) => Response, with middleware being (req, next) => Response

@keithamus
Copy link

We've been discussing this over Discord but to document some of my opinions/votes here:

Middleware<{}, {auth: string}> vs Middleware<{auth: string}>

This - to me - is a huge positive because I can tell at a glance from <{}, {auth: string}> that this middleware does not need additional context to derive an auth. The latter example is less clear, IMO, especially for newcomers. Explicit over implicit.

Our signature would be reduced to (req: HttpRequest) => Response, with middleware being (req, next) => Response

This is most definitely an ideal signature. Making ConnInfo a param puts it's importance far too high for something stores a couple of strings I'm unlikely to touch.

I think we're very much heading in the right direction toward an ideal implementation.

@LionC
Copy link
Contributor Author

LionC commented Sep 22, 2021

@keithamus and I are in contact and working on a PR, stay tuned :-)

@bartlomieju bartlomieju added the suggestion a suggestion yet to be agreed label Nov 10, 2021
@kitsonk
Copy link
Contributor

kitsonk commented Nov 19, 2021

As I stated back in #1283 (comment) I think this goes too far in the framework direction for std.

The middleware type would be really hard to get wide consensus on. It fundamentally wouldn't work for oak, so I would consider using this out of std.

I feel this is something that should exist outside of std.

@iuioiua
Copy link
Collaborator

iuioiua commented Oct 4, 2023

I agree with @kitsonk's view - this is outside the Standard Library's scope. I'm -1 on this.

@iuioiua
Copy link
Collaborator

iuioiua commented Oct 23, 2023

WDYT, @kt3k?

@kt3k
Copy link
Member

kt3k commented Oct 24, 2023

I'm in favor of including some kind of Middleware type in the way we can use them with Deno.serve and also in any frameworks such as fresh, oak, hono, aleph, etc.

I think we can borrow some basic ideas from golang's middleware convention

The problem in the previous attempt (#1555) by @LionC was introduction of non-standard HttpRequest type in my view. We probably would like to avoid extending web standard types.

@iuioiua
Copy link
Collaborator

iuioiua commented Oct 24, 2023

Hmm, ok. It might be okay if it's unopinionated and complements the Web API and Deno.serve(). How about if it looked something like this:

type Middleware = (
  request: Request,
  next: () => Response | Promise<Response>,
) => Response | Promise<Response>;

function createHandler(
  ...middlewares: Middleware[]
): (request: Request) => Response | Promise<Response> {
  // ...
}

Deno.serve(createHandler(middleware1(), middleware2()))

Note: I've omitted info: Deno.ServeHandlerInfo as one of the middleware arguments, for brevity, but I think it should be included in the implementation.

@LionC
Copy link
Contributor Author

LionC commented Oct 24, 2023

The problem in the previous attempt (#1555) by @LionC was introduction of non-standard HttpRequest type in my view. We probably would like to avoid extending web standard types.

@kt3k There are ways to do a similar approach without that (e.g. by just having more arguments) - I tried to outline the tradeoffs in the last part of the original post. I just think it is a challenge to make the middleware signature ergonomic without extending Request, but it is definitely possible.

Hmm, ok. It might be okay if it's unopinionated and complements the Web API and Deno.serve().

@iuioiua I would say that is how the proposed solution in the original post works. Deno.serve does not know about middleware. std would just exposes a helper to build a standard Deno handler via a middleware pattern, but with a Typescript-first safety approach (compared to other middleware systems). Check out the middleware folder in the linked branch for examples and check the description of the linked PR #1555 for more details.

This was referenced Nov 9, 2023
@syhol
Copy link
Contributor

syhol commented Dec 14, 2023

I'm a +1 on this from a philosophical standpoint. The following comment just talks about the justification for middleware in deno_std and nothing about its design.

The state of http middleware in JavaScript

Think about how many middleware formats nodejs has:

  • express middleware
  • koa middleware
  • fastify plugins
  • hono middleware
  • more...?

While this is annoying, each middleware system had a good reason to be created:

  • express middleware was the first I'm aware of, a lot of frameworks created afterwards simply reused this format
  • koa middleware introduced stack dynamics, making it easy to process responses
  • fastify plugins I assumed tried to stay away from middleware for performance reasons
  • hono middleware introduced Web Request/Response to Node.js
  • elysiajs middleware (bun not node) introduced type safe middleware

Now look at Deno:

  • Oak has its own middleware format
  • Aleph.js has its own middleware format
  • Ultra uses hono and hono's middleware
  • Lume has its own middleware format
  • Fresh has its own middleware format

It looks to me that they were mostly created only because there was no alternative, with the exception of Oak there is no innovation here.

Whats the problem?

This is a real problem for middleware authors. If I wanted to publish a middleware module for the JavaScript ecosystem, I would need to consider a dozen formats.

Just look at the Deno module https://deno.land/x/cors@v1.2.2, it exposes 5 different middleware formats.

It fundamentally wouldn't work for oak

Even if Oak didn't adopt a new middleware system, it would be fairly trivial to create an adapter considering both the proposal and Oak are based on the req/res WebAPIs and the next/stack pattern. This means middleware authors only need to target one format and Oak can still leverage this new middleware ecosystem, whether Oak provides this adapter or its provided by a 3rd party.

Previous art

Hono had a bash at creating a standard middleware but ultimately found it out of scope. It also references PHP's PSR-15 which seems to be quite successful. However the scope of deno_std is providing for the Deno ecosystem, so I feel that creating a standard middleware interface for Deno is very much in scope.

Final thoughts

If there was a solid alternative, I would recommend the Deno frameworks adopt that instead but it doesn't seem to exist today.

As for if it exists in deno_std or elsewhere, I think deno_std can help give it the extra credibility and visibility it needs to get adoption. I think it can become a core part of the Deno HTTP landscape.

The best time to plant a tree deno_std middleware was 20 5 years ago. The second best time is now.

@iuioiua
Copy link
Collaborator

iuioiua commented Dec 15, 2023

Initially, I was against this idea. Now, I think it could be a nice solution for those who don't always want to use a web framework.

@timonson
Copy link
Contributor

I created a very generic library for Deno.serve handlers here: https://github.com/Zaubrik/composium .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion a suggestion yet to be agreed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants