jmcdo29 / nest-commander

A module for using NestJS to build up CLI applications
https://nest-commander.jaymcdoniel.dev/
MIT License
435 stars 53 forks source link

Change CommandFactory in order to be able to access the `INestApplicationContext` before runner execution. #852

Closed floriantz closed 1 year ago

floriantz commented 1 year ago

Is there an existing issue that is already proposing this?

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

Hello ! First of all thank you for this very nice project !

In the nestjs application I have and on which I'd like to add a cli, we are relying on nestjs-pino with a custom LoggerModule which we register like so:

const app = await NestFactory.create(AppModule, { bufferLogs: true });
app.useLogger(app.get(Logger));

As the CommandFactory is currently designed, it is impossible to access the INestApplicationContext returned from the NestFactory.create(...) before the runner is run, which is what I'd like to have control on.

Describe the solution you'd like

I understand the benefits of having most CommandFactory methods as private but eventually making those protected or changing the API might allow extending the CommandFactory and make a custom one that fits more complex use-cases where accessing instantiated modules before we start the command runner is necessary.

Teachability, documentation, adoption, migration strategy

No response

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

The current API is a bit too restrictive and block our usage of nestjs-pino in our current app. A bit more flexibility on the CommandFactory would unblock some usages such as this one.

jmcdo29 commented 1 year ago

I'm fine with a PR to make most of the methods protected instead to give that flexibility. I don't think a logger makes much sense in the final version of the CLI, at least for the Nest startup logs, but that's my opinion

jmcdo29 commented 1 year ago

I'll get to work on this here soon to add in some more usability for if you need scenarios like your use case :)

grant-otus commented 1 year ago

I'd like to +1 this functionality, I'm also trying to implement pino logger into a CLI to get more details on some of the inner working.

jmcdo29 commented 1 year ago

@grant-otus check out the linked PR and see if you believe it should fit your use case :)

grant-otus commented 1 year ago

@jmcdo29 I believe it will, all I need is access to the INestApplicationContext, so that looks golden to me.

jmcdo29 commented 1 year ago

Available in 3.7.0

Sadzurami commented 1 year ago

@jmcdo29

Hello! Thanks for great project and for changes made here. Now app creation looks much better. But, can we pass to options all available options of NestFactory.createApplicationContext?

As mentioned above, i would like to logs all with nestjs-pino, but initial logs are missed since 'bufferLogs' are not set here.

https://github.com/jmcdo29/nest-commander/blob/d3d89b403bbd5b969f39acc0b182d0241df31e80/packages/nest-commander/src/command.factory.ts#L49C10-L54

Maybe merge CommandFactoryRunOptions with Partial<NestApplicationContextOptions> ?

https://github.com/jmcdo29/nest-commander/blob/d3d89b403bbd5b969f39acc0b182d0241df31e80/packages/nest-commander/src/command-factory.interface.ts#LL12C1-L20C2

Also.. I think this is a bug. If we pass an empty object in the CommandFactory.createWithoutRunning option or for example 'cliName' - as a result, the built-in Nest logger will be enabled (but we did not enable it ourselves).

Sadzurami commented 1 year ago

@jmcdo29 What do you think about my previous message?

jmcdo29 commented 1 year ago

Also.. I think this is a bug. If we pass an empty object in the CommandFactory.createWithoutRunning option or for example 'cliName' - as a result, the built-in Nest logger will be enabled (but we did not enable it ourselves).

Do you think you could make a reproduction with this? Just want to double check it/make it easily added to the test suite

As mentioned above, i would like to logs all with nestjs-pino, but initial logs are missed since 'bufferLogs' are not set here.

d3d89b4/packages/nest-commander/src/command.factory.ts#L49C10-L54

Maybe merge CommandFactoryRunOptions with Partial ?

d3d89b4/packages/nest-commander/src/command-factory.interface.ts#LL12C1-L20C2

Makes sense. We should be able to merge those interfaces pretty easily.

What do you think about my previous message?

All sounds good. @Sadzurami Would you like to work on a PR for this? Apologies for the slight delay, I was taking the weekend to enjoy some time with family :)

Sadzurami commented 1 year ago

@jmcdo29 Sorry to bother you, I didn't notice it was the weekend.

PR created.

jmcdo29 commented 1 year ago

Available in 3.8.0

coler-j commented 7 months ago

@Sadzurami do you have an example of how you pass your pino logger?

coler-j commented 7 months ago

NVM It is

export async function bootstrap() {
  const app = await CommandFactory.createWithoutRunning(
    AppModule,
    standaloneAppConfigurationOptions,
  );
  app.useLogger(.... ) < do what need to do
  await CommandFactory.runApplication(app);
}

void bootstrap();