suites-dev / suites

A testing meta-framework that simplifies unit testing by reducing boilerplate code and enabling developers to build comprehensive and reliable test suites
https://suites.dev
Apache License 2.0
366 stars 12 forks source link

Abstract and Function support for `InjectableIdentifier` #288

Closed SnowMarble closed 7 months ago

SnowMarble commented 7 months ago

Is there an existing issue for this?

Current behavior

Currently, identifier of unit reference does not support Abstract class and Function. Can you consider adding those to InjectableIdentifier?

Abstract type from @nestjs/common

interface Abstract<T> extends Function {
    prototype: T;
}

Minimum reproduction code

I believe reproduction code is not needed for this issue.

Steps to reproduce

Passing abstract class or function causes an error.

Expected behavior

Ignoring type error, it seems that passing them still works. I think only adding the type would be fine.

Automock version

2.1.0

Node.js version

Bun 1.0.31

In which operating systems have you tested?

Other

No response

omermorad commented 7 months ago

Hello @SnowMarble,

You can easily solve it by casting the type of the subject dependency, here is an example:

your-class.ts

export abstract class UserAbsSvc { ... }

your-unit.spec.ts

import { Type } from '@automock/types'; // Could be also from `@nestjs/common`

const { unit, unitRef } = TestBed.create(...).compile();
const userSvc = unitRef.get(UserAbsSvc as Type<UserAbsSvc>);

Let me know if it worked!

SnowMarble commented 7 months ago

@omermorad I appreciate your reply. Type casting definitely works. But I think we should be able to use abstract classes and functions without type casting by default. Because this is not an uncommon use case. What do you think about it?

omermorad commented 7 months ago

I see, that's actually an interesting scenario, I think it can work. Would you be able to supply some more context regarding the code?

Does the class you are trying to get() from the unitRef extends from an abstract class? Or is it the abstract class itself that being resolved from the unit reference?

Suppose this class:

export abstract class UserAbsSvc { ... }

If the following is the case here - how can abstract class act as a dependency of a another class? Or am I missing something?

export class ConcreteClass {
  public constructor(private readonly dep: UserAbsSvc) { ... }
}
SnowMarble commented 7 months ago

I'm working on Nestjs. I use an abstract class as a provider, and bind it with an implementation class.

// some.port.ts
export abstract class SomePort {
  public abstract foo(): void
}
// some.adapter.ts
export class SomeAdapter implements SomePort {
  foo() {
    // do something
  }
}
// some.module.ts
@Module({
  providers: [
    {
      provide: SomePort,
      useClass: SomeAdapter
    }
  ]
})
export class SomeModule {}

This can make me use abstract class as an injection token. This is similar to specifying a string as a provider. I'm sure you already know it.

So, answering your question, what I want to get from get() is not the abstract class. Providing target is just different.

For more information, you can check out the nestjs docs: https://docs.nestjs.com/fundamentals/custom-providers#class-providers-useclass

omermorad commented 7 months ago

Alright, I understand now.

It may be beneficial to take a few factors into account when considering this approach:

Typically, abstract classes are used as blueprints for concrete implementations rather than as identifiers themselves. This is because abstract classes are not intended to be directly instantiated. Instead, they serve as templates for subclasses. Unlike interfaces, which are cancled at runtime, abstract classes not.

When it comes to dependency injection and identifiers, it is more typical to utilize concrete classes or other forms of identifiers such as symbols or strings. These references offer a more precise and explicit indication of the dependencies being injected.

If you're interested in finding an alternative method to uniquely identify your dependencies for injection, considering a concrete class or a custom symbol or string token could be a more suitable option. This approach is more in line with the common patterns and conventions found in NestJS and other dependency injection frameworks. Take a look at TSyringe, for instance, or even libraries from various programming languages. Using Java Springboot as an example - it is not possible to use interfaces as identifiers. Instead, you need to provide the DI engine with the specific class you want to resolve.

Based on this, it seems more logical to use casting with Type rather than including the Abstract type in the Identifier type.

What do you think? 🦾

SnowMarble commented 7 months ago

I agree with your point. However, personally, I prefer avoiding @Inject decorator and managing additional DI tokens. After doing some research, I found that people have used abstract classes for the same reason to me. If we use abstract classes as implemented, we can use them like an interface. Using type cast might be enough to use, but I think supporting abstract classes is also worth to consider. And, actually, it's not a big deal, you know - just a new type to be added. If you still think my suggestion is unnecessary, I respect your decision :)

By the way, I'm not a Java dev, but it seems @Autowire decorator allows interfaces as DI tokens.

omermorad commented 7 months ago

I spent some time looking into this. Let me break it down and share the conclusions :)

Consider the following scenario: you have an abstract class SomePort with various concrete implementations such as SomeAdapter and SomeOtherAdapter. This configuration is completely legal and is frequently used to ensure consistency across multiple implementations.

Consider a class SomeService that requires both SomeAdapter and SomeOtherAdapter:

@Injectable()
export class SomeService {
  constructor(
    private readonly someAdapter: SomeAdapter,
    private readonly someOtherAdapter: SomeOtherAdapter,
  ) { ... }
}

Alternatively, you may apply the dependency inversion concept and utilize the abstract type of the abstract class SomePort directly (rather than the concrete):

@Injectable()
export class SomeService {
  constructor(
    @Inject('SOME_ADAPTER') private readonly someAdapter: SomePort,
    @Inject('SOME_OTHER') private readonly someOtherAdapter: SomePort,
  ) { ... }
}

In both scenarios, Automock confronts a dependency resolution challenge. Because it acts at runtime and relies on given identifiers to find dependencies, it may fail to distinguish between numerous implementations of the same abstract class. As a result, attempting to resolve SomePort using unitRef.get(SomePort) may result in ambiguity.

This problem is similar to one addressed earlier (https://github.com/automock/automock/issues/67), emphasizing the significance of finding clear and unambiguous means to indicate dependencies, which is why I mentioned the string (or symbol identifiers).

I hope this clarifies everything!

SnowMarble commented 7 months ago

@omermorad I appreciate your opinion. I've never thought about multiple implementations of abstract classes. I gotta bring back the proposal when I can prevent these potential problems. Thanks :)