Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEAT] Add support for optional dependencies #1694

Open
derevnjuk opened this issue Dec 28, 2021 · 4 comments
Open

[FEAT] Add support for optional dependencies #1694

derevnjuk opened this issue Dec 28, 2021 · 4 comments

Comments

@derevnjuk
Copy link
Contributor

derevnjuk commented Dec 28, 2021

Informations

As a user, I would like to use the empty constructor new Foo(), to avoid registering noop dependencies if they are not required.

The new parameter decorator can be introduced to be used on constructor parameters, which marks the parameter as optional. At the same time, the IoC container should provide null or undefined if the dependency is not found.

interface Bar {
  // ...
}

const Bar: unique symbol = Symbol('Bar');

@Injectable()
class Foo {
  constructor(@Optional() @Inject(Bar) private readonly bar?: Bar) {}
}

Currently, while resolving an instance, the IoC container fails with the following error:

    TypeError:

   487 |   static getParamTypes(targetPrototype: any, propertyKey?: string | symbol): any[] {
   488 |     console.log(DESIGN_PARAM_TYPES, targetPrototype, propertyKey);
 > 489 |     return Reflect.getMetadata(DESIGN_PARAM_TYPES, targetPrototype, propertyKey!) || [];
       |                    ^
   490 |   }

However, there is a workaround. You can register a value provider manually like this:

registerProvider({provide: Bar, useValue: null});
@Romakita
Copy link
Collaborator

Hello @derevnjuk

Yes the DI isn’t designed to have optional dependencies. I don’t know what is the exact usecase, because dependency always exist if it’s an injected class (tokenProvider===class). And circular dependency musn’t be injected like this (by constructor) and must use @Inject decorator for that (on property).

The error is a symptom of a circular dependencies, because type isn’t stored by typescript.

Adding optional decorator is strange for me. In your example, the bar will never be injected… so what is the interest? That I don’t see ^^

I think isn’t a huge work to solve this but I want to be sure there is a real usecase.

See you ;)

@Romakita Romakita added this to To do in Global Board Dec 29, 2021
@Romakita Romakita moved this from To do to Spec in Global Board Dec 29, 2021
@derevnjuk
Copy link
Contributor Author

derevnjuk commented Dec 29, 2021

must use @Inject decorator for that (on property).

That's not exactly true. Even if you use a decorator on the property, it will raise the same exception.

The error is a symptom of a circular dependencies, because type isn’t stored by typescript.

In this particular case, it has nothing to do with circular dependencies. Once you declare a dependency, InjectorService will try to resolve it using class provider (i.e useClass) by default.

deps = deps || Metadata.getParamTypes(provider.useClass);

Since you imply that a dependency is optional, you don't register a provider. As a result, provider.useClass is undefined. See the example below (inspired by @tsed/mikro-orm package):

// ./src/services/RetryStrategy.ts
export interface RetryStrategy {
  acquire<T extends (...args: unknown[]) => unknown>(task: T): Promise<ReturnType<T>>;
}

export const RetryStrategy: unique symbol = Symbol("RetryStrategy");

// ./src/interceptors/TransactionalInterceptor.ts
@Interceptor()
export class TransactionalInterceptor implements InterceptorMethods {
  @Inject(RetryStrategy)
  private readonly retryStrategy?: RetryStrategy;

  constructor(
    @Inject() private readonly registry: MikroOrmRegistry,
    @Inject() private readonly context: DBContext,
    @Inject() private readonly logger: Logger
  ) {}

  // ...
}

It provides a user the ability to register a custom implementation of RetryStrategy when it is necessary.

export class ExponentialBackoff implements RetryStrategy {
  // ...
}

registerProvider({
  provide: RetryStrategy,
  useClass: ExponentialBackoff
});

Ofc, we can declare a noop implementation and let the user override it, but it doesn't look like the right solution from the design perspective.

// ./src/services/RetryStrategy.ts
export interface RetryStrategy {
  acquire<T extends (...args: unknown[]) => unknown>(task: T): Promise<ReturnType<T>>;
}

// ./src/services/RetryStrategy.ts
export class NoopRetryStrategy implements RetryStrategy {
  // ...
} 

// ./src/interceptors/TransactionalInterceptor.ts
@Interceptor()
export class TransactionalInterceptor implements InterceptorMethods {
  @Inject()
  private readonly retryStrategy: NoopRetryStrategy;

  constructor(
    @Inject() private readonly registry: MikroOrmRegistry,
    @Inject() private readonly context: DBContext,
    @Inject() private readonly logger: Logger
  ) {}

  // ...
}

@OverrideProvider(NoopRetryStrategy)
export class ExponentialBackoff implements RetryStrategy {
  // ...
}

@Romakita
Copy link
Collaborator

Haaaaa totally clear :D Thanks for the explanation ^^. I'm ok to add this feature to the backlog!

@Romakita Romakita moved this from Spec to To do in Global Board Dec 30, 2021
@stale
Copy link

stale bot commented May 31, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: To do
Development

No branches or pull requests

2 participants