Controllers, as depicted by the documentation, are actually Routers

[ericmorand]

The documentation encourages to give controllers two reponsibilities:

• Dispatching routes
• Handling routes execution

For example:

@Controller('cats') // dispatch
export class CatsController {
  @Get() // dispatch
  findAll(): string {
    return 'This action returns all cats'; // execution
  }
}

https://docs.nestjs.com/controllers#routing

It de-facto defines NestJS controllers as routers: deciding how requests are dispatched is the exclusive responsibility of a Router.

It is a violation of the Single Responsibility Principle: the routing decision is not hidden to the controller and the controllers have two responsibilities.

Possible Solution

NestJS documentation should clearly disambiguate the meaning of controllers and establish clearly that they are routers. It should also provide proper examples on how they should be implemented in regard to the SOLID principles.

[jmcdo29]

Could you give an example, in say, Express or Fastify, how you would define a Router and a Controller that follows the patterns you mention? I'm having a hard time seeing the separation (or lack thereof) you're mentioning.

[ericmorand]

Here is how we handle that issue in our application (condensed into a single file for readability):

import { MessagePattern, Transport } from '@nestjs/microservices';
import { ATopic } from './index';
import { Inject } from '@nestjs/common';

export interface ControllerInterface {
  execute: (message: any) => Promise<void>;
}

/**
 * A properly single-responsibility controller, knowing nothing about the message pattern that was matched
 */
class ControllerA implements ControllerInterface {
      execute: (message: any) => Promise<void> {
         // do something
     }
}

/**
 * The actual dispatch function, using MessagePattern property decorator under the hood
 */
const dispatchKafkaTopic = (name: string, to: ControllerInterface) => {
  MessagePattern(name, Transport.KAFKA)(undefined, undefined, {
    value: (message: any) => to.execute(message)
  });
};

/**
 * Unfortunately, in NestJS terminology, Controllers are Routers instead of Route Handlers.
 * This is easily demonstrated by the fact that NestJS Controllers are the only components aware of the route
 * they are supposed to handle - for example through the @MessagePattern decorator. Knowing how to route
 * a message or a request is the exclusive scope of routers. Hence NestJS controllers are routers.
 *
 * https://github.com/nestjs/docs.nestjs.com/issues/1857
 *
 * We mitigate this violation of the SOLID principle by providing a proper router which responsibility is
 * to dispatch routes to route handlers instead of actually handling the routes.
 */
export class Router {
  constructor(
    @Inject(ControllerA) private readonly controllerA: ControllerA,
    @Inject('TopicA') private readonly topicA: string
  ) {
    dispatchKafkaTopic(this.topicA, this.controllerA);
  }
}

And our module looks like this:

@Module({
    controllers: [Router],
    providers: [
        ControllerA,
        TopicA
    ]
})

As you can see, only the Router knows how to dispatch the messages. The controllers actually don't know anything about what route was matched. It gives them a single responsibility and respect the S of the SOLID principle:

• Would the message pattern change, only the router would be impacted
• Would the message handler change, only the controller would be impacted

With the approach of the documentation, the controller would be impacted by those two changes: either the message pattern changes or the message handler change, the controller would be impacted.

Ideally, NestJS's Get, Post, MessagePattern... decorators should be class decorators, taking a token to a controller interface instance, and a Router decorator should be added. They all could be used this way:

@Router('Transport.KAFKA')
@MessagePattern('my-topic', MyTopicController) 
@MessagePattern('my-other-topic', MyOtherTopicController) 
class Router {
}

By using this approach, Routers are responsible for dispatching while Controllers are responsible for execution.

[kamilmysliwiec]

I'm looking at the examples you provided above and honestly, they look way more difficult to follow than what Nest's docs propose as a best practice. Luckily, Nest is flexible enough that anyone can apply their own patterns and, similarly to your example above, come up with its own pattern and use in its applications, but we definitely don't want to encourage such ^ solution on our side. I think the key thing here is what we/you understand as a "router". For instance, Nest is designed in a way in which "router" is in fact hidden somewhere within the framework's internals, and it's in charge of routing requests to specific route handlers that in turn are defined in controllers. Controllers can define routes using annotations, but they are not truly routers so your nomenclature wouldn't fit here as they technically cannot register anything without a framework. Without a real router, a tool that could map those methods to corresponding routes, they are simple TS, library/framework-agnostic classes, so you can instantiate a controller's class within your test suite using new and that wouldn't lead to registering any route.

Anyways, I could go on and on about this, but I don't have enough time and I think I had similar discussion(s) in the past, so if you start looking through old issues, you should definitely find more on that. TL;DR I don't plan any changes regarding the naming conventions/architectural building blocks we promote currently as a framework.

[ericmorand]

Still, controllers are impacted by two changes of different perimeters:

• Changes on the route pattern
• Changes on the route execution

From a SOLID point of view, this is a violation. If the message pattern changes, then the controller is impacted. If the business logic of the route handler changes, then the controller is impacted.

A class should have only one reason to change

https://en.wikipedia.org/wiki/Single-responsibility_principle

Without a real router, a tool that could map those methods to corresponding routes, they are simple TS, library/framework-agnostic classes, so you can instantiate a controller's class within your test suite using new and that wouldn't lead to registering any route.

And using a router, all the above is equally possible.

