Skip to content
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

feature request: RabbitMQ module support custom logger through DI #607

Closed
Hareloo opened this issue Jun 21, 2023 · 9 comments
Closed

feature request: RabbitMQ module support custom logger through DI #607

Hareloo opened this issue Jun 21, 2023 · 9 comments
Labels
enhancement New feature or request rabbitmq

Comments

@Hareloo
Copy link

Hareloo commented Jun 21, 2023

Currently, I am not sure if this package supports NestJS v10.
I am seeing one weird behavior which is that every log from the logger within rabbitmq.module.ts (private readonly logger = new Logger(RabbitMQModule.name); at line 67) seems to not go to the logger instance I passed but rather goes to the default logger of NestJS...

@Hareloo Hareloo changed the title Allow support for NestJS v10 Allow support for NestJS v10 (will fix logger too?) Jun 21, 2023
@underfisk
Copy link
Contributor

@Hareloo Nestjs is a peer dependency just asking for >9.x.x meaning that v10 is supported
Could you add a repro of what you're seeing, that might be easier to check the issue

@underfisk underfisk added the Pending Repro A repro is pending in order to replicate the issue label Jul 2, 2023
@Hareloo
Copy link
Author

Hareloo commented Jul 2, 2023

Here's a reproduction: https://github.com/Hareloo/repro270623
Take note that you'll need to manually start up a rabbitmq instance on your local machine for it to run

@underfisk
Copy link
Contributor

Thanks, as soon as i can I'll give it a shot and investigate

@underfisk
Copy link
Contributor

@Hareloo So it looks like our logger is being created statically, meaning it doesn't rely on the DI logger. I think we would have to support receiving the logger through the constructor so that it could be initialized using the right factory. Right now I don't have enough time to develop such a feature as I'm not also using the rabbitmq module anymore but if you're willing to push such feature I'll be happy to review and push

@underfisk underfisk added enhancement New feature or request rabbitmq and removed Pending Repro A repro is pending in order to replicate the issue labels Jul 2, 2023
@underfisk underfisk changed the title Allow support for NestJS v10 (will fix logger too?) feature request: RabbitMQ module support custom logger through DI Jul 2, 2023
@Hareloo
Copy link
Author

Hareloo commented Jul 3, 2023

Seems like this would be more complicated than you suggested, since all the packages of @golevelup are looking for NestJS version 9 exactly, not > 9.x.x which means v10 isn't supported.
This means we would have to upgrade all packages of @golevelup to support NestJS v10 as well as make sure nothing breaks...

@underfisk
Copy link
Contributor

underfisk commented Jul 3, 2023

@Hareloo We might have to support v10 then first if you're willing to contribute that otherwise once I have some free time I'll push the peer to support v10

@underfisk
Copy link
Contributor

@Hareloo I had some free time and I'm pushing support to v10 #617

@Hareloo
Copy link
Author

Hareloo commented Jul 4, 2023

@underfisk Alright, after it's merged I will test again (since when I'm using NestJS v9 my custom logger does work) and see if maybe that was enough to solve this.
If it isn't I will see how I can create a PR about this

@Hareloo
Copy link
Author

Hareloo commented Jul 21, 2023

Fixed now, I guess the logger couldn't initialize correctly due to mismatch in nestjs versions.

@Hareloo Hareloo closed this as completed Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rabbitmq
Projects
None yet
Development

No branches or pull requests

2 participants