-
Notifications
You must be signed in to change notification settings - Fork 633
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
Comments
A small question about the branch code: why extend request? Could stuff just be attached to 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 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
I guess that would be ok? The whole That is definitely one solution to the problem :-) I think I personally would push for thinking about extending |
TypeScript forgives passing optional parameters to spread operations so this wouldn't need the |
Good point!
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 |
If I think my main concern with the existing Request definition is how subtle the syntax for 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 ( 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()
}
|
But within your middleware implementation, the type is not potentially undefined anymore if you define
I agree, but that is an implementation detail - we can "just" separate |
A minimal way to integrate some form of context and the other http server specific stuff into 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 Our signature would be reduced to |
We've been discussing this over Discord but to document some of my opinions/votes here:
This - to me - is a huge positive because I can tell at a glance from
This is most definitely an ideal signature. Making I think we're very much heading in the right direction toward an ideal implementation. |
@keithamus and I are in contact and working on a PR, stay tuned :-) |
As I stated back in #1283 (comment) I think this goes too far in the framework direction for 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 I feel this is something that should exist outside of |
I agree with @kitsonk's view - this is outside the Standard Library's scope. I'm -1 on this. |
WDYT, @kt3k? |
I'm in favor of including some kind of 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 |
Hmm, ok. It might be okay if it's unopinionated and complements the Web API and 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 |
@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.
@iuioiua I would say that is how the proposed solution in the original post works. |
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 JavaScriptThink about how many middleware formats nodejs has:
While this is annoying, each middleware system had a good reason to be created:
Now look at Deno:
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.
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 artHono 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 thoughtsIf 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 |
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. |
I created a very generic library for |
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
std/http
to be used for actual applications directly in the future. Once a pattern is established, there are already some modules instd
that could easily be wrapped into out-of-the-box middlewareServer
sHandler
signature. Composing middleware should always just return a new middleware, so that compositions can be modularized and passed around opaquelyPOC
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:Animal
and adding ananimal: Animal
property) It could be used like this (lots of abstracted functions in here to show the idea):A
composeMiddleware
function that takes twoMiddleware
s and returns a newMiddleware
that is a composition of both in the given order. The resultingMiddleware
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:
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 givenMiddleware
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: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 arbitrarystack().add().add().add().handler
chainMiddleware<Request>
is assignable tostd/http
Handler
- meaning there is no additional wrapping necessaryServer
, stating which properties are missingTo be fair, it makes some assumptions. It assumes that you always add the same type to your
next
call, so if you have conditionalnext
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.
Check out the branch, e.g. with
Start the server with
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:
Fail JSON content type:
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:
Request
below. The pattern above works either way, but I think we should take a look at that.On
Request
and API ergonomicWhile 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 separatelyimport
ed 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 toextend
Request
instd/http
to have one central API for everything concerning the incoming request - includingconnInfo
, 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 followingRequest
for everything it offers.The text was updated successfully, but these errors were encountered: