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

BREAKING: feat(http): Request API & Middleware #1555

Closed
wants to merge 100 commits into from

Conversation

LionC
Copy link
Contributor

@LionC LionC commented Nov 11, 2021

std/http Request API and Middleware

After delaying this again and again to polish it, here is a (evolved) review-ready version of POC in the linked issue #1295 .

Before I go into the ideas and changes here, I want to give some credit. Even though I wrote the code, I want to explicitly mention and thank:

Goals

  • Make the std/http API ergonomic and powerful enough to be a good starting point for actual applications at some point as well as offer smooth out of the box utilities for deno deploy HTTP cases
    • Similar to most http frameworks, there should be an ergonomic central req => res API offering all information needed regarding a specific request
    • Establish a minimal but complete middleware pattern that can be used to build out-of-the-box and third-party middleware in the future to cover very common use cases like CORS, compresison, deserialization, logging etc. Middleware should be...
      • ...just a function, there should be no magic and no additional IOC style runtime stuff, just a straight callstack
      • ...as composable and modularizable as possible
      • ...able to handle Errors with normal JS constructs
      • ...type(script) safe to use and combine, ensuring correct middleware context, order and termination. Middleware should not be less safe than combining normal functions

Changes

I try to summarize the changes in the PR below, but I would highly encourage reading the docs - they are an important part of the module and should be good at explaining the concepts.

Add Method enum to the Status module

Adds another enum with the HTTP methods which actually have a standardized representation, parallel to Status.

HttpRequest Delegate Wrapper

Adds an HttpRequest-class (got a better name? please say so!) that wraps a Request object, delegating to it (and thus implementing the Request API interface itself) and adding convenience methods and properties to have a unified API:

  • A connInfo property, holding the connInfo that was passed separately before
  • A lazy parsedUrl property to access a URL object for the request url (as a first start for convenience APIs)
  • A context object to hold (initially empty) request context with an .addContext() method to add onto it

Note that there should probably be a lot more convenience methods here - like a (CookieStore compatible?) lazy cookie reading API, maybe a way to access parsed content types lazily etc - I just wanted to create a space where that is possible and clean up the API without bloating the PR too much.

BREAKING: Handler Signature

The Handler signature has been reduced to just req: HttpRequest => Response.

This only breaks code that relies on the positional connInfo argument that was removed. Code that just depends on the request will continue to work, as HttpRequest implements Request.

This cleans up the API, creating a place to add more request information onto in HttpRequest instead of adding more parameters while also enabling the established middleware signature in the change below.

Middleware & chain()

Adds a chain() function that allows to chain (req: HttpRequest, next: Handler) => Response middleware together with very little actual runtime code in a type safe, order aware manner. Chains can be arbitrarily nested, understand when they are terminated and will generally stop you from running into runtime errors.

Terminated chains are actual Handlers and thus cann be passed to serve:

const handler = chain(parseMiddleware)
  .add(validateMiddleware)
  .add(dataHandler);

await serve(handler);

The actual runtime code of chain() is very small - types omitted, the whole middleware.ts file is just:

function composeMiddleware(first, second) {
  return (req, next) =>
    first(
      req
      (r) =>
        second(r, next),
    );
}

export function chain(middleware) {
  const copy = middleware.bind({})

  copy.add = (m) =>
    chain(
      composeMiddleware(
        middleware,
        m,
      ),
    );

  return copy;
}

But the type safety and resulting DX is made possible by typing code around the Middleware type, which is the TS way to write middleware, enforcing middleware authors to be type safe. The typing code mostly constraints what one can do to provide maximum safety and instant LSP feedback.

The typing code looks pretty big, which is mostly because TS does not have a good way to reuse type parameter sets (at least none that I know of).

README Docs

Adds a table of contents to the README, and a section about Middleware, while integrating the other changes and lightly touching up some of the other docs.

Removes docs about the legacy server.

Try it out

Under http/middleware/example_server.ts you find a server using the middleware examples from the docs.

@keithamus
Copy link

I've validated this pattern with a couple of closed-source projects that currently use the bare net/http server, and it works very well. I'm excited by this change.

@kt3k
Copy link
Member

kt3k commented Nov 18, 2021

@LionC
The chaining of types of necessary context (Needs) and additional context (Adds) looks great & useful 👍 I like the idea. But the signature of Middleware and chain utility looks a little magical to me. Isn't it possible to make Middleware something like (f: Handler) => Handler (with Needs and Adds type params)? And chaining those like middlewareA(middlewareB(middlewareC(handler))). I guess that might be more natural and memorable

@kt3k
Copy link
Member

kt3k commented Nov 19, 2021

I think the custom HttpRequest is a little too opinionated. It introduces learning barrier to the very first example of server in Deno..

Isn't a similar thing possible with Handler of type (req: Request, context: Context) => Response where Request is Web standard request?

@ry
Copy link
Member

ry commented Nov 19, 2021

I'm concerned this is incompatible with how middleware works in Oak (cc @kitsonk)

@kt3k
Copy link
Member

kt3k commented Nov 20, 2021

Oak's Middleware depends on oak specific concepts like Context, Application, etc. So I don't think it's reasonable to provide oak-compatible middleware in std/http. Rather I suggest we should provide an adapter utility which converts std/http-Middleware to oak-Middleware

@LionC
Copy link
Contributor Author

LionC commented Nov 22, 2021

Thanks for all the input. I will try to add my view:

@kt3k

The chaining of types of necessary context (Needs) and additional context (Adds) looks great & useful 👍 I like the idea. But the signature of Middleware and chain utility looks a little magical to me.

What do you mean exactly with magical regarding the Middleware type? It feels like a normal function type with parameterised inputs to me personally, similar to something like e.g. type Consumer<T> = (it: T) => void.

Regarding chain, that is actually intentional - while it would be possible to achieve the same thing without the chain helper by nesting as you suggested, I think that API would be pretty inconvenient to use. Imagine this:

const handler = 
  log(  
    gzip(
      cors("example.org",
        authenticate(
          authorize(
            validate(someSchema)
          )
        )
      )
    )
  )

that does not feel optimal to me.

Pretty much all middleware systems (Express, Koa / Oak, even ones in other languages) seem to use some form of API that reads like a sequence on one level, in the JS ones they all have .use() chains. The chain utility was meant to mirror that style while making clear that it is just a function again that can be arbitrarily nested (not too different from Express Applications).

I think the custom HttpRequest is a little too opinionated. It introduces learning barrier to the very first example of server in Deno.

I personally do not see the learning curve but maybe I am misunderstanding - it is still perfectly fine and works no problem to just write a (req: Request) => Response handler, that has not changed. The Request object you are getting just has more convenience things on top that you can use but you do not have to, Request works just fine.

Isn't a similar thing possible with Handler of type (req: Request, context: Context) => Response where Request is Web standard request?

We (I talked to @keithamus about this specific point a lot) thought about that - the problem is that adding another parameter every time we want to pass some new data to handlers that is not covered by the Request standard - which is a more generic standard not targeting our use case - will lead to pollute the signature and introduce more breaking changes in the future. It also makes the whole Api more complicated, as the place some functionality is located in becomes pretty random, think cookies, which would be in some imported functions, url, which would be in req directly and context, which would be another parameter - that is hard to explain to someone new.

I think it would be really valuable to have one central place to access all information and functionality associated with one request that can be safely extended and that is easily discoverable (also via LSP).

@ry

I'm concerned this is incompatible with how middleware works in Oak (cc @kitsonk)

I agree with @kt3k, from everything I have seen, I think oak could provide a function to convert this format into oak middleware.

@LionC
Copy link
Contributor Author

LionC commented Dec 9, 2021

Updated to main, will fix updated tests soon (again^^)

@bartlomieju
Copy link
Member

@LionC thank you for the PR, but I'm going to close it without a merge. This PR sparked conversation among the core team to support middlewares and we are still debating adding support for std/http; with roughly 50/50 split among the team. Current suggestion is to use similar middleware type as https://github.com/lucacasonato/fresh does.

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

Successfully merging this pull request may close these issues.

5 participants