golevelup / nestjs

A collection of badass modules and utilities to help you level up your NestJS applications 🚀
MIT License
2.23k stars 259 forks source link

RabbitMQ: Logs still not right Nest 8 #487

Closed zogot closed 1 year ago

zogot commented 2 years ago

Versions:

Something still isn't quite right with the logs. When I do this:

const app = await NestFactory.create(AppModule, {
    logger: false,
  });

I still get logs from RabbitMQModule:

[Nest] 31  - 08/02/2022, 2:54:27 PM     LOG [AmqpConnection] Trying to connect to RabbitMQ broker (default)
[Nest] 31  - 08/02/2022, 2:54:27 PM     LOG [AmqpConnection] Successfully connected to RabbitMQ broker (default)
[Nest] 31  - 08/02/2022, 2:54:27 PM     LOG [RabbitMQModule] Successfully connected to RabbitMQ
[Nest] 31  - 08/02/2022, 2:54:27 PM     LOG [AmqpConnection] Successfully connected a RabbitMQ channel "AmqpConnection"
[Nest] 31  - 08/02/2022, 2:54:27 PM     LOG [RabbitMQModule] Initializing RabbitMQ Handlers
[Nest] 31  - 08/02/2022, 2:54:27 PM     LOG [RabbitMQModule] Registering rabbitmq handlers from AddressListenerService
[Nest] 31  - 08/02/2022, 2:54:27 PM     LOG [RabbitMQModule] AddressListenerService.onInteractionRecorded {subscribe} -> pubsub::interaction.recorded::service-suspect:address-listener:on-interaction-recorded

and nothing further.

I am suspecting its to do with the new logger instances created here but I haven't had the chance currently to put into real testing.

  1. https://github.com/golevelup/nestjs/blob/master/packages/rabbitmq/src/rabbitmq.module.ts#L91

because the official recommended ways to replace the logger just dont work with the latest changes:

  1. https://github.com/iamolegga/nestjs-pino#example
  2. https://docs.nestjs.com/techniques/logger#dependency-injection

Do you think that it can just be changed to how all the other modules do it?

  1. https://github.com/nestjs/graphql/blob/master/packages/graphql/lib/graphql.module.ts#L54
  2. https://github.com/nestjs/bull/blob/3b94b9dc6f9dcde444dbed6c04f41f28d7b9e025/packages/bullmq/lib/bull.explorer.ts#L32
  3. https://github.com/nestjs/sequelize/blob/5b4c350db87a4fb7b519ada41b7eb604366ca0d9/lib/common/sequelize.utils.ts#L10
  4. https://github.com/nestjs/cqrs/blob/d6c176bed923fe38c18d2a05c35c72a3dca6e663/src/event-bus.ts#L36
  5. https://github.com/nestjs/azure-database/blob/e368290afd64ff0768e13ddfaad9813c16a7905d/lib/table-storage/azure-table.service.ts#L3

I don't use all of those, but none of them require defining a custom logger in their arguments for it to work correctly with overriding your own logger.

zogot commented 2 years ago

@alko89 Yep I think that might have been it.

I'm guessing version 3.0 is Nest 9 only then? Perhaps that needs to be defined in the CHANGELOG? And if so, could we get #482 backmerged into the 2.x.x line?

As my nestjs-rabbitmq now has node_modules of @golevelup/nestjs-modules 0.6.1 which https://github.com/golevelup/nestjs/blob/master/packages/modules/package.json#L39 has a peerDep on @nestjs/common 9.x

zogot commented 2 years ago

Specifically point at

"@golevelup/nestjs-rabbitmq": "^2.4.0",

Still suffers from the issue I described in the main issue though, so I think its 2 parts.

3.X depends on Nest 9 dependencies 2.X still has something not correct with the logger

zogot commented 2 years ago

Oh, I still see the sub node_modules, so looks like Nest 9 is set as peer dependency before the 3.X version

zogot commented 2 years ago

Issue is in 2.4.1 and up. Could we get version 2 to stay with Nest 8 and version 3 to go with Nest 9?

alko89 commented 2 years ago

I guess one solution would be to have both nest 8 and 9 as peer dependencies. The only real breaking change in v9 were unit tests, so it should (theoretically) work with both.

I wouldn't backport these features to later versions though and I don't think versioning works like that in this project. But I'm not a maintainer and it works for me :)