Proposal: New middlewares loading API

[MichalLytek]

Ref. https://github.com/pleerock/routing-controllers/issues/131#issuecomment-301256671

Introduction

Right now, RoutingControllersOptions has following signature:

/**
 * List of middlewares to register in the framework or directories from where to import all your middlewares.
 */
middlewares?: Function[]|string[];

We allow to load middlewares in two ways:

// first
createExpressServer({
    middlewares: [__dirname + "/controllers/**/*.js"],
});

// second
createExpressServer({
    middlewares: [JwtMiddleware, AuthMiddleware, LoggingMiddleware, ErrorMiddleware],
});

I would like to introduce the new API:

createExpressServer({
    middlewares: {
        before: [
            AuthorizationMiddleware,
        ],
        after: [
            LoggingMiddleware,
        ],
        error: [
            CustomErrorMiddleware,
        ]
    }
});

Combining together with #255 users can get rid of the @Middleware decorator in classes (with problematic priority option) and define the order in array or in index file.

However in this case, proposal from #255 might need some boilerplate depending on used convention:

• two/three index files and

  import * as beforeMiddlewares from "./middlewares/before";
  import * as afterMiddlewares from "./middlewares/after";

  • configuration-like index file

    
    import { One } from "./one-middleware"
    import { Two } from "./two-middleware"
    import { Three } from "./three-middleware"

export default { before: [ One, Two, ], after: [ Two, Three, ], }

```ts
import middlewaresConfig from "./middlewares";
createExpressServer({
    middlewares: middlewaresConfig
});

We can still support old version for glob string or force to use naming convention:

createExpressServer({
    middlewares: {
        before: [path.join(__dirname, "../middlewares/before/*.js")],
        after: [path.join(__dirname, "../middlewares/after/*.js")],
        error: [path.join(__dirname, "../middlewares/error/*.js")],
    }
});
// or
createExpressServer({
    middlewares: {
        before: [path.join(__dirname, "../middlewares/*.before.js")],
        after: [path.join(__dirname, "../middlewares/*.after.js")],
        error: [path.join(__dirname, "../middlewares/*.error.js")],
    }
});

But I would stick to all-in-one glob string as we still need @Middleware({ priority: number }) decorator:

createExpressServer({
    middlewares: [path.join(__dirname, "../middlewares/*.js")],
});

Main proposal

interface MiddlewaresConfig {
    before?: ExpressMiddlewareInterface[];
    after?: ExpressMiddlewareInterface[];
    error?: ExpressErrorMiddlewareInterface[];
}

interface RoutingControllersOptions {
    middlewares?: string[] | MiddlewaresConfig;
}

Adding @NoNameProvided @pleerock for discussion.

[marshall007]

@19majkel94 related to your suggested tweaks to my proposal in https://github.com/pleerock/routing-controllers/issues/144#issuecomment-321973571, I think we could go much farther in simplifying things by ditching entirely the concept of "middleware" as it is today.

You hinted at implementing error handlers as interceptors rather than middleware. I think we can go a step further to unify these concepts and expose everything as interceptors. I would propose exposing the following interfaces:

interface Interceptor {
  intercept(action: Action, content: any): any;
}

interface ErrorInterceptor {
  error(action: Action, error: Error): any;
}

interface BeforeInterceptor {
  before(action: Action): any;
}

interface AfterInterceptor {
  after(action: Action, content: any): any;
}

So, instead of configuring the middleware lifecycle externally, it is defined by the implementation. This would allow you to define a single Interceptor that manages the entire request lifecycle or whichever portions it cares about. For example:

class RequestTimer implements BeforeInterceptor, AfterInterceptor {
  private start: Date;

  before(action: Action, content: any) {
    this.start = new Date();
  }

  after(action: Action, content: any) {
    action.request.start_time = this.start.toString();
    action.request.took = new Date() - this.start;
  }
}

Note: in the above example, it is assumed that interceptors would be instantiated per-request. This is not strictly necessary for the proposal, but I think it would make a lot of sense under this paradigm.

Lifecycle

1. before hooks called on all registered interceptors
2. controller method executed
3. intercept hooks called on all registered interceptors
4. after hooks called on all registered interceptors

All interceptor methods are allowed to either return a value, return/throw an error object, or return undefined. When a return value is present in the former two cases, it is used as the response. When the return value is undefined, we preserve the existing content for the response.

The error hook is called when any interceptor or controller method returns or throws an error. The ErrorInterceptor would be a special case in that, unlike other interceptors, if you return a defined value from the error() method, we respond with the error immediately instead of continuing down the chain.

Loading API

Finally, we get to keep the simplified the configuration API. We no longer have a need for both middleware and interceptors, we don't need any @Middleware decorators, nor do we have to configure different middleware types individually.

createExpressServer({
  interceptors: Interceptor[],
});

[NoNameProvided]

We can still support old version for glob string or force to use naming convention

I am the only one who dont like the glob string approach? Typos are not highlighted as in import/export statements, and they have no advantage over the proposed changes. We can just drop the support arent we. Or at least deprecate them so we can remove them in the future.

@marshall007 When the body is parsed in the lifecycle? Before the before hook or after?

Note: in the above example, it is assumed that interceptors would be instantiated per-request. This is not strictly necessary for the proposal, but I think it would make a lot of sense under this paradigm.

They dont have to and they shouldn't. It's NodeJS nature that you don't create handlers per-request they are created once. They can be stateless or stateful, but they should be initialized once. For your example you can just simply attach your date to the action.request in the before function and then read it from there.

[NoNameProvided]

@19majkel94 related to your suggested tweaks to my proposal in #144 (comment)

The proposed change by you is different in #144 and I like that a way more better as it's give better flexibility for error handling. But I like the other parts.

Also one idea to discuss with interceptors, as they will be called by routing-controllers how should one signals that it wants to end the processing of request? (Like when you dont call next in a middleware.)

[MichalLytek]

They dont have to and they shouldn't. It's NodeJS nature that you don't create handlers per-request they are created once. They can be stateless or stateful, but they should be initialized once. For your example you can just simply attach your date to the action.request in the before function and then read it from there.

100% agree as we've already discussed it here 😉

@marshall007 You can do it like this:

class RequestTimer implements BeforeInterceptor, AfterInterceptor {

    static readonly NS_PER_MS = 1e3;
    static readonly MS_PER_S = 1e6;

    before(action: Action, content: any) {
        action.request.startTime = process.hrtime();
    }

    after(action: Action, content: any) {
        const deltaTime = process.hrtime(action.request.startTime);
        action.request.took = this.toMiliseconds(deltaTime);
    }

    private toMiliseconds([seconds, nanoseconds]: [number, number]) {
        return (seconds * RequestTimer.MS_PER_S) + (nanoseconds * RequestTimer.NS_PER_MS);
    }
}

[MichalLytek]

I am the only one who dont like the glob string approach? Typos are not highlighted as in import/export statements, and they have no advantage over the proposed changes. We can just drop the support arent we. Or at least deprecate them so we can remove them in the future.

See https://github.com/pleerock/routing-controllers/issues/255#issue-250692118 😜

Also, I think that importing all from directories by glob string is an antipattern and this proposal reduce a lot of boilerplate of explicit array option. If users get used to this feature I would even deprecate loading from directories feature.

I really don't like glob string loading as you have no control and even if you remove controller/middleware/entity, it still load in runtime because compiled JS file still exist - you have to clean compile folder on every recompilation :no_mouth:

But I understand that migration of pleerock apps with 200 controllers, middlewares, etc. might be a waste of time making an index file with exports. So we should deprecate and remove it later if users won't protest.

[marshall007]

@NoNameProvided to answer your questions:

@marshall007 When the body is parsed in the lifecycle? Before the before hook or after?

Do you mean the request body? I was imagining we would implement body parsing as a global default BeforeInterceptor (the implementation of which could be overridden). So the answer to your question is (optionally) during before.

It's NodeJS nature that you don't create handlers per-request they are created once. They can be stateless or stateful, but they should be initialized once.

I don't have a terribly strong opinion here, I just thought instantiating interceptors per-request better aligned with expected behavior. Interceptors are not strictly middleware, they are abstractions, so I don't necessarily think the conventional behavior applies.

The proposed change by you is different in #144 and I like that a way more better as it's give better flexibility for error handling. But I like the other parts.

Agreed. I was just showing the basics for ErrorInterceptor as it relates to interceptors directly, but I agree we should flesh it out more with helper methods as discussed in #144.

... how should one signals that it wants to end the processing of request? (Like when you dont call next in a middleware.)

You can signal intent by the return value:

• undefined: "I'm not handling the response" - equivalent to next()
• Promise<any> | any: "I'm handling the response" - varies depending on Interceptor type, but mostly equivalent to storing the content on res.content, calling next() and having something later down the chain use res.content for the response
• Promise<Error> | Error | throw Error - equivalent to next(err)

I didn't really see a need for perfectly emulating next() under the Interceptors paradigm:

• BeforeInterceptor - handle validation, auth, etc and throw errors to signal that they handled the request
• Interceptor - handle response validation/transformation, so we may consider interrupting the chain as soon as one of them returns a value
• AfterInterceptor - handle resource cleanup, request timings, etc and generally have no need to signal handling the request
• ErrorInterceptor - handles formatting the error response, signals handling the request by returning a value

[marshall007]

@NoNameProvided to better answer your last question, I think we should strive to make Interceptors eliminate the need for a next() callback just like controller methods do.

[MichalLytek]

Finally, we get to keep the simplified the configuration API. We no longer have a need for both middleware and interceptors, we don't need any @Middleware decorators, nor do we have to configure different middleware types individually.

createExpressServer({
interceptors: Interceptor[],
});

I don't like it. Interceptors idea as middleware 2.0 is good but only for custom stuffs. With this you force people to wrap classic express middlewares into interceptors classes or again do awful configuration:

const app = express();

app.use(jwt(jwtOptions))
app.use(serveStatic({ path: "/public" }))
app.use(bodyParser.urlencoded(false))

useExpressServer(app, {
    interceptors,
});

api.use(morgan(process.env.LOG_FORMAT || 'common'))
api.use(compression())

Instead of:

createExpressServer(app, {
    middlewares: {
        before: [
            serveStatic({ path: "/public" }),
            jwt(jwtOptions),
            bodyParser.urlencoded(false),
        ],
        after: [
            morgan(process.env.LOG_FORMAT || 'common'),
            compression(),
        ],
    }
    interceptors,
});

So I would like to reword my first propsal:

import { RequestHandler as ExpressMiddlewareFunction } from "express";
// (req: Request, res: Response, next: NextFunction): any;

interface MiddlewaresConfig {
    before?: Array<ExpressMiddlewareInterface | ExpressMiddlewareFunction>;
    after?: Array<ExpressMiddlewareInterface | ExpressMiddlewareFunction>;
    error?: ExpressErrorMiddlewareInterface[];
}

interface RoutingControllersOptions {
    middlewares?: string[] | MiddlewaresConfig;
}

So we could direct register classic global middleware in configuration and also support functional, not class-based middlewares:

export const customMiddleware: ExpressMiddlewareFunction = (req, res, next) => {
    console.log("do something...");
    next();
}

As with direct loading we don't have to decorate the class, we can use pure function when DI for middleware is not needed.

[MichalLytek]

interface Interceptor {
  intercept(action: Action, content: any): any;
}

interface ErrorInterceptor {
  error(action: Action, error: Error): any;
}

interface BeforeInterceptor {
  before(action: Action): any;
}

interface AfterInterceptor {
  after(action: Action, content: any): any;
}

Great, but what about priority? RequestTimer should be called first before and last after. How do your API is able to declare this?

As we agreed that they should be stateless and singletons, sot there's no advantages of multi-hooks signatures, only priority problem. So I'm afraid I have to 👎 and @pleerock might be of the same mind.

[marshall007]

With this you force people to wrap classic express middlewares into interceptors classes or again do awful configuration: ...

@19majkel94 I don't see the "awful configuration" example you mentioned as necessarily bad. I think configuring classic express middleware on the Express object is the preferred approach (that's my preference in our projects, at least). That said, if there are strong objections to managing classic middleware this way, I think we could expose a static method Interceptor.fromMiddleware(func: ExpressMiddlewareFunction) (or something) for creating Interceptors from classic middleware which could then be loaded.

Great, but what about priority? RequestTimer should be called first before and last after. How do your API is able to declare this?

@19majkel94 I was imagining this would take advantage of your proposal in #255. That is, priority would be determined based on import order. I think that coupled with discrete before/after interceptors would cover our needs while limiting API complexity.

[MichalLytek]

That is, priority would be determined based on import order.

Ok, so please tell me how the import order will make this example work:

Interceptor 1:

• before (lowest prio, just parse body)
• after (highest prio, transform action result value)

Interceptor 2:

• before (highest prio, log request incoming time)
• after (lowest prio, log request handling time)

So the order should be [2, 1]? But then I1 after would be called last, after the time logging. So maybe [1, 2]? No, now the body is parsed before time logging.

So with multipart interceptors we can't determine the priority by the order in an array/object.

That said, if there are strong objections to managing classic middleware this way, I think we could expose a static method Interceptor.fromMiddleware(func: ExpressMiddlewareFunction) (or something) for creating Interceptors from classic middleware which could then be loaded.

Again, as interceptors suppose to be stateless (singletons), there is no advantages of multipart interceptors. It only introduce more complications like Interceptor.fromMiddleware and the priority problem.

[marshall007]

@19majkel94 I think you'd want Interceptor 1 to implement before and intercept. Then your order would be [2, 1].

[marshall007]

We could disallow modifying the response in AfterInterceptor to make it clear that formatting is supposed to be handled in the intercept method.

[MichalLytek]

I think you'd want Interceptor 1 to implement before and intercept. Then your order would be [2, 1].

... That was just an example! So without descriptions:

Interceptor 1:

• before (lowest prio) // for some reason, business logic needs it
• after (highest prio) // for some reason, business logic needs it

Interceptor 2:

• before (highest prio) // for some reason, business logic needs it
• after (lowest prio) // for some reason, business logic needs it

So the order is [1, 2] or [2, 1]? Do you see the edge case now? 😉

don't see the "awful configuration" example you mentioned as necessarily bad.

It looks bad and decouple low and api from routing-controllers. And your framework-registered before middlewares will be called after the global, app.use() ones.

[marshall007]

As we agreed that they should be stateless and singletons, sot there's no advantages of multi-hooks signatures, only priority problem. So I'm afraid I have to 👎 and @pleerock might be of the same mind.

@19majkel94 are you saying you are not in favor of the entire proposal aimed at unifying the "interceptor" and "middleware" concepts or just that we still need a solution to the priority problem?

---

I'd be interested in hearing feedback from others in regards to priority and whether or not we'd need to be more explicit about that. Your examples are somewhat contrived in that you are trying to encapsulate multiple interceptor hooks, each requiring a discrete priority, but I don't think this is a common case.

You don't have to encapsulate multiple hooks in a single Interceptor, that's strictly for convenience when it makes sense. If the before and after hooks actually require some shared business logic you still have the option of encapsulating that in a base class and exposing separate before/after interceptor classes (to control priority via import ordering).

[MichalLytek]

requiring a discrete priority, but I don't think this is a common case.

But it CAN happens. You have to consider all rare edge cases, not the one optimistic scenario.

If the before and after hooks actually require some shared business logic you still have the option of encapsulating that in a base class and exposing separate before/after interceptor classes (to control priority via import ordering).

So we only introduce new concepts and more complications that don't solve any problem. I don't see any points of your proposals. The only benefit of before & after in one class was the state (storying data in this) but since we abandon this idea, there's no benefits but only problems and more complications.

[NoNameProvided]

Have been some time I had the chance to review this. So here are my notes below regarding the current state of this.

I don't see the "awful configuration" example you mentioned as necessarily bad. I think configuring classic express middleware on the Express object is the preferred approach (that's my preference in our projects, at least).

@marshall007 I consider mixing "routing-controller" style and "express style" is bad. (As @19majkel94 said also) We wrap like 90% of express, we should wrap that last 10% as well instead of embracing of using different approaches outside of routing-controllers. Also having stuff "outside" of routing-contollers can lead to issues in future development.

undefined: "I'm not handling the response" - equivalent to next()

@marshall007 So if I understand correctly we cannot return value for chaining in interceptors? Putting stuff on req is not a good option. I would rather return a Symbol something like TerminateRequest (defined in routing-controllers so it's imported and the same across the whole app. ) What do you guys think about that?

requiring a discrete priority, but I don't think this is a common case.

@19majkel94 @marshall007 we wont need priority after the new loading mechanism of middlewares and interceptors lands? (defined in #255)

But because of this, @19majkel94 insights about priorities is right if we want to use both after/before in 2 interceptors with a different order than they were imported. But actually I dont see any valid use case for this, if priority matters users just can split the 2 interceptor into 4 (one interceptor for every method) and call them in the correct order. Right?

The only benefit of before & after in one class was the state (storying data in this)

I am strongly against using this as well, but as the interceptors will be inited once on startup we can simply seal/froze them to prevent usage of this.

And we would have the other benefit of encapsulating the logic of the same task (Like the RequestTimer example.)

Also taking this one step further this can seems more restrictive but I should prevent access to the req and res on every method and only give access to specific things to enforce proper usage.

A new addition would be an InterceptorContext type what is unique to every interceptor on every request. (Basically we can just attach it to res.context[numberOfThisInterceptor])

// InterceptorContext is typed and known, so we can have intellisense across the app 
// the class after creation is sealed so nothing can be changed by using `this`
// InterceptorAction instances are frozen as well when passing them into the methods to prevent bad stuff
class ListPaginator<MyContextType> implements BeforeInterceptor, AfterInterceptor {

  // the before method can access context what will be stored on the request (its a simple object)
  // action would not have req and res but instead body, queryParams, routeParams, root context etc
  // context is also injectable in other parts of the app eg in routes
  before(context: InterceptorContext, action: InterceptorAction) {
    contex.limit = action.queryParam.limit ? action.queryParam.limit : 30;
    contex.offset = action.queryParam.offset ? action.queryParam.offset : 0;
  }

  // there is no access to the request and response, so users cannot do bad stuff
  // context is already set in the before method so now it is 
  intercept(context: InterceptorContext, content: any) {
   // so i can do eg: 
    content.meta.limit = context.limit;
    content.meta.offset = context.offset;

    return content;
  }

  // the after method is called after the response is sent, it has access to everyting, but cannot send anything back tho the client
  after(context: InterceptorContext, action: InterceptorAction, content: any)) {
    action.request.start_time = this.start.toString();
    action.request.took = new Date() - this.start;
  }
}

 // error method has access to the context and the error
 error(context: InterceptorContext, error: Error) {
    // send error response
  }

Kind of a wild idea 😄 and I just sketched it so it will need some re adjustments ofc, but I think it can be a viable option.

[MichalLytek]

But actually I dont see any valid use case for this

The basic and described here example is the request timer.

if priority matters users just can split the 2 interceptor into 4 (one interceptor for every method) and call them in the correct order we would have the other benefit of encapsulating the logic of the same task (Like the RequestTimer example

But why introduce whole new concept which is complicated in use - sometimes you have class with two hooks, sometimes two classes with one method in one file, etc. If you can easily have encapsulation on module level:

// old JS way:
const NS_PER_MS = 1e3;
const MS_PER_S = 1e6;

function toMiliseconds([seconds, nanoseconds]: [number, number]) {
    return (seconds * MS_PER_S) + (nanoseconds * NS_PER_MS);
}

export const requestTimerBefore: MiddlewareSignature = (action, content) =>{
    action.request.startTime = process.hrtime();
}

export const requestTimerAfter: MiddlewareSignature = (action, content) => {
    const deltaTime = process.hrtime(action.request.startTime);
    action.request.took = toMiliseconds(deltaTime);
}

createExpressServer(app, {
    middlewares: {
        before: [
            requestTimerBefore,
            serveStatic({ path: "/public" }),
            jwt(jwtOptions),
            bodyParser.urlencoded(false),
        ],
        after: [
            morgan(process.env.LOG_FORMAT || 'common'),
            compression(),
            requestTimerAfter,
        ],
    }
    interceptors,
});   

// static class way
class RequestTimer {

    static readonly NS_PER_MS = 1e3;
    static readonly MS_PER_S = 1e6;

    static before(action: Action, content: any) {
        action.request.startTime = process.hrtime();
    }

    static after(action: Action, content: any) {
        const deltaTime = process.hrtime(action.request.startTime);
        action.request.took = this.toMiliseconds(deltaTime);
    }

    private static toMiliseconds([seconds, nanoseconds]: [number, number]) {
        return (seconds * this.MS_PER_S) + (nanoseconds * this.NS_PER_MS);
    }
}

createExpressServer(app, {
    middlewares: {
        before: [
            requestTimer.before,
            serveStatic({ path: "/public" }),
            jwt(jwtOptions),
            bodyParser.urlencoded(false),
        ],
        after: [
            morgan(process.env.LOG_FORMAT || 'common'),
            compression(),
            requestTimer.after,
        ],
    }
    interceptors,
});   

A new addition would be an InterceptorContext type what is unique to every interceptor on every request. (Basically we can just attach it to res.context[numberOfThisInterceptor])

This is good idea. Routing-controllers should provide abstraction above the bare req/res so injecting this from request property makes more sense. However it should be generic or sth to provide TS ability to attach custom property with type-safe.

[pleerock]

guys sorry for a small of-topic, but before discussing this I have a question. What did we decide with calling "next" after each controller action? As I remember last decision was to remove next call, am I right? If yes, then we need to do it. And if we do it then what is going to happen to "after middlewares". Will they work? (I think no, am I right?)

[MichalLytek]

Discussion about it is here - #223. All we need to do is to manually call the first after middleware after handleSuccess, passing the handler req,res/ctx and next parameters.

After middlewares in routing-controllers meaning doesn't exist at all in express/koa (due to not calling next in route handler). In koa it's ease to replace after middlewares with await next() but in express creating after middleware (hooking to handle the response from handler) is awful as you need to patch express res.end and other methods like the compression or session 3rd party middlewares do.

However, what are the use case of after middleware different than logging? Maybe we could just expose a hook for calling a function with injected action metadata rather than supporting whole big full middlewares?

[pleerock]

I don't know other use cases. Probably this question needs some research in exist express middlewares

[pleerock]

okay so what is resolution on this issue? Do you decided to go with original proposal in first message, or is something changed with interceptor proposals? Or do we change both of them?

[MichalLytek]

I don't know other use cases. Probably this question needs some research in exist express middlewares

In express there is no after middleware (registered after action handler) because routes don't call next. All after-like features use patching res.end to hook and do things after response (compression, cookie, etc.) so we won't find any example. They has to be registered as before middlewares so they doesn't suffer when we remove after middlewares feature, only custom middlewares that are used only for logging/cleanup. But custom middlewares can be rewritten with different signature outside of the express/koa stack, so we might expose a different option to register "after interceptors" and leave just "middlewares".

okay so what is resolution on this issue? Do you decided to go with original proposal in first message, or is something changed with interceptor proposals? Or do we change both of them?

I would introduce new signature now as it's a small change. New interceptors needs more discussion and polishing before implementing and making PR right now.

[pleerock]

We need to decide if we remove after middlewares or not before implementing this PR. Because if we do remove after middleware we don't need proposals in this PR

[NoNameProvided]

I would keep them, they are useful and we use them.

I am kind of lost now, what is the small change we want to introduce, @19majkel94 can you write dont again the final signature of the new loading api? To see what part is included and what not?

[MichalLytek]

Nothing has changed in my proposal from first post https://github.com/pleerock/routing-controllers/issues/256#issue-250706915. @marshall007 should open new issue with his own proposal.

So I will repeat - the old way:

createExpressServer({
    middlewares: [
        JWT, //before
        Logger, //after
        ErrorHanlder, //error
    ]
});

I propose an alternative way of loading which allows to omit @Middleware decorator (type and priority controlled in imperative way):

createExpressServer({
    middlewares: {
        before: [JWT],
        after: [Logger],
        error: [ErrorHandler],
    }
});

I would keep them, they are useful and we use them.

But do we need standard signature like (req, res, next)? With no action metadata it has limited usability. As we said, there's no standard middlewares registered as after middlewares, so we don't need compatibility - we can just let register it like an interceptors with access to returned value and action metadata.

[pleerock]

If we remove after middleware we don't need this functionality since separating before and error middlewares does not make any sense

[MichalLytek]

If we remove after middleware we don't need this functionality

@pleerock But we want to replace middlewares with enhanced interceptors - change of the signature req, res, next to action, ...others because we don't need compatibility with 3rd party after middlewares (they don't exist) but many users would like to have access to action metadata inside middlewares (#312).

So we should decide if we stick to the big interceptor class:

class RequestTimer implements BeforeInterceptor, AfterInterceptor {

    static readonly NS_PER_MS = 1e3;
    static readonly MS_PER_S = 1e6;

    before(action: Action, content: any) {
        action.request.startTime = process.hrtime();
    }

    after(action: Action, content: any) {
        const deltaTime = process.hrtime(action.request.startTime);
        action.request.took = this.toMiliseconds(deltaTime);
    }

    private toMiliseconds([seconds, nanoseconds]: [number, number]) {
        return (seconds * RequestTimer.MS_PER_S) + (nanoseconds * RequestTimer.NS_PER_MS);
    }
}

So the shape of the class (after, before and other methods from interface interceptor signature) decide when call which method, or we prefer simpler functional approach:

export const requestTimerBefore: MiddlewareSignature = (action, content) =>{
    action.request.startTime = process.hrtime();
}

export const requestTimerAfter: MiddlewareSignature = (action, content) => {
    const deltaTime = process.hrtime(action.request.startTime);
    action.request.took = toMiliseconds(deltaTime);
}

And we need to register it explicitly:

createExpressServer({
    middlewares: {
        before: [JWT],
        after: [Logger],
        error: [ErrorHandler],
    }
});

I know that the class one looks better but there's a problem with priority - request timer should be called as first before and last after, so we might need method decorators for priority which this #256 issue was all about 😆

Ping @NoNameProvided @marshall007 - we should decide as the "middlewares 2.0" is a big move and change that will fix calling next and other issues.

[NoNameProvided]

but there's a problem with priority - request timer should be called as first before and last after

Isn't this can be the golden rule? So after middlewares are executed in reverse order as before middlewares?

before: 
FirstMiddleware
SecondFirstMiddleware
ThirdFirstMiddleware

after:
ThirdFirstMiddleware
SecondFirstMiddleware
FirstMiddleware

And then we need no priority, I think this is the best path, won't fit all scenario, but makes the most sense. If someone needs some other logic, it must be split into different middlewares.

[MichalLytek]

Ok, so I need e.g. gzip comp middleware that has to be called as last after middleware - I should place it at first element of the array? Seems unintuitive 😕

[NoNameProvided]

Yeah, that's true. Ok so how about if we wait functions and we can provide it like this:

createExpressServer({
    middlewares: {
        before: [Logger. before , JWT],
        after: [Logger.after],
        error: [ErrorHandler],
    }
});

I know this is was your second option, but we should stress this way in the docs, so we can encapsulate code by logic, and have the flexibility at the same time. So the app expects an array of MiddlewareSignatures but we also have some XInterceptor interfaces.

[MichalLytek]

I want to do one big breaking change PR. Universal middlewares/interceptors with action metadata access + removing after middlewares + removing auto calling next (+ decorator for fallthrough/inject @Next) + loading mechanism change (if needed).

So we have to decide - class based mechanism (implement before, after, intercept methods) with optional priority decorator on methods (for cases like logger) or explicit configuration with static class help.

[NoNameProvided]

I vote for explicit configuration with static class help.

[github-actions[bot]]

Stale issue message