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
67.28k stars 7.59k forks source link

Configuration for microservice in hybrid application #4261

Closed 2Kable closed 4 years ago

2Kable commented 4 years ago

Feature Request

related to: https://github.com/nestjs/nest/issues/4112

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

We are running NestJS container that run a HTTP/GraphQL api, but also process GRPC, Queuing requests, in the same nestjs instance

I would like to run grpc alongside the graphql/http instance, and being able to intercept all request to log them, whether they are from GRPC or HTTP, using app.useGlobalInterceptors

const app = await NestFactory.create(AppModule, { logger })
app.useGlobalInterceptors(LoggerInterceptor)
app.connectMicroservice({
  transport: Transport.GRPC,
  options: {}
})

Describe the solution you'd like

We fixed it by patching nest source code and passing the NestApplication config in the NestMicroservice constructor

- const applicationConfig = new ApplicationConfig();
- const instance = new NestMicroservice(this.container, options, applicationConfig);
+ const instance = new NestMicroservice(this.container, options, this.config);

But this would lead to a huge breaking change for current application that wants a separate configuration.

I would suggest an optional argument/property to pass to the connectMicroservice function to use or provide an existing ApplicationConfig instance

Another solution could be adding a method to attach a NestMicroservice instance to the NestApplication without relying on connectMicroservice

  public attachMicroservice(instance: NestMicroservice): INestMicroservice {
    instance.registerListeners();
    instance.setIsInitialized(true);
    instance.setIsInitHookCalled(true);
    this.microservices.push(instance);
    return instance;
  }

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

I would expect that hybrid application can share the same ApplicationConfig. Not being able to set global guard/interceptors for nest-grpc running in an hybrid application, greatly reduce the experience of it.

jmcdo29 commented 4 years ago

Are you able to globally bind interceptors and guards using custom providers, e.g.

{
  provide: APP_INTERCEPTOR,
  useClass: InterceptorClass
}
2Kable commented 4 years ago

That's how i'm currently doing it. But the APP_INTERCEPTOR is set using useGlobalInterceptors at NestFactory.createApplicationContext time

NestFactory.createApplicationContext -> NestFactory.initialize -> Scanner.applyApplicationProviders -> Scanner.getApplyProvidersMap -> it then adds the global interceptor to the ApplicationConfig

but the ApplicationConfig instance is not used when running connectMicroservice(), it create a new one

jmcdo29 commented 4 years ago

Oh, that's super interesting. I haven't had a chance to play around with Hybrid Applications much, so that's good to know.

kamilmysliwiec commented 4 years ago

Maybe we can add an additional argument to the connectMicroservice() method(?). What about something like this:

const transportOptions = {
  transport: Transport.GRPC,
  options: {}
};

app.connectMicroservice(transportOptions, { inheritAppConfig: true });
2Kable commented 4 years ago

Looks good to me. I can start working on the PR if the specs are a go

jmcdo29 commented 4 years ago

Out of curiosity, would this also affect apps with WebSockets? I'm working on a logging library and using the custom provider for globally binding interceptors doesn't seem to bind for gateways. Only HTTP calls.

2Kable commented 4 years ago

I not familiar with WS implementation, but after a quick look I don't think this is the same issue

Jekiwijaya commented 4 years ago

Hi, i'm also facing this same issue, my global interceptor is ignored in microservices,

I see the pull request has been made, is there anything I can help to make it merge? Thank you

2Kable commented 4 years ago

Hi, PR is ready, but I'm struggling with the test coverage. As far as I know it's just because I added lines, but can't actually covers them So feel free to contribute/help

kamilmysliwiec commented 4 years ago

@2Kable I'll take care of this, no worries! Thanks

kamilmysliwiec commented 4 years ago

Added in 7.1.0. Thanks!