Dependency injector resolves dependencies using a class name instead of a class reference

[tooleks]

Bug Report

Current behavior

Dependency injector resolves dependencies using a class name instead of a class reference. It causes conflicts when two different classes share the same name.

Input Code

REPL: https://repl.it/@tooleks/CloudyRoundGnuassembler

// index.ts

import { NestFactory } from '@nestjs/core';
import { ExampleModule } from './example.module';

async function bootstrap() {
  await NestFactory.createApplicationContext(ExampleModule);
}

bootstrap();

// example.module.ts

import { Inject, Module } from '@nestjs/common';
import { AService as A1Service } from './a-1.service';
import { AService as A2Service } from './a-2.service';

@Module({
  providers: [A1Service, A2Service],
})
export class ExampleModule {
  constructor(@Inject(A1Service) private readonly aService: A1Service) {}

  onModuleInit(): void {
    console.log('Expected: a-1.service.ts');
    console.log(`Actual: ${this.aService.file}`);
  }
}

// a-1.service.ts

import { Injectable } from '@nestjs/common';

Injectable()
export class AService {
  public readonly file = 'a-1.service.ts';
}

// a-2.service.ts

import { Injectable } from '@nestjs/common';

Injectable()
export class AService {
  public readonly file = 'a-2.service.ts';
}

// Output
Expected: a-1.service.ts
Actual: a-2.service.ts

Expected behavior

// Output
Expected: a-1.service.ts
Actual: a-1.service.ts

Possible Solution

Register and resolve dependencies using a class reference instead of class name as a dependency injection token.

Environment

Nest version: 7.4.4
 
For Tooling issues:
- Node version: 12
- Platform:  Linux

Others:
N/A

UPD: Code formatting.

[jmcdo29]

Duplicate of #789

[tooleks]

@jmcdo29

As far as I understand the proposed solution is to not have classes with the same name in the same module. Even with that approach (with two different modules for two classes) there is the same issue when theese classes ment to be exported by each of the module and imported into the another one.

Is there some technical reason why the class reference can't be used as a dependency injection token for the class?

[jmcdo29]

A dependency injection token must be made to save the token to the instance in Nest's container system. What kind of token would you suggest, if not the name of the class. I suppose Symbol(name) may be usable, as Symbols are unique

[tooleks]

I was thinking about using a class reference itself, if not provided other token.

REPL: https://repl.it/@tooleks/EnormousUpbeatObjectmodel

// index.ts

import { AService as A1Service } from './a-1.service';
import { AService as A2Service } from './a-2.service';

type ContainerKey = string | symbol | any;
type ContainerValue = any;

const container = new Map<ContainerKey, ContainerValue>();

container.set(A1Service, A1Service);
container.set(A2Service, A2Service);

console.log(container.get(A1Service) === container.get(A1Service)); // true
console.log(container.get(A2Service) === container.get(A2Service)); // true
console.log(container.get(A1Service) === container.get(A2Service)); // false

// a-1.service.ts

export class AService {}

// a-2.service.ts

export class AService {}

[jmcdo29]

Hmm, looks like it should work as expected. Works with WeakMap too, which is a lot of what the internals are using in places. @kamilmysliwiec where all would this need to be updated if we are to implement this?

[tooleks]

@jmcdo29

I've prepared an experimental PR with the changes. Any comments would be appreciated.

[kamilmysliwiec]

We have used class references in the past but we've eventually migrated towards a string representation because it made the DI system somewhat more flexible. Using class names allows us to, for example, inject a Reflector class (from core package) into a ClassSerializerInterceptor (from common package) without importing a class reference which in turn would create a circular dependency between the official packages. There are certain edge-cases in which this is valuable while using the same class names (within a single module) is considered a bad practice either way

[tooleks]

@kamilmysliwiec

I see, thanks for the details.

The biggest issue for me is that you're not always has a control over all used modules yet it can lead to an unexpected behavior. See the more real-world example below and think of console-logger and file-logger as third-party packages. LoggerService classes are located in separate modules yet you can't import and use them both.

REPL: https://repl.it/@tooleks/HugeMysteriousCleaninstall

// ./index.ts

import { NestFactory } from '@nestjs/core';
import { ExampleModule } from './example.module';


async function bootstrap() {
  await NestFactory.createApplicationContext(ExampleModule);
}

bootstrap();

// ./example.module.ts

import { Inject, Module } from '@nestjs/common';
import { ConsoleLoggerModule } from './console-logger/console-logger.module';
import { LoggerService as ConsoleLogger } from './console-logger/logger.service';
import { FileLoggerModule } from './file-logger/file-logger.module';
import { LoggerService as FileLogger } from './file-logger/logger.service';

@Module({
  imports: [ConsoleLoggerModule, FileLoggerModule],
})
export class ExampleModule {
  constructor(@Inject(FileLogger) private readonly logger: FileLogger) {}

  onModuleInit(): void {
    console.log(this.logger.moduleName); // expected" "file-logger", actual: "console-logger"
  }
}

// ./console-logger/console-logger.module.ts

import { Module } from '@nestjs/common';
import { LoggerService } from './logger.service';

@Module({ 
  providers: [LoggerService],
  exports: [LoggerService],
})
export class ConsoleLoggerModule {}

// ./console-logger/logger.service.ts

export class LoggerService {
  public readonly moduleName = 'console-logger';
}

// ./file-logger/file-logger.module.ts

import { Module } from '@nestjs/common';
import { LoggerService } from './logger.service';

@Module({ 
  providers: [LoggerService],
  exports: [LoggerService],
})
export class FileLoggerModule {}

// ./file-logger/logger.service.ts

export class LoggerService {
  public readonly moduleName = 'file-logger';
}

Anyway, I'll try to play around with the source code and see whether it's possible to overcome the case that you've described.

[svvac]

I'm struggling with this too. It gets especially painful when several modules use very common names such as ConfigService. Another pain point is that you just can't minify your code because of this. Also, I'm pretty sure that this approach creates collisions with string tokens when providing both ConfigService and "ConfigService".

I think the issue with circular deps between core modules is a moot point, as one can always use some shared string token to break the cycle. You can also have your cake and eat it too by using useExisting to alias the string to the ref.

---

If using class refs is not desirable (please consider it though...), an alternative approach could be to define a well-known symbol that could be put on a class that would define its injection token if present or e.g. passed to @Injectable() (and if not fallback to old Function.name based injector).

@Injectable()
export class MyService {
    public static readonly [PROVIDER_TOKEN]: string | symbol = Symbol();
}

// or maybe

@Injectable(Symbol())
export class MyService {}

But this would really be suboptimal since it requires opt-in and would not allow to handle name collisions on third party modules.

[kamilmysliwiec]

Let's track this here #6141