deepkit / deepkit-framework

A new full-featured and high-performance TypeScript framework
https://deepkit.io/
MIT License
3.18k stars 121 forks source link

[type] [di]: DependenciesUnmetError, but they are met #496

Closed alpharder closed 9 months ago

alpharder commented 10 months ago

This issue is very weird. When I use promises in provided class of injectable interface, the whole DI crashes:

/Users/alpharder/dev/var/deepkit-repros/node_modules/@deepkit/injector/src/injector.ts:716
            throw new DependenciesUnmetError(
                  ^
DependenciesUnmetError: Undefined dependency "cms_svc: CmsService" of CmsHttpController(?). Type has no provider in scope http.

I tried to atomize reproducible code, here's single-file app.ts repro:

import {App, createModule} from '@deepkit/app';
import { FrameworkModule } from '@deepkit/framework';
import { Logger, JSONTransport } from '@deepkit/logger';

import { AppConfig } from './src/app/config';
import {http} from "@deepkit/http";
import {provide} from "@deepkit/injector";
import {readFileSync} from "node:fs";

export type ObjectTypeDefinitionJsonMetaSchema = {
    foo: string;
};

export type ObjectType = {
    title: string;
    jsonSchema: object;
};
export interface ICmsDomainEntityRepository {
    createObjectType(objectType: ObjectType): Promise<void>;
}

export class CmsDomainEntityRepositoryImpl
    implements ICmsDomainEntityRepository
{
    createObjectType(objectType: ObjectType): Promise<void> {
        return Promise.resolve(undefined);
    }
}

export type IObjectTypeJsonMetaSchemasProvider = {
    getObjectTypeDefinitionJsonMetaSchema(): Promise<ObjectTypeDefinitionJsonMetaSchema>;
};

export class ObjectTypeJsonMetaSchemasProviderImpl
    implements IObjectTypeJsonMetaSchemasProvider
{
    protected readonly objectTypeDefinitionJsonMetaSchema: ObjectTypeDefinitionJsonMetaSchema = {foo: 'bar'};

    constructor() {

    }

    getObjectTypeDefinitionJsonMetaSchema(): Promise<ObjectTypeDefinitionJsonMetaSchema> {
        return Promise.resolve(this.objectTypeDefinitionJsonMetaSchema);
    }
}

export class CmsService {
    constructor(
        protected readonly repo: ICmsDomainEntityRepository,
        protected readonly object_types_json_meta_schemas_provider: IObjectTypeJsonMetaSchemasProvider,
    ) {}

    async getObjectTypeDefinitionJsonMetaSchema(): Promise<ObjectTypeDefinitionJsonMetaSchema> {
        return this.object_types_json_meta_schemas_provider.getObjectTypeDefinitionJsonMetaSchema();
    }
}

@http.controller('/cms')
export class CmsHttpController {
    constructor(protected readonly cms_svc: CmsService) {
    }

    @http.GET('/object-types-json-meta-schema')
    async getObjectTypeDefinitionJsonMetaSchema(): Promise<ObjectTypeDefinitionJsonMetaSchema> {
        return await this.cms_svc.getObjectTypeDefinitionJsonMetaSchema();
    }
}

export class CmsModule extends createModule({
    controllers: [CmsHttpController],
    providers: [
        CmsService,

        provide<ICmsDomainEntityRepository>({
            useClass: CmsDomainEntityRepositoryImpl,
        }),

        provide<IObjectTypeJsonMetaSchemasProvider>({
            useClass: ObjectTypeJsonMetaSchemasProviderImpl,
        }),
    ],
}) {}

new App({
    config: AppConfig,
    controllers: [],
    providers: [],
    imports: [new FrameworkModule({ debug: true }), new CmsModule()]
})
    .loadConfigFromEnv({ envFilePath: ['production.env', '.env'] })
    .setup((module, config: AppConfig) => {
        if (config.environment === 'production') {
            //enable logging JSON messages instead of formatted strings
            module.setupGlobalProvider<Logger>().setTransport([new JSONTransport]);

            //disable debugging
            module.getImportedModuleByClass(FrameworkModule).configure({debug: false});
        }
    })
    .run();

But when we avoid promises here there's no error:

...
export type IObjectTypeJsonMetaSchemasProvider = {
    getObjectTypeDefinitionJsonMetaSchema(): ObjectTypeDefinitionJsonMetaSchema; // it was wrapped in promise before
};
...
   // it returned promise before
    getObjectTypeDefinitionJsonMetaSchema(): ObjectTypeDefinitionJsonMetaSchema {
        return this.objectTypeDefinitionJsonMetaSchema;
    }
...
alpharder commented 10 months ago

Please note that this interface+impl do use Promises and it doesn't crash for them:

export interface ICmsDomainEntityRepository {
    createObjectType(objectType: ObjectType): Promise<void>;
}

export class CmsDomainEntityRepositoryImpl
    implements ICmsDomainEntityRepository
{
    createObjectType(objectType: ObjectType): Promise<void> {
        return Promise.resolve(undefined);
    }
}
marcj commented 10 months ago

The problem here is that CmsService and IObjectTypeJsonMetaSchemasProvider share the same interface (they are compatible). So when the DI container handles IObjectTypeJsonMetaSchemasProvider it goes through all already known providers and sees CmsService being the same interface and thinks you want to override it. It then replaces its token with the interface type. We have to either accept this behaviour as well defined or find another behaviour. If you request injector.get(CmsService) it currently errors as it uses the CmsService class reference as token to resolve the provider, this could be changed to use it's computed interface instead. That would change the behaviour only in a way that it doesn't error anymore, but it returns no longer CmsService but whatever you have registered as IObjectTypeJsonMetaSchemasProvider, so probably still unexpected behaviour.

marcj commented 10 months ago

We have a way to override providers to make modules extendable. This is based on types, so for each provider we look if there is already one defined that is compatible and assume you want to replace it. Let us assume this behaviour can be disabled somehow, now we have another question to answer:

If you have two services registered, one class based and one via provide<Interface>, and both share the same public interface (they are structurally the same/compatible), so are compatible, what service do you expect to get when you request its interface? The DI container would have two compatible services.

alpharder commented 10 months ago

and thinks you want to override it

&

This is based on types, so for each provider we look if there is already one defined that is compatible and assume you want to replace it

From my developer experience perspective I find this kinda weird.

In other words, we have structural DI rather than nominal, right?

I never saw anything like that (which isn't an indicator of anything), but at the same time I believe it's a combination of practically bad ideas:

We have a way to override providers to make modules extendable

I'm not sure whether I'm fully seeing some related pitfalls or edge-case scenarios, but on paper I think this can be achieved in a way, that creates less problems.

If you have two services registered, one class based and one via provide, and both share the same public interface (they are structurally the same/compatible), so are compatible, what service do you expect to get when you request its interface

So let me represent this in code in order to avoid any misinterpretations. Assuming we have:

class Foo {
   bar: string;
}

interface StructurallySimilarToFoo {
   bar: string;
}

// and register those as injectable, for example at app module
createModule({
  providers: [
   Foo,
   provide<StructurallySimilarToFoo>({
     useValue: {bar: ''}
   })
 ]
})

In this scenario:

I tread both StructurallySimilarToFoo and Foo as injection tokens. They are different entities, and are not meant to be interchangeable. I expect their providers to be explicitly declared. I expect DI to use nominal injection tokens.

In case I need to override a provider, I should explicitly do that:

createModule({
  providers: [
   Foo,
   provide<StructurallySimilarToFoo>({
     useValue: {bar: ''}
   }),

   // while this provider is registered, I expect it to be used for anything that requests injector.get(StructurallySimilarToFoo)
   provide<StructurallySimilarToFoo>({
     useValue: {bar: 'something else'}
   }),
 ]
})
alpharder commented 10 months ago

I cannot understand what problems structural DI is trying to solve, which cannot be solved in regular, nominal DI.

marcj commented 10 months ago

They are different entities, and are not meant to be interchangeable

No, that's not how TypeScript works. For TypeScript they are the same. Do you think this is a problem in Typescript, too?

We aligned the DI to how structural typings in TypeScript works. We have with that advantages by not having nominal interfaces and we have drawbacks as they don't work like we know from PHP & co, but on the other side TypeScript devs are aware that its a structural type system. I think this is a great feature. Would be unexpected to suddenly have everything nominal in the DI container.

If you want to use nominal types instead, we can make it possible. The question is only how. Maybe something like provideNominal<T>(), provideUnique, or a type wrapped, provide<Nominal<T>>() or something like that.

marcj commented 10 months ago

Or we can turn it around and make it nominal per default and structural explicitely. No sure yet, we have to think about it and go through various use cases and see how it feels.

alpharder commented 10 months ago

No, I don't think that having structural type system is a problem for TypeScript.

However, I believe that DI and language's type system are different layers of abstraction, pursuing different goals, and thus cannot be compared directly.

They are different entities, and are not meant to be interchangeable.

No, that's not how TypeScript works. For TypeScript they are the same.

I was talking about injection tokens, each of which are basically a combination of slug and type. So it has nothing to do with TypeScript, that's another layer of abstraction – injection tokens are not interchangeable.

We aligned the DI to how structural typings in TypeScript works

I'm not sure what specific goals were pursued when this decision was made (except for aligning with TS just for the sake of aligning itself). I still don't see any practical benefits of having DI behaving like it currently does.

As demonstrated above, there are real-world practical issues with this approach.

I suppose any other C#, Java or even TS dev (Angular, Nest) would expect DI to function in an explicit way, without implicit, under-the-hood overriding of injection tokens bases on structural compatibility.

I have a strong feeling that we're probably discussing different things, maybe there's some kind of misunderstanding.

I've re-read all of your messages in this thread. I might probably understand what you've meant, but I still don't understand the implicit overriding by duck typing when declaring providers. Please note that I'm not standing against this scenario:

// these two are structurally compatible
class InjectableFoo { val: string }
class InjectableBar { val: string }

function injectablesConsumer(x: InjectableFoo) {
    // DI CONSUMER should be STRUCTURAL of course, i.e. it doesn't really care what "x" really is until it's compatible with "InjectableFoo" interface
}

// so this should be possible in order to inject something else instead of "InjectableFoo"
// "NOMINAL" injection
createModule({
  providers: [
   // original
   InjectableFoo,

   // EXPLICIT override of "InjectableFoo" token provider using "InjectableBar" class instance
   provide<InjectableFoo>({
     useClass: InjectableBar
   }),
 ]
})

This is what I'm arguing about, it should not work like this:

// "STRUCTURAL" injection

class InjectableFoo { val: string }

interface ICompletelyUnrelatedButCompatible { val: string }

class CompletelyUnrelatedButCompatibleImplementation implements ICompletelyUnrelatedButCompatible {
    val: string
}

createModule({
  providers: [
   InjectableFoo,

   // IMPLICIT, unexpected override under the hood. I just wanted to define another injection token which is "ICompletelyUnrelatedButCompatible", and not override "InjectableFoo"
   provide<ICompletelyUnrelatedButCompatible>({ useClass: ICompletelyUnrelatedButCompatible })

  // if I wanted to actually override InjectableFoo, I'd write
  provide<InjectableFoo>(...)
 ]
})

In other words, I'm standing for structural consumption of injectables and nominal (token-based) provider declaration of injectables.

Let me metaphor how it currently works to demonstrate its weakness. I'll be using variables instead of injection tokens:

type Foo = { val: string }

let foo : Foo = { val: 'one' };

let bar: {val: string } = {val: 'another' } // at this point Deepkit's DI would decide to override "foo" variable value instead, because it's structurally compatible, despite the fact I've explicitly assigned this value to "bar' variable

function fn() {
   console.log(foo.val) // "another", why??
}

fn();
marcj commented 10 months ago

I never saw anything like that

That doesn't matter. Deepkit is the first DI container that uses full TypeScript capabilities. Of course you have never seen anything like that. You have also never seen a full fledged runtime type system based on TypeScript. The fact that you never have seen anything like that has no weight whatever. Or does it only have weight if it is an aspect you don't like? Rhetorical question. Does that mean that the issue found here is by design and we should ignore it? No, we will fix it and make it so it feels better of course.

but at the same time I believe it's a combination of practically bad ideas I'm not sure what specific goals were pursued when this decision was made (except for aligning with TS just for the sake of aligning itself)

Temper your voice and stop these disrespectful speculations.

In other words, I'm standing for structural consumption of injectables and nominal (token-based) provider declaration of injectables.

Let us assume we take your proposal and implement it like that, so that each type declaration is treated as if they were nominal. This means A and B are different:

interface A {
   hello(): string;
}

interface B {
    hello(): string;
}

createModule({
  providers: [
      provide<A>({hello: () => 'World'}),
      provide<B>({hello: () => 'Nope'}),
  ]
}

What does the following consumer get? A or B? You said consumption is structural. This means the DI container has two providers to choose from. Probably picks the first found. Or the last found? Both need explanation and can result in getting unexpected results. You bullet list of "practically bad ideas" apply to this as well.

class Consumer {
    constructor(dep: A) {}
}

You see, it is not as trivial as you are trying to make it seem. Everything has a tradeoff, you have to carefully choose. And usually your first design is not the most ideal. That's why you iterate on it and get feedback, then adjust where necessary.

What about this one? Same thing as above probably.

interface MyDep {
   hello(): string;
}

class Consumer {
    constructor(dep: MyDep) {}
}

What about this one? Same thing as above probably.

class Consumer {
    constructor(dep: {hello(): string) {}
}

If they are treated as nominal types, then how do we override A or B with a new provider? Do we have to get a real reference to A or B? That means we tightly couple our code to whatever package we want to override a type from.

import { A } from 'someDependency';

new App({
   providers: [
      provide<A>({hello: () => 'overridden'})
   ]
   imports: [MyModule]
})

Saying "Yes, I expect this to work like that (nominal interfaces) since I'm used to it from other language" is not an argument. We work in TypeScript where stuff is structural. This is a powerful feature that differentiate TypeScript from other languages, which we do not throw away blindly just because "other DI container in other languages work differently". We do something here that has never been done, so we are in uncharted territory. We will see if it is better or worse. What we can do is to solve upcoming issues like the one you described and apply TypeScript's philosophy (that made it so successful) also in runtime. What we don't do is to say everything is bad, and that it works in other languages different, and that we should stick to how other languages do it.


An issue I see is that when a class is registered, we could treat this class as nominal token (contrary to what TypeScript does) and don't override by an object literal. Is this a perfect solution to the issue described above? Unlikely, but it fixed the issue for now and is an easy to explain detail about the DI Container: Use a class for nominal providers and an interface for structural providers.

When an object literal (interface) is registered, we treat it structurally. And if you decide to declare two providers providing the exact same shape, then we treat it as one and override. We can make the matching smarter than it currently is by for example see what providers matches "the best" (not like now, what providers matches the last, even if only one method fits). The best would mean that the more methods and the less excessive methods match, the more points.

interface Connection {
    write(data: Uint8Array): void;
    clientIp: string;
}

interface Writer {
    write(data: Uint8Array): void;
}

const app = new App({
   providers: [
       provide<Writer>({...}),
       provide<Connection>({...}),
   ]
});

//Better match: Returns Writer, not Connection although Connection matches structurally and is the last known provider
app.get<{writer(data: Uint8Array): void}>();

If you want to change this structural matching, you work against TypeScript and should define the interface as nominal by using solutions like type branding or primitive injector tokens (like a string or unique symbol as token).

alpharder commented 10 months ago

First of all, let me clarify: my goal of participating in this discussion is to contribute by providing feedback and thoughts.

My intent was not in that some of my messages would be interpreted as "disrespectful speculations".

I'm sorry you felt uncomfortable from what I said.

I'm not trying to say "yo, Deepkit sucks".

I'm trying to say "hey, here's how it could be improved, and here's why".

Please let me know if this is not what you currently expect, and I'll focus on something else.

My strong belief is that healthy, open discussion of a feedback (which can be negative too) is crucial for evolving.


That doesn't matter.

Yes, I've mentioned that – my full quote was:

I never saw anything like that (which isn't an indicator of anything)

You've interpreted that phrase by assuming my lack of critical thinking and inability to adapt to unusual concepts:

Saying "Yes, I expect this to work like that (nominal interfaces) since I'm used to it from other language" is not an argument.

However that could be interpreted in a following way:

DI in Deepkit works nothing similar to what your potential userbase is used to, and that could be handled somehow – by either documenting this difference and/or by softening the entry via providing a way to emulate something everyone is used to.

Next, DI is a well-described, both language agnostic and type system agnostic concept. It solves certain problems in a certain way. And I expect it to be able to solve the same problems in the same way. Please read the detailed explanation below where I explain the importance of a context of use.

We could name Deepkit's DI as "type injector" instead of "DI container/injector" and the way it works would make a perfect sense. But from what I understood we're specifically discussing its usage in IoC / DI scenario.

Does that mean that the issue found here is by design and we should ignore it? No, we will fix it and make it so it feels better of course.

Awesome, happy to hear that.


What does the following consumer get? A or B?

I'd say "A".

Why: in my mental model, DI injection token is a combination of slug and type, not the type itself. Type without slug lacks the context of use. Slug provides the context information. When used in a DI, class, interface and type names play the role of a slug, providing the context information.

consider this consumer:

class DbConnection {
   constructor(connection_string: string, log_prefix: string);
}

Both arguments have the same type, but different context of use. And current implementation ignores the context, solely relying on types. Types themselves make no sense in terms of DI. Context of use together with type – do.

Let's begin with primitive types first. Here's how this is usually solved for primitive types:

class DbConnection {
   constructor(@Inject('DB_DSN') connection_string: string, @Inject('LOG_PREFIX') log_prefix: string);
}

We added context of use, unambiguous identifiers – injection token slugs. Now we have two injection tokens – each of which consists of a slug and a type, providing DI container with the information of what can be injected into the consumer.

To me it makes no practical sense to just register an abstract string within a DI container, but it makes sense to register a DB_DSN, which should be compatible with string.

For this scenario, we could leverage runtime types system in a following way, which is both practical for DI purposes and idiomatic to TypeScript:

// replace const injection token slugs with named types:
type DB_CONN = string;
type LOG_PREFIX = string;

class DbConnection {
   constructor(connection_string: DB_CONN, log_prefix: LOG_PREFIX);
}

Another similar example could be:

class StreamSplitter {
   constructor(input: ReadableStream, output_l: WritableStream, output_r: WritableStream);
}

For StreamSplitter.constructor() its argument type definitions are enough to function properly, because they're tightly coupled to argument names (different contexts of use), but these types alone are not meaningful for DI injector.

Solution without Deepkit would involve decorators again, but using named types, whose names would be treated as injection tokens, would remove context of use unambiguity.

Summary of my idea:

Deepkit's DI can and should be idiomatic to TypeScript, as well as leveraging its unique runtime type capabilities. However, it should fulfill the necessity to inject dependencies, rather than types. And dependency !== type. It is a value of a certain type (compatible with a certain type) suitable for a specific context of use. Type without context of use is meaningless and is not safely interchangeable. Context of use is declared via injection token slug. Deepkit can use class, interface and type names as injection slugs, because it has an access to them during runtime.

This means the DI container has two providers to choose from.

No, because the provider declaration is nominal. Injection tokens should be unique within DI injector's scope.

Given this code, DI container should only have a single provider declaration for A token:

createModule({
  providers: [
      provide<A>({hello: () => 'World'}),
      provide<B>({hello: () => 'Nope'}),
  ]
}

What about this one? Same thing as above probably.

interface MyDep { hello(): string; }

class Consumer { constructor(dep: MyDep) {} }

I don't see any pitfalls in this code according to what I've explained above.

What about this one? Same thing as above probably.

class Consumer { constructor(dep: {hello(): string) {} }

{hello(): string } has no practical meaning for DI purposes because it doesn't define boundaries and context of use. It should be coupled with a context of use, which – again – can be achieved with injection tokens – either string consts, or, with runtime types – with named types.

If they are treated as nominal types, then how do we override A or B with a new provider?

Use injection token slugs, which I've described above.

Do we have to get a real reference to A or B? That means we tightly couple our code to whatever package we want to override a type from.

No, following the IoC idea, you should depend on an interface rather on a specific implementation. And that interface should be defined in your code, not externally:

import { A } from 'someDependency';

type MyApplicationExpectsThisKindOfStuff = typeof A;

// but ideally you want to avoid this as well, meaning:
// type MyApplicationExpectsThisKindOfStuff = { hello: () => string };

new App({
   providers: [
      provide<MyApplicationExpectsThisKindOfStuff>(A),
      provide<MyApplicationExpectsThisKindOfStuff>({hello: () => 'overriden'}),
   ]
   imports: [MyModule]
})

Thanks for reading this.

My goal is not to convince you in anything or to argue just for sake of arguing.

My goal was to explain this concern of valuing DI's main idea of dependency injection.

marcj commented 10 months ago

Ok, cool.

Do we have to get a real reference to A or B? That means we tightly couple our code to whatever package we want to override a type from.

No, following the IoC idea, you should depend on an interface rather on a specific implementation. And that interface should be defined in your code, not externally:

but A is a type in my example, not a concrete implementation. So typeof A is invalid syntax as well as passing A as value in provide<...>(A). For me the question is then still open: When making every type nominal in the context of the DI container, are we only able to override certain tokens when we have a direct reference to a type? (in this case A). After thinking about it, I guess it's fine. Feels weird to have everything as nominal though. For use cases where we want to override a token that satisfies a given interface, we can provide an additional way of declaring that type, e.g. via overrideProviderFor<Interface>({...}).

I don't see any pitfalls in this code according to what I've explained above.

I think it is, since we defined an abstract type as dependency. When we would have two registered providers that both satisfy our dependency, what provider do we get? There is no real reference to the type A or B here, so we can not resolve nominal types.

To me it makes no practical sense to just register an abstract string within a DI container, but it makes sense to register a DB_DSN, which should be compatible with string.

To me both don't make sense. Registering a string token and then use a custom Injector Token as dependency (via e.g. decorator) is in my eyes abusing the DI container.

class DbConnection {
   constructor(connection_string: DB_CONN, log_prefix: LOG_PREFIX);
}

These primitive values are no services but static values, so how we solve that in Deepkit is that we introduce the "context" via index access operator that has as container a known reference, e.g. configuration class.

class DbConfig {
    connectionString: string;
    logPrefix: string;
}

class DbConnection {
   constructor(connection_string: DbConfig['connectionString'], log_prefix: DbConfig['logPrefix']);
}

This works fine and introduces context as you would say. However, the approach via nominal types:

type DB_CONN = string;
type LOG_PREFIX = string;

could also work when we define that all types are treated as nominal in the DI container. It's not as flexible though since overriding via a configuration loader is not easily possible I assume. Currently with index access references to a config class, this works very well.

{hello(): string } has no practical meaning for DI purposes because it doesn't define boundaries and context of use. It should be coupled with a context of use, which – again – can be achieved with injection tokens – either string consts, or, with runtime types – with named types.

Context is not necessary for Dependency Injection. You just say that you want an object of this type, and the user is responsible for providing/injecting the right one. Sure, in the context of auto-wiring in a DI container this leads to a problem since the DI container itself doesn't know the context when multiple providers match, so it has to move that responsibility to the developer.

When we say everything is nominal and a type is defined as dependency that has no nominal candidate in the DI container, we can still switch to the strategy of finding whatever provider is known that satisfies the given dependency type. If a type is not unique we have to either throw an error so that the developer is responsible to resolve it (by e.g. defining a provider factory and manually injects it), or we simply inject the last matching provider.

No, following the IoC idea, you should depend on an interface rather on a specific implementation

That has nothing to do with IoC. What you mean is probably Dependency Inversion.

marcj commented 9 months ago

This is fixed in master via https://github.com/deepkit/deepkit-framework/commit/92315f91ed4295d77b8c7bb693e8cbf8d5d32b23