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

Hybrid application's microservice doesn't inherit application configuration #4112

Closed 2Kable closed 4 years ago

2Kable commented 4 years ago

Bug Report

Similar to #353 but with suggestion to fix

Current behavior

When connecting a microservice to a NestApplication I would expect it to inherit configuration like Interceptors and such

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

In this case the microservice will not inherit the Application global interceptor

this is caused in packages/core/nest-application.ts

  public connectMicroservice(options: MicroserviceOptions): INestMicroservice {
    const { NestMicroservice } = loadPackage(
      '@nestjs/microservices',
      'NestFactory',
      () => require('@nestjs/microservices'),
    );

    const applicationConfig = new ApplicationConfig();
    const instance = new NestMicroservice(
      this.container,
      options,
      applicationConfig,
    );
    instance.registerListeners();
    instance.setIsInitialized(true);
    instance.setIsInitHookCalled(true);

    this.microservices.push(instance);
    return instance;
  }

NestMicroservice instance is instantiated with a whole new ApplicationConfig()

Expected behavior

I would expect that hybrid application can share the same ApplicationConfig

Possible Solution

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 the 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;
  }

Environment


Nest version: 6.11.6
kamilmysliwiec commented 4 years ago

image

This is intentional, see docs: https://docs.nestjs.com/guards#binding-guards

2Kable commented 4 years ago

Hi, I get that this is intended, but is this design improvable ?

Not being able to set global guard/interceptors for nest-grpc running in an hybrid application, greatly reduce the experience of it.

My use case is: I want to run grpc alongside the graphql/http instance, and being able to intercept all request to log them is kinda basic.

And there's currently no way to set it up in an hybrid, or event create a NestMicroservice instance ourselves and attach it to the NestApplication

The change can be non breakable, and greatly improve the hybrid experience

regevbr commented 2 years ago

@2Kable totally agree with you!