Nest: [Discussion] Middleware API (community question)

[kamilmysliwiec]

I'm submitting a...

[ ] Regression 
[ ] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.
[x] Discussion

Current behavior

Currently, each class middleware (injectable middleware) API looks like this:
```typescript
import { Injectable, NestMiddleware, MiddlewareFunction } from '@nestjs/common';

@Injectable()
export class LoggerMiddleware implements NestMiddleware {
resolve(...args: any[]): MiddlewareFunction {
return (req, res, next) => {
console.log('Request...');
next();
};
}
}

In order to pass paramaters to `resolve` method, we can use `.with()` construction:
```typescript
consumer
      .apply(LoggerMiddleware)
      .with('ApplicationModule') <--- THIS
      .forRoutes(CatsController);

However, I'm not certainly sure whether this syntax is still relevant (truly makes sense). Is there anybody who uses this approach? Instead, I prefer using a typical DI to pass options:

export class LoggerMiddleware implements NestMiddleware {
  constructor(
     @Inject(LOGGER_MIDDLEWARE_OPTIONS) options: LoggerMiddlewareOptions,
  ) {}

  resolve(...args: any[]): MiddlewareFunction {
    return (req, res, next) => {
      console.log('Request...');
      next();
    };
  }
}

OR in more advanced cases, with mixins:

export function LoggerMiddleware(options: LoggerMiddlewareOptions) {
  @Injectable()
  class LoggerMiddlewareCtor implements NestMiddleware {
    resolve(...args: any[]): MiddlewareFunction {
      return (req, res, next) => {
        console.log('Request...');
        next();
      };
    }
  }
  return LoggerMiddlewareCtor;
}

Expected behavior

It leads me to a conclusion, should we really still support with() syntax? I believe that without this functionality, we could simplify API to the following one:

@Injectable()
export class LoggerMiddleware implements NestMiddleware {
  use(req: Request, res: Response, next: Function) {
    console.log('Request...');
    next();
  }
}

What do you think?

Minimal reproduction of the problem with instructions

What is the motivation / use case for changing the behavior?

Easier API

Environment

Nest version: X.Y.Z


For Tooling issues:
- Node version: XX  
- Platform:  

Others:

[marcus-sa]

Thought you were against breaking changes to the API?
Kinda misconception that it doesn't apply when its yourself, but only when it comes to suggestions everybody else makes.

[BrunnerLivio]

Thanks for your transparency and sharing your thoughts with the community!

In my opinion this does totally make sense. Your new proposal is definitely less confusing.

In the manner of my application which is using NestJS; this should be totally able to upgrade to the new proposal.
But I do not know about others; can this potentially create some issues when upgrading? If so, why?

[adrien2p]

I am totally agree with this change. I don’t think that will be an hard
upgrade to do as well.

[Wizp]

By my opinion Middlewares should be stateless, so I like your ideas.
This changes will be breaking changes anyway, right?
"Expected behavior" example contains syntax similar with express.js, what about fastify.js compatibility?

[kamilmysliwiec]

"Expected behavior" example contains syntax similar with express.js, what about fastify.js compatibility?

Nothing would change in this case (middleware would remain compatible with fastify).

[pumano]

Middleware in case of express is "Chain of responsibility" pattern.

Also similar scenario under the hood (but in more object oriented way) at Angular http client module (with interceptors).

IMHO looks like your @kamilmysliwiec expected result is clearer, than using with().

[kamilmysliwiec]

6.0.0 has been published.

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.