But it also allows for something that is not currently possible: having routes as part of the configuration. Nest's approaches enforces having routes as constants known as soon as the controller module is imported - that is before anything is executed. And then, enforces a hard dependency between a controller and a route.

Maybe I missed something but can you explain me how you would deal with the following flow of events without changing the controller twice because of two changes of different perimeter?

• Specification says that there is a route named "foo" that returns 5
• Later on, specification says that the route should be named 'bar'
• Later on, specification says that the route should return 6

From my perspective, using NestJS's documentation approach:

• A controller is created using @MessagePattern('foo') on a function that returns 5
• The controller is changed to use @MessagePattern('bar')
• The controller is changed again to have the function return 6

It seems to break the principle "A class should have only one reason to change". Here, the controller class has two reasons to change: because the route name did change; because the route result change.

I'm looking at the examples you provided above and honestly, they look way more difficult to follow than what Nest's docs propose as a best practice.

That they look more difficult should not be a matter of interest. I'm not talking about the ease of use of the framework. I'm talking about its appliance to the SOLID principles that the framework advertises itself as being compliant to.

[jmcdo29]

But it also allows for something that is not currently possible: having routes as part of the configuration. Nest's approaches enforces having routes as constants known as soon as the controller module is imported - that is before anything is executed. And then, enforces a hard dependency between a controller and a route.

Technically this isn't impossible, as you could use process.env to get your route configurations, or read them from a file, so long as you get how decorators in Typescript work

Maybe I missed something but can you explain me how you would deal with the following flow of events without changing the controller twice because of two changes of different perimeter?

If you're really adverse to making changes to the controller/gateway/resolver twice, you could keep your route strings in a separate file of constants, import them from the controller/resolver/gateway class, and then have this separation you're looking for. Something like

// constants.ts
export const catPrefix = 'cats';
export const getCats = '';
export const getOneCat = ':id';
export const newCat = 'new'

import { catPrefix, getCats, getOneCat, newCat } from './constnats';

@Controller(catPrefix)
export class CatsController {

    @Get(getCats)
    getAllCats() {}

    @Get(getOneCat)
    getOneCat(@Param() catParams: GetOneCatParamsDto) {}

    @Post(newCat)
    insertNewCat(@Body() cat: NewCatDto) {}
}

And now if you need to change the route, the controller class isn't touched. If you're using DTOs too with strict validation, the controller class isn't affected when you change the expected payload type either, because those are also separate classes, so now the controller is a one-and-done situation.

That they look more difficult should not be a matter of interest.

I disagree, if clarity of code is sacrificed to follow a design principle, then the reason for following that principle is already lost. The paradigms are around to help us as developers create clean, easily readable code that can be modified with relative ease.

I'm talking about its appliance to the SOLID principles that the framework advertises itself as being compliant to.

Nest (NestJS) is a framework for building efficient, scalable Node.js server-side applications. It uses progressive JavaScript, is built with and fully supports TypeScript (yet still enables developers to code in pure JavaScript) and combines elements of OOP (Object Oriented Programming), FP (Functional Programming), and FRP (Functional Reactive Programming). - from the first page of the Docs.

The only time Nest uses the term SOLID and points to an outside reference in in the feature module documentation

[ericmorand]

import { catPrefix, getCats, getOneCat, newCat } from './constnats';

@Controller(catPrefix)
export class CatsController {

    @Get(getCats)
    getAllCats() {}

    @Get(getOneCat)
    getOneCat(@Param() catParams: GetOneCatParamsDto) {}

    @Post(newCat)
    insertNewCat(@Body() cat: NewCatDto) {}
}

Except if I missed something, you can't add a new route to a controller without changing the controller module with your approach. For example, you can't have a new route linked to an existing controller action with your approach. Which once again lead to a "two reasons of change" for this component.

With a proper router, you would just register the new route with the proper controller and the controller would never know that it is now targetted by a new route.

I disagree, if clarity of code is sacrificed to follow a design principle, then the reason for following that principle is already lost. The paradigms are around to help us as developers create clean, easily readable code that can be modified with relative ease.

SOLID principle has not been established to make things more readable. It has been established to make the product more predictable, resilient and maintainable. You are totally entitled to think that these qualities are less important than code clarity but you should not confuse these two separate concerns.

[jmcdo29]

SOLID principle has not been established to make things more readable

SOLID allows programmers to write code that is easier to understand and change later on -from Wikipedia. Personally, I read "easier to understand" as "more readable". I also don't feel it's too difficult to add a new route to the controller class, but you may have different feelings on that.

Except if I missed something, you can't add a new route to a controller without changing the controller module with your approach.

That you are correct about, but, and I'll repeat myself here, the only mention of SOLID in the docs is about feature modules. While SOLID is a good design approach, I feel it is overkill here.

I get what you're getting at, but as Kamil said, Nest is flexible enough if you want to implement a system as you've shown above to have, you're free to do so. If it feels like it's that major of a violation for you, then feel free to implement your own system around it, but I don't think there's any action item for changing the docs in any way.

[ericmorand]

I get what you're getting at, but as Kamil said, Nest is flexible enough if you want to implement a system as you've shown above to have, you're free to do so.

You are perfectly right. And I may add that it was a very good surprise for us that we were able to solve that issue so easily and without hacking anything.