nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript 🚀
https://nestjs.com
MIT License
68.06k stars 7.66k forks source link

Add ability to disable LazyModuleLoader's logging #9107

Closed Gbuomprisco closed 2 years ago

Gbuomprisco commented 2 years ago

Is there an existing issue that is already proposing this?

Is your feature request related to a problem? Please describe it

As a serverless user, I love the ability to lazy load services!

The only thing I would change is the ability to disable the logging of each dependent service when initializing a lazy-loaded module.

See below for the default behavior:

image

Describe the solution you'd like

I'd leave this to the contributors, but maybe a second parameter added to the load method would work okay, such as:

const module = await this.lazyModuleLoader.load(() => NestModuleClass, { logger: false });

return module.get(Service);

Again, 100% up to the frameworks designer for the best way to design the API

My current workaround is to extend the logger and add a list of contexts that I skip when logging an entry.

Teachability, documentation, adoption, migration strategy

Users can choose to disable/override/change the default behavior, eg. logging each dependency that gets initialized. The API above would be one way.

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

I lazy load a lot, and I'd love to see cleaner logs.

micalevisk commented 2 years ago

btw this is due to the following

https://github.com/nestjs/nest/blob/89124ad6d33cbeb55d1f11d6dd69f9e7a95b0f47/packages/core/injector/internal-core-module-factory.ts#L39-L48

kamilmysliwiec commented 2 years ago

Sounds good! Would you like to contribute & address this issue?

Gbuomprisco commented 2 years ago

I can give it a try! What do you think of the interface?

hcharley commented 2 years ago

I was googling for how to disable "InstanceLoader" and came across this just now. I think this would be helpful for me as well.

hcharley commented 2 years ago

I started on a PR for disabling logs for InstanceLoader. Not sure if it applies to LazyModuleLoader.

It is still very much in "WIP" status (although for some reason, GH won't let me set the PR to "draft").

https://github.com/nestjs/nest/pull/9143

micalevisk commented 2 years ago

interface 1

I'm thinking on adding the option logger to LazyModuleLoader#load like the one used in NestFactory.create (not sure if is feasible)

https://github.com/nestjs/nest/blob/df2dab4fccc5e9a32e2305929e09c7392b3daf78/packages/common/interfaces/nest-application-context-options.interface.ts#L10

this.lazyModuleLoader.load(() => NestModuleClass, { logger: false })
// ... or
this.lazyModuleLoader.load(() => NestModuleClass, { logger: MyLogger })
// ... or
this.lazyModuleLoader.load(() => NestModuleClass, { logger: ['error'] })

The consumer of that provider could create a wrapper around it to avoid supplying that option every time

like this ```ts @Injectable() export class AdapterLazyModuleLoader { constructor(private readonly lazyModuleLoader: LazyModuleLoader) {} load(arg: Parameters[0]) { return this.lazyModuleLoader.load(arg, { logger: false }) } } ```
micalevisk commented 2 years ago

interface 2

or maybe we should add a new public method to LazyModuleLoader class instead? :thinking:

@Injectable()
export class AppService {
  constructor(private readonly lazyModuleLoader: LazyModuleLoader) {
    this.lazyModuleLoader.useLogger(false) // this implies in using a 'noop logger' under the hood
    this.lazyModuleLoader.useLogger(['error'])
    this.lazyModuleLoader.useLogger(MyLogger)
  }
}
kamilmysliwiec commented 2 years ago

"interface 1" option would be great to have @micalevisk!

micalevisk commented 2 years ago

the disabling part is easy to achieve, but allow passing another logger instance or class ref is not. We cannot just override the logger like how is done below otherwise we will be changing the 'main' logger

https://github.com/nestjs/nest/blob/df2dab4fccc5e9a32e2305929e09c7392b3daf78/packages/core/nest-factory.ts#L229-L243

in my POC I didn't find any way to implement that while allowing the logger to be injectable :/

kamilmysliwiec commented 2 years ago

the disabling part is easy to achieve, but allow passing another logger instance or class ref is not. We cannot just override the logger like how is done below otherwise we will be changing the 'main' logger

So let's just implement the disabling part for the time being. I can't think of a use case where using a separate logger class (specifically for lazy-loaded modules) would make sense anyway.

kamilmysliwiec commented 2 years ago

Let's track this here https://github.com/nestjs/nest/pull/9836

clintonb commented 11 months ago

I see that https://github.com/nestjs/nest/pull/9836/files was merged, but how does one actually make use of the changes to disable logging?

micalevisk commented 11 months ago

@clintonb image

clintonb commented 11 months ago

I read all of that, and it doesn’t help me. I am using nest-commander to write CLI tools. I want to get rid of the “…dependencies initialized” messages. How can I do that?

Alternatively, given that nest-commander initializes apps in foreign (to me) manner, how would I do this on the simplest of web apps? If I have that example, I can fork/adapt commander as needed.

micalevisk commented 11 months ago

@clintonb

oh, that's unrelated to this issue.

Try the following instead: