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

Support for async service factories #20

Open
jahudka opened this issue Mar 20, 2023 · 16 comments
Open

Support for async service factories #20

jahudka opened this issue Mar 20, 2023 · 16 comments

Comments

@jahudka
Copy link

jahudka commented Mar 20, 2023

Hi, this library looks like a dream come true to me, I hate decorator-based DI libraries with a passion and I've wanted to get around to writing a compile-time DI implementation for literally years (in fact I tried, several times, but always fell short). So first of all, thank you from the bottom of my heart for writing and releasing this library!

I'm using MikroORM in most of my backend NodeJS apps. It's a pretty neat ORM library, I like it a lot. But its initialisation is asynchronous - and so if I want to register it as a service in a DIC, I need to either initialise it first and then register the resolved instance (which means the ORM is always initialised, even when it's not needed), or I have to type the service as a promise for the correct type. But then I want to inject this service into other services. Either I could inject the typed promise (which the dependent services cannot await in their constructor, so they would have to store a reference to the promise instead of the actual resolved service, and await that promise each time they want to access the dependency), or I have to wrap all the dependent services in a promise as well and write an async factory function for each of those services, which would do something like return new DependentService(await container.get('async-service')). This is, in fact, what I'm doing right now, because it feels like the "cleanest" solution in terms of separating the actual services from the DI implementation or the fact that some services are initialised asynchronously.

My current DI implementation is basically a lot of handwritten factories which mostly do exactly what your DI library does at runtime, except it allows for services to be async. I'm not sure how the support for generics currently works in this library - from the linked issue it seems I can use generics, but maybe the generic part of the registration is ignored..? (based on the last comment from @cmidgley, which seems to suggest that stripping the type parameter is the intended behaviour?). So if this library does support generic services, then that would probably also mean promises for services (since a promise is just a generic from this point of view). Is that a correct assumption?

And, well, further on the topic, I think that one amazing feature this library could include would be support for async services out of the box - that is, I'd like to do something like this:

container.registerSingleton<Promise<ServiceOne>>(async () => createServiceOneSomehow());
container.registerSingleton<Promise<ServiceTwo>>();

class ServiceTwo {
  constructor(private readonly one: ServiceOne) {}
}

// the transformer can now detect that "ServiceOne" is wrapped in a promise, so it should be possible to generate factory code for "ServiceTwo" which awaits the container.get() call in order to autowire the unwrapped ServiceOne instance instead of the promise

Maybe the container.register* methods might not even need the type parameter wrapped in a promise - the transformer should be able to detect that the service is async based on either the passed-in factory function, or on the fact that the service depends on other services which are async. When one wants to access such a service, the transformer could detect that container.get<ServiceTwo>() is attempting to access an async service without specifying the Promise wrapper and throw an error (since this would break at runtime)..

I'm not sure if all of this is perhaps already supported - I can't find it anywhere in the docs or issues. If it is, then that's amazing! And if it's not: is this something you'd consider adding to the library? I would very much like to offer my help in case you would.

Thanks in any case!

@cmidgley
Copy link
Contributor

I also love this tool - works amazingly well. My two cents here is that what you propose won't work well as it would then require all calls to get to also be async (as any dependency might be an async factory) thereby making it a requirement that the entire program is basically async. Adding to the difficulty is that constructors themselves can't be async, so they could no longer call get conditionally (await not allowed). And I think that's just the tip of the iceberg, but that's just quickly off the top of my head.

The solution is to use a factory function (or class, but I find it cleaner to use a function). Your factory can then create the object with async, provide any additional properties, and off you go. That factory can in turn call other factories and use get for injections, so it remains pretty clean.

In theory, the DI compiler might provide helpers to create those factories, but I love the small footprint, and simplicity of this DI and personally would not recommend it getting bloated like almost every other DI out there.

@jahudka
Copy link
Author

jahudka commented Mar 20, 2023

@cmidgley I think the get() method doesn't necessarily have to be async itself - I think this could be either solved by providing a .getAsync() method and checking at compile time if the correct method is called based on whether the requested service is async, or the .createInstance() method which is used under the hood could detect async services from the registration record and delegate to an async version of the service creation workflow (so that await can be used). I think it wouldn't add too much bloat to the runtime part of the library and there is a growing list of use-cases in my head for services which might benefit from async initialisation - like services which depend on something loaded from a (non-TS) file, e.g. configuration, encryption keys and similar.

Another thing is that depending on how you use the DI container, I don't necessarily think that application code would need to be updated that much even if the .get() method was async - since most of the time you shouldn't need to call it directly - your services would be always injected with the resolved instances of dependencies, regardless of whether those dependencies require async initialisation or not - it's only explicit calls to .get() which would need to be awaited, and there shouldn't be that many of those in my opinion.

There's also another optimisation I think this might possibly unlock.. I imagine that using this library one would probably have something like a bootstrap.ts file, where there's a lot of imports, then a const container = new DIContainer(), then a bunch of container.register*() calls and then export { container }; at the end; and then in the application entrypoints one would simply import the container, get a single service from it and call a method on that service - e.g. get a CLI application service and run it, or get a HTTP server and start it and so on. That's what I'd do, anyway. Well, in such a scenario, imagine there's a lot of services - hundreds, perhaps. Then simply importing the boostrap.ts file would take quite a bit of time, since that file in turn needs to import all the services, even if it doesn't immediately instantiate them. Having the option of making services async would allow the compiler to optimise startup times by dynamically importing the service class only when it's being instantiated.

@jahudka
Copy link
Author

jahudka commented Mar 20, 2023

Lastly, sure, you couldn't use async .get() calls conditionally within service constructors. For me that's a valid tradeoff which would easily be solved by writing a factory which does the conditional .get() before instantiating the service itself - I don't see that many situations where I'd need that, so it appeals to me much more than having to write tens or hundreds of factories for all the async services - but that's my use-case and I get that it may not be the universal preference. But you could also argue that calling .get() conditionally from within a service constructor is not exactly clean - because it means that the service doesn't get all its dependencies injected externally - it now needs to know that a DI container exists and how it works - which means tighter coupling between the service and the DIC, which in my opinion is the opposite to what one wants to achieve with DI.

@cmidgley
Copy link
Contributor

I've not spent the time you have on this, so perhaps you have a solution path that I haven't yet seen.

I'm not in favor of the idea of having the code that instantiates the object tree to decide if it should use async or not (get and getAsync). It's not appropriate for that code to know the implementation of all objects that might be injected - that sounds like an anti-pattern to me. For me, this implies that for get` to support async construction, it must always be async ... thereby forcing a lot of code to support async, even if it doesn't otherwise need it.

You have to use get, at least at the start, as that is how all injection works. If your code just has a single object tree, then that call will load the entire dependency tree for all objects (to inject into all children objects) and at that point async could be eliminated. But I find it's common, in all nearly all my projects, that I need to use get a fair amount.

Perhaps my earlier use case of constructors doing conditional get calls wasn't the best use case (I'm not sure I've ever done that), but I do often need to build lists of things that are injected (in the constructor as well as in general non-async methods as well as in factory methods). If it is needed in the constructor, and yet get is async, then we have the two-step initialization anti-pattern where an init call is needed after construction before the object is ready. This would force us to use factories when otherwise we would not have needed them, and any code that needs to consume that factory would therefore also now be async. And speaking of factories, the same problem exists - now any factory that wants to inject dependencies along with custom properties must now be async.

But as I stated at the top, perhaps I misunderstand your idea. It would be great to have async construction (and for that matter, a way to pass properties along with injected objects into constructors without factories - which I think is much more doable given the code generation model) as long as these challenges of infecting async throughout the code and/or two-step initialization anti-patterns can be resolved.

@jahudka
Copy link
Author

jahudka commented Mar 21, 2023

Sorry, this will be longer 😅

I'm used to compile-time DI from the PHP world, where the way most frameworks work is that they compose a factory method for each service; at runtime, the container doesn't need to know anything about the dependencies of a service, it just calls the factory method, and that in turn calls container.get() to resolve the dependencies. This is perhaps useful to understand the way I'm thinking about this, even though this library works slightly differently - in practice both approaches achieve the same thing.

If you think about it this way, though, it's apparent that when creating service A, you only need to know whether its own initialisation needs to be async or not; within the code which resolves its dependencies, you then either resolve all dependencies synchronously, or you resolve all dependencies asynchronously, regardless of whether the dependencies themselves need asynchronous initialisation (although it would probably be helpful to check at compile time for errors which would arise from getting an async dependency synchronously, but that should be relatively easy to do).

To give an example which approaches something like a real-world scenario, imagine the following: you have an asynchronously-initialised service called Config, which has to be instantiated using a static async init() method, and then you have a couple of other services, some of which depend on Config.

export class Config {
  static async init(configDir: string): Promise<Config> {
    // ...
    return new Config(loadedConfig);
  }
}

export class AuthenticationChecker {
  constructor(
    private readonly config: Config,
  ) {}
}

export class HttpServer {
  constructor(
    private readonly auth: AuthenticationChecker,
  } {}
}

export class Logger {
  constructor(
    private readonly authCheckerPromise: Promise<AuthenticationChecker>,
  ) {}
}

const container = new DIContainer();

container.registerSingleton<Config>(async () => Config.init(process.env.CONFIG_DIR)); // [1]
container.registerSingleton<AuthenticationChecker>(); // [2]
container.registerSingleton<HttpServer>(); // [3]
container.registerSingleton<Logger>(); // [4]

Now, when the compiler finds the registration of the Config service at [1], it has to analyse the factory function for dependencies to inject - at which point it can also quite easily figure out that the factory function is async (or really anything that returns a Promise, it doesn't necessarily have to be async to do that). So it can remember that Config is initialised asynchronously.

When the compiler finds the AuthenticationChecker registration at [2], it has to analyse the constructor for dependencies to inject, and finds Config. It already knows that Config is an async service, so it can mark AuthenticationChecker as async as well.

Similarly, when the compiler reaches HttpServer at [3], it would know to mark the service as asynchronous, because at least one of its direct dependencies is async - AuthenticationChecker, in this case - it doesn't have to know anything at all about why that is (ie, it doesn't care about the Config service at this point).

And finally, when the Logger registration at [4] is reached, the compiler knows that the service depends on a promise for the AuthenticationChecker service, not the resolved instance - so it knows it doesn't need to make the Logger service async (I'll show why shortly if it's not yet clear).

At runtime, when creating an instance of any service, the container has a list of dependencies it should inject - the current implementation would basically do something like this:

createInstance(serviceClass: Constructable, dependencies: string[]) {
  return new serviceClass(...dependencies.map(dependency => this.get(dependency)));
}

All that would be needed to support async services at runtime would then be to have a flag available for each service (provided by the compiler) which would tell this method whether the service is async, and then the above code could be modified to something like this:

createInstance(serviceClass: Constructable, dependencies: string[], isAsync: boolean) {
  return isAsync
    ? this.createInstanceAsync(serviceClass, dependencies)
    : this.createInstanceSync(serviceClass, dependencies);
}

private createInstanceSync(serviceClass: Constructable, dependencies: string[]) {
  return new serviceClass(...dependencies.map((dependency) => this.get(dependency)));
}

private async createInstanceAsync(serviceClass: Constructable, dependencies: string[]) {
  return new serviceClass(...await Promise.all(dependencies.map(async (dependency) => this.get(dependency))));
}

The only other thing that would need to be considered is that when asynchronously creating singleton service instances at runtime the container would always have to store the promise for the service (ie. the result of createInstanceAsync, without awaiting it), so that the service is still guaranteed to be only created once, regardless of multiple concurrent requests for it. That's simply a matter of where something like this.services.set(id, instance) is called and where the internal collection of created services is being checked.

Lastly, calls to container.get<SomeService>(); need to be processed by the compiler to transform <SomeService> to something the code can use at runtime. Since the compiler knows which services are async, it should be trivial for it to check that they are retrieved using a Promise<SomeService> specifier and throw an error otherwise - so for example:

const config = await container.get<Promise<Config>>(); // works as expected
const authChecker = container.get<AuthenticationChecker>(); // throws an error at compile time
const promiseForServer = container.get<Promise<HttpServer>>(); // works as expected
const logger = container.get<Logger>(); // works as expected (!?)

Now finally for the Logger service: from the last code snippet it might be slightly more apparent how and why I think the Logger service could remain synchronous even though it depends on the AuthenticationChecker, which is async - because in fact the Logger depends on a promise for the AuthenticationChecker, rather than the resolved instance, and that's exactly what you get by calling container.get<Promise<AuthenticationChecker>>() without awaiting the result, as also shown in the last code snippet with the promiseForServer thing. So you can have synchronous services which depend on async services, they would just need to deal with resolving the promise internally - which might be totally legitimate in some scenarios (and I don't mean two-step initialisation) - e.g. in the Logger constructor, you could have something like:

// it's a contrived example, but it allows for the Logger to be available immediately
// without waiting for the AuthenticationChecker to be created, while at the same time
// allowing Logger to bind to some event on the AuthenticationChecker as soon as it can:
authCheckerPromise.then(authChecker => authChecker.on('fail', () => this.log('authentication failed')));

Notice that none of the services knows or cares that one or more of its dependencies needs asynchronous initialisation; and also the runtime container doesn't care about the entire tree, just the immediate dependencies. If you imagine what the generated code would look like if actual factory methods were generated like they usually are in PHP frameworks, the factory method for e.g. the HttpServer service would look like this:

async (container) => new HttpServer(await container.get('AuthenticationChecker'))

I hope it's now clearer what I have in mind. I can't say for sure because I don't know all the ins and outs of this library, but I think that it would end up being a smaller change to the code than you'd expect, and at least to me the benefits would be immense - most services in my apps have the MikroORM service somewhere in their dependency chain, and so most services in my apps need to be async, which would currently mean writing and maintaining a lot of factories where most of them wouldn't need to exist with the support for async services.

@jahudka
Copy link
Author

jahudka commented Mar 21, 2023

One other thing which might not be entirely obvious - this approach allows you to have both synchronous and asynchronous services at the same time, it can deal with dependencies between them, and the only time you need to care about whether a service is async or not is when you call container.get() - which would work exactly as it does now for synchronous services (ie. no need to await the result), you'd only need to await it when accessing an async service - and it appears you probably don't have many of those, so for use-cases which don't need async services, basically nothing would change :-)

@jahudka
Copy link
Author

jahudka commented Mar 21, 2023

And another thing - regarding what you said about building lists of things to be injected - wouldn't it be just swell if you could do something like this? :-)

container.registerSingleton<LogWriterInterface, FileLogger>(); // this one's async
container.registerSingleton<LogWriterInterface, ConsoleLogger>(); // this one's not
container.registerSingleton<LoggerInterface, Logger>();

export class Logger implements LoggerInterface {
  constructor(
    writers: LogWriterInterface[], // yay, you don't care whether they're async!
  ) {}
}

I'm not sure if the container currently allows registering multiple services which implement the same interface and then obtaining all of them based on the interface, without knowing the individual services in advance, but it's another thing that is commonly possible in PHP DI frameworks. But supporting this is a separate issue - just something I thought I'd mention for completeness' sake.

@cmidgley
Copy link
Contributor

Sorry for the long delay - rather busy right now.

In your example, you have this factory set as a class injector for Config:

container.registerSingleton<Config>(async () => Config.init(process.env.CONFIG_DIR));

Whenever injection forms objects, it is always done with get. Perhaps if this is a single set of get calls at the very start, this isn't a big deal, but I find I often need to cause injection within my code. Take this example, where I want to take a list of something and construct new (injected) objects for each of the members:

someArray.forEach((x) => newArray.push(container.get<SomeClassThatMightDependOnConfig>())

For that to be constructed, I would have to await it when using get:

someArray.forEach((x) => newArray.push(await container.get<SomeClassThatMightDependOnConfig>())

For this to work (and get typed correctly), doesn't that mean everything would have to be async?

@jahudka
Copy link
Author

jahudka commented Mar 30, 2023

Well, a lot depends on how far the library goes. I'm of the strong opinion that you should almost never use the container directly - because that creates a dependency on the container itself, and therefore, in my opinion, it goes directly against the core concept of DI - that services shouldn't know DI exists at all - and much less its implementation details, like the existence and signature of a get() method. It's why I hate decorator-based DI - because it creates tight coupling between the DI implementation you're using and your actual code.

And I think that a good DI implementation will actually give you the tools to achieve this. I'm not sure since you're not explicitly stating it, but the use case you're describing looks to me like a situation where the list of services you're interested in should all implement a specific interface. If that's the case, it would be solved by the container & compiler supporting the injection of lists of services, like this:

interface Animal {
  getSound(): string;
}

class Dog implements Animal {
  getSound(): string { return 'bark! bark!'; }
}

class Cat implements Animal {
  getSound(): string { return 'meow! meow!'; }
}

class Snake implements Animal {
  getSound(): string { return 'hisss! hisss!'; }
}

class Zoo {
  constructor(
    readonly animals: Animal[],
  ) {}
}

const container = new DIContainer();
container.registerSingleton<Animal, Dog>();
container.registerSingleton<Animal, Cat>();
container.registerSingleton<Animal, Snake>();
container.registerSingleton<Zoo>();

I'm intentionally omitting async services in this example for now. Imagine that the DI could solve this kind of problem - having multiple services of the same type and injecting them all where a list of that type is expected - so that when a new instance of Zoo is being created, this is approximately what would happen:

return new Zoo([
  container.get<Animal>('dogServiceId'),
  container.get<Animal>('catServiceId'),
  container.get<Animal>('snakeServiceId'),
]);

Now let's bring back async services into the mix: imagine that e.g. the Cat service depends on Config, which is async - then Cat would have a constructor like this:

constructor(private readonly config: Config) {}

Now, when the compiler is building service factories, it knows that Config is async, therefore Cat must also be async. An instance of Cat would be created like this:

return new Cat(await container.get<Promise<Config>>('configServiceId'));

Note that get() needs to return Promise<Config>, which is then immediately awaited, so that the actual resolved instance of Config is passed to Cat. A Zoo instance would then be created like this:

return new Zoo([
  container.get<Animal>('dogServiceId'),
  await container.get<Promise<Animal>>('catServiceId'),
  container.get<Animal>('snakeServiceId'),
]);

This is what happens under the hood - no matter whether it's actual generated code or something else - you don't see this yourself, the DI does this for you. But notice that even though the Cat service is now async, the Zoo service didn't need to change at all - even though it depends on Cat: it will always get a Cat, no matter if the DI needs to await it to obtain it. Like I said before, the only place where you'd need to care about async services being async is when manually calling container.get(), and one of the use cases you describe for doing that would in my opinion be solved this way, and more cleanly at that.

There's all sorts of other things the container could support, like on-the-fly generated service accessors (which is a common way of getting lazy services, or solving circular dependencies):

type GetCat = () => Cat;

class Dog {
  constructor(readonly getCat: GetCat) {}
}

class Cat {
  constructor(readonly dog: Dog) {}
}

Here, Cat and Dog depend on each other, and normally there would be no way of injecting one into the other at the same time. But the container could be smart enough to see the GetCat type as a service accessor, and when creating a Dog, it could do something like this:

return new Dog(() => container.get<Cat>('catServiceId'));

Again, this can work with async services, or even lists of (possibly) async services, as well:

type GetCat = () => Promise<Cat>;

class Dog {
  constructor(readonly getCat: GetCat) {}
}

// DI generates this:
return new Dog(async () => container.get<Promise<Cat>>('catServiceId'));

// of course, now you have to await dog.getCat(), because it's async - but I find that
// I only need service accessors quite rarely, because most situations can be solved
// by injecting the service / service list directly

So in conclusion, yes, with async services, you'd sometimes have to make your code async as well. But not in quite as many places as you might think, and in many of the places where you would be forced to do it, you'd probably have to do it anyway.

EDIT: Thought it's important to mention that the service accessor pattern serves one very important purpose: it decouples the service code from the DI container - notice that Dog doesn't even need to know that a container exists, or how it works. The only place where this principle is broken (to some extent) is when Cat is async because of one of its dependencies and we want to inject an accessor for it - because in theory we shouldn't care whether Cat is async or not when asking for an accessor for it, just like we didn't care when asking for the resolved instance as a constructor parameter. So this means that our Dog now has to know something about how its dependencies are created, and that's not okay (even though it still doesn't know anything about the DI itself).

But I still think this is worth exploring, since it would neatly solve a large number of cases which currently aren't supported, even if it would still leave some (more or less edge) cases out. And there's probably also some magic which could be done with Proxy in this area (injecting a Proxy where one might inject an accessor; maybe for async services the Proxy could expose async wrappers of all public methods of the original service, and the wrappers would resolve the service on the first method call.. but I've never really used Proxy, so I have no idea what's possible or not there).

@cmidgley
Copy link
Contributor

cmidgley commented Apr 1, 2023

I generally understand what you are saying (except for a key point, below), but I do think that means that get has to be async, doesn't it? Every example you show that uses a generated get with await depends on that method being async, and therefore whatever calls it, which means the top of the food chain is get ... implying get has to be async to fulfill the await to create any object that might potentially be async.

And, if get becomes async, that means it can no longer be used in a constructor, and constructors that need to consume get to make an object ready for use will be at risk of violating the two-step initialization anti-pattern. Perhaps your point is that nobody should consume get other than the initial kickoff from main (where using async is easy)?

To that point, and using your example code fragment:

return new Zoo([
  container.get<Animal>('dogServiceId'),
  container.get<Animal>('catServiceId'),
  container.get<Animal>('snakeServiceId'),
]);

This code, which I believe you are saying would be created automagically by the DI compiler, references identifications of what objects to create by using various IDs (dogServiceId). Where do these come from, and how do they cause the correct object type to be instantiated? I can see that as a factory method, where I then can use logic such as looking up something in a database and determining the correct object to create - sharing the same abstract base type as you suggest - but that returns us to having to use get in our factory. Am I missing something here?

I do wish DI could let me inject non-class entities (arrays, simple types, etc) without having to wrap them in a class. My initial research into this is that the compiler appears to do the right thing, but the DI instantiation logic currently requires that the instantiated member is a function that can be executed. It can be done using a registration function, but it's not native. For example:

        DIContainer.container.registerSingleton<IThing, "test">();
        const simpleType = DIContainer.container.get<IThing>(); <-- "TypeError: constructable is not a function"
        expect(simpleType).toBe("test");

But this does work (at least for registration and get, I've not tried auto-injecting into constructors so that may be a sticking point):

        DIContainer.container.registerSingleton<IThing>(() => "test");
        const simpleType = DIContainer.container.get<IThing>();
        expect(simpleType).toBe("test");

With plumbing work, I could see how a similar concept could be implemented to inject a Promise. It works if you use an instantiation function, but that would then need to consume get to retrieve the object from DI. That syntax is ugly, and might be a place for a creative solution to generate it correctly, perhaps container.registerSingletonPromise<IMyClass, MyClass>() or even decode that it starts with a Promise such as container.registerSingleton<Promise<IMyClass>, MyClass> and auto-generate the promise wrapper when instantiating the object. Again, checking what happens on the constructor for auto-injection would also need to be done as I've not looked at that yet.

Of course, having a promise to an object passed into a constructor is another mess, as constructors can't await and therefore extra steps would have to be made within the implementation to avoid the two-step instantiation anti-pattern. But this at least would localize the issue to the classes that need that functionality, rather than affecting the entire program.

I don't recommend this approach but wanted to bring it into the conversation to see where it leads us. It would be much nicer to have those promises automatically fulfilled, as you suggest, so perhaps I'm just missing something here and it actually could be possible?

BTW, I don't (yet) see a major flaw in consuming get within the code. It maintains inversion of control because the code doesn't know what the implementation is - it just knows it needs an Animal (or whatever). It's maintained because the registrations control the implementation, not the class that consumes it. Indeed, you have not accomplished independence from the injection system, but I'm not sure that is as important as the core principles of inversion of control.

@jahudka
Copy link
Author

jahudka commented Apr 1, 2023

As for container.get() having to be async, imagine the following extremely bare-bones implementation of a Container class:

export type Factory<T> = (container: Container) => T;

export class Container {
  private readonly services: Map<string, any> = new Map();
  private readonly factories: Map<string, Factory<any>> = new Map();

  register(id: string, factory: Factory<any>): void {
    this.factories.set(id, factory);
  }

  get<T>(id: string): T {
    const service = this.services.get(id);
    return service ?? this.create<T>(id);
  }

  create<T>(id: string): T {
    const factory = this.factories.get(id);
    const service = factory(this);
    this.services.set(id, service);
    return service;
  }
}

Now imagine you have an async service (e.g. Foo) you want to register and use in such a container:

container.register<Promise<Foo>>('foo', async () => new Foo());
const foo = await container.get<Promise<Foo>>('foo');

The important part is that the container never has a reference for the final instance of Foo - when the factory is called in container.create(), it returns a Promise<Foo>, and that's what is stored in the internal container.services map. Whenever you call container.get('foo'), you get that Promise, whether it's during the first call to .get() which has to call .create() since the service doesn't exist yet, or any later call. The get and create methods both work synchronously - it's just their output that may or may not be asynchronous. And when you work with both sync and async services, you can still get any sync services by calling just container.get<SomeSyncService>('something'), without using await, so long as the service is synchronous - which just means that it doesn't have any async dependencies or an async factory.

Now, when you're using the container, whenever you call container.get(), you have to know whether the requested service is async or not - so that you can provide the correct Promise-wrapped type argument, and more importantly, so that you actually await the result (which you'll be forced to do if you provide the Promise-wrapped type argument). But when you register a second service (e.g. Bar), which depends on Foo, you just make the factory for Foo async, and within the factory you await the call to container.get('foo'), so that Bar receives the resolved instance - therefore Bar doesn't know or care that Foo was async - its constructor is being passed a Foo value, not a Promise<Foo>:

export class Bar {
  constructor(foo: Foo) {}
}

container.register<Promise<Bar>>('bar', async (c) => new Bar(await c.get('foo')));

Obviously now whenever you want to get Bar, you must also use await container.get<Promise<Bar>>('bar'), instead of container.get<Bar>('bar') - that's the price of having async services. But - and I think this is the crux of the difference in our viewpoints - I'm strongly of the opinion that you shouldn't care, because you shouldn't call container.get() explicitly all that often - in my opinion, the point of having a DI container (and especially a smart one which does its best to handle injection for you) is that you don't inject services yourself - you have the container do it for you. From my point of view this is both "purer" in terms of inversion of control core principles, and at the same time better for me as a programmer because I can write less code.

So I think that summarises your questions regarding the get() method:

  • It doesn't have to be async to support async services - just the factories which actually instantiate async services need to be async (but those would be autogenerated most of the time) and any place which consumes the output of .get() needs to await it if the output is an async service - and again, I think most of the time that would happen in autogenerated factory code.
  • Yes, I think explicitly consuming get() is a dependency injection anti-pattern and should be avoided (because it makes your services depend on the container, rather than on the dependencies they actually need) - I'm not saying you should never do it, just that you should avoid doing it whenever possible, and that if you do that, you end up caring a lot less about whether the result needs to be awaited (because it's hidden away by the container).

Regarding the injection of non-class values: I just think that that's a matter of whether the container exposes a method to register an instance of a service, rather than a factory - as far as I understand, the current implementation in this library only allows you to register factories - but it doesn't really prevent you from injecting non-class values, as long as you register them using factories (e.g. container.registerSingleton<string>(() => "some interesting string"), like you suggested). I don't see a reason that wouldn't work. Well, with a caveat - as far as I know, you can currently only register one service of a given type, so you could only have e.g. one string. I'm not entirely sure about this, but I think that the TS compiler reduces type aliases for primitive types to the actual primitive types, so you probably can't do something like type Username = string; type Password = string; and then register these as services, as both would probably be reduced to string before the DI compiler can see they're different named aliases. But you should be able to register and inject e.g. type Credentials = { username: string, password: string } without issues.

And lastly, regarding service IDs - in most of the examples I wrote in this thread I'm not talking about any specific DI implementation, it's mostly intended as pseudo-code; but it's often the case that DI container implementations allow you to register services without explicitly giving them IDs, and the IDs are then generated by the DI compiler. So when I write e.g. dogServiceId in some example code, it's meant as a placeholder for whatever ID the DI compiler would come up with, whether that's a number, a random string, or something which is actually derived from the service type like a dogServiceId string.

@cmidgley
Copy link
Contributor

cmidgley commented Apr 1, 2023

OK - that helps a lot. The registration would be for a Promise, and get would be async when getting those injections. That is what I've been asking for a bit now, so thanks for this clarity.

I think this is the crux of the issue:

Obviously now whenever you want to get Bar, you must also use await container.get<Promise>('bar'), instead of container.get('bar') - that's the price of having async services.

This means I must know if classes, including those that depend on countless others that I have no idea of (thanks to inversion of control), are going to require async. That to me is the key problem, and why I keep saying get would have to be async to maintain inversion of control and maintain abstraction from implementation, but perhaps there are alternatives (see below the line for some imperfect thoughts).

I'm strongly of the opinion that you shouldn't care, because you shouldn't call container.get() explicitly all that often

That's cool. It is a difference of opinion. I like the simplicity and clarity of stating what I want to achieve in a simple condition (I have this and I need that, all without any knowledge of implementation). The alternative, if I understand your suggestion, is to add a class that receives all possible instantiated objects, and then select the one we want and return it, discarding the others. That seems inefficient, less obvious and adds to the boilerplate. But like many things in programming, there are many ways to solve a problem and even more opinions on what is right!

so you probably can't do something like type Username = string; type Password = string; and then register these as services

This appears to work correctly, at least up to the point of calling get and getting the expected value back. As I said earlier, I didn't finish the test all the way down to the automatic constructor injection, but I would expect that to "just work" seeing how the internals work. Specifically, the interface name is transferred into the generated code by the compiler, and it matches the interface name and not the native type.

This also means the ability to get a Promise understood by registration has many of the underpinnings in place, though there is work to be done to get to the desired end state (and, perhaps getting non-classes to work as well).


Shifting gears to implementation ideas...

If get might need async, and you can't know at compile time if it will or not, it would be a breaking change to DI. That would need to be avoided.

One possibility is a compile-time flag (maybe in tsconfig) that tells the compiler (and in turn the library) how to behave, to avoid breaking changes. This is risky because imported packages might use a different setup and yet expect the injection to behave a certain way (I do this a lot, as I have about 70 packages that make up my project using a monorepo).

Another approach could be a second get call (getAsync) and then have a fail exception if get needs to resolve async. This has the undesirable side-effect of failing, at runtime, when an injection is formed for something async when the code didn't expect it (and solely based on what was injected, causing an inversion of control issue).

I don't really like either of these but thought I'd at least get them on the table. Do you have ideas on how to approach this without breaking existing deployments and without these side effects?

@jahudka
Copy link
Author

jahudka commented Apr 3, 2023

This means I must know if classes, including those that depend on countless others that I have no idea of (thanks to inversion of control), are going to require async.

Well, yes and no. You don't necessarily need to keep track of that yourself - remember the compiler has all the info, so when it comes across a call to container.get<Something>(); (which it needs to do in order to inject the generated service IDs into the call sites), it knows whether Something is async or not; and so it can tell you when you try to inject an async service synchronously. It'd be a compile time error - basically saying something like Attempting to inject async service "Something" synchronously, please change line X in file Y to 'await container.get<Promise<Something>>()'. Is that potentially bad DX? In some ways yes. But I'd say it is far outweighed by all the work you'd need to do manually in order to have async services without native support in the compiler. And even then - you'd only ever need to care about this if you a) actually had any async services, and b) explicitly manually consume container.get() often - and from what I gather, the first one doesn't really apply to you since you don't seem to use or need async services all that often (and with synchronous services the implementation would work exactly the same as it does now), and the second doesn't matter all that much to me, because I don't consume container.get() manually very often. So having an implementation along the lines of what I propose is at worst a win for my kind of use case and a "no-op" for yours. Just another thing the library can do, use it if you want.. or not.

Regarding your opinion that container.get() would have to be async to maintain inversion of control: I don't actually believe that's the case - from an implementation standpoint, it doesn't need to be async in order to work as intended; and as for following the principles of IoC I think you're technically right, but practically it isn't relevant - as soon as you have even a single async service in your code, you'll need to await it somewhere - and whether that's at the call site where you're retrieving the service, or perhaps at the very start of your application where you'd do something like await container.createEveryAsyncServiceInCaseINeedItLater(), you can't avoid the await - the only other way around it would be to have any injectable async value injected as an unresolved Promise, and await it manually in every method of every service which uses it, which just shifts all the hassle further down the chain. Long story short - if you want to have async services at all, you simply cannot get around using await somewhere - and I think doing it the way I'm proposing actually provides the best-possible-case scenario for how much you're shielded from having to do it yourself.

The alternative, if I understand your suggestion, is to add a class that receives all possible instantiated objects, and then select the one we want and return it, discarding the others.

Well, no, not really. I'm not entirely sure what kind of usecase you're solving with the forEach(), but it appears to me as though it's something like the decorator pattern applied to a list of services. So for example, let's say you have a NotifierInterface with several implementations, and a LoggingNotifier façade which you want wrapped around all the NotifierInterface implementations you register in your DIC. Without assuming any special support for the decorator pattern within the DIC (which you could also have! but let's keep it simple for now), using just what the DIC can do now, quite possibly without any modifications needed:

// bare implementations:
export class SmsNotifier implements NotifierInterface {}
export class EmailNotifier implements NotifierInterface {}
export class SlackNotifier implements NotifierInterface {}

// logging façade:
export class LoggingNotifier<T extends NotifierInterface> implements NotifierInterface {
  constructor(private readonly notifier: T) {}
}

// register implementations:
container.registerSingleton<SmsNotifier>();
container.registerSingleton<EmailNotifier>();
container.registerSingleton<SlackNotifier>();

// register a façade for each implementation:
container.registerSingleton<LoggingNotifier<SmsNotifier>>();
container.registerSingleton<LoggingNotifier<EmailNotifier>>();
container.registerSingleton<LoggingNotifier<SlackNotifier>>();

// consumption, e.g.:
const notifier = container.get<LoggingNotifier<SmsNotifier>>();

// or, and this is the only thing that I don't think is currently supported:
const notifiers = container.get<LoggingNotifier<any>[]>(); // get all of them at once, as a list

This means that you don't need to actually inject the container itself into your services, you can inject the decorated implementations directly, without calling container.get() in the constructor. If you need a list of services, the DI could just support that - again without the need to explicitly call container.get() - and however you need to transform the services, no matter whether they're async or not, in the constructor you get the resolved array of instances, so you can .map() over that and transform it however you want - say that e.g. you use the bare NotifierInterface implementations in most of your app, but then one service needs them wrapped in the LoggingNotifier façade - you could do this:

class SomethingWhichNeedsAllTheNotifiers {
  private readonly notifiers: LoggingNotifier<any>[];

  constructor(notifiers: NotifierInterface[]) {
    this.notifiers = notifiers.map(notifier => new LoggingNotifier(notifier));
  }
}

// register bare services just like before, then register the new guy:
container.registerSingleton<SomethingWhichNeedsAllTheNotifiers>();

// and prosper

And if you need to perform some logic to determine which services from a set of candidates should be injected, it can still be done in the service factory (perhaps with a sprinkling of awaits here and there, if some of your services are async); it might even be argued that this way's cleaner, because otherwise your service is resolving its own dependencies, instead of simply being passed them by "whoever" is creating the service (in this case the factory function).

Then there is also the concept of lazy accessors - basically autogenerated callbacks you can inject into a service, which you can then call from within the service to obtain a dependency lazily only when it is needed - this can be used if you need to decide which dependencies to inject from within the constructor; the only thing this would have a problem with is async services, because those would need an async lazy accessor callback, and that can't be resolved within the constructor. But it's still a way to reduce the need to manually call container.get() at least in some scenarios.


Regarding the implementation ideas, I believe I already answered that at the start of this comment - get() doesn't need to be async, and the compiler should have all the information it needs to determine at each call site whether the result should be a Promise and / or whether it needs to be awaited - so no need for additional methods like .getAsync(), the regular old .get() can do it all - no side effects, no runtime errors, no library compatibility issues.

@cmidgley
Copy link
Contributor

cmidgley commented Apr 4, 2023

I don't have time right now to look at the Notifier sample you provided (though I will, as I'm quite interested) ... but one point I'll make quickly about the core issue at hand with get:

remember the compiler has all the info

That is not true. The compiler only knows about what it is processing right now, which is usually the current source file and whatever it imports (often the current package). If you use multiple packages, which I do a lot of, those packages are not aware of the others at compile time. Any package can register interfaces that are later consumed by other packages. This works with DI because registration is a runtime concept, not compile time (at compile time it's really just sugar on the registration call). I think this means that get would have to become async to be able to inject unknown objects that need to be resolved promises.

@jahudka
Copy link
Author

jahudka commented Apr 4, 2023

The compiler only knows about what it is processing right now, which is usually the current source file and whatever it imports

Aha, wow, didn't realise that. Well, that does complicate things.. It doesn't mean that container.get() needs to be async, but it does mean that you'd need a runtime check in .get() and that you would get runtime errors if you tried to resolve an async service without specifying the Promise wrapper in the type hint. Hmm, and in fact, it would probably mean that you'd have to do some extra work in the service factory because you wouldn't be able to use await (otherwise yes, .get() would have to be async).

@cmidgley
Copy link
Contributor

cmidgley commented Apr 4, 2023

but it does mean that you'd need a runtime check in .get()

That's essentially my second proposal but perhaps tuned slightly to infer the return type of get based on if a Promise type was provided (to avoid both get and getAsync).

In thinking more about what happens internally, the DI compiler uses the name of the type as the id so that the DI runtime can build the injections. This means that any injection that wants an <Interface> would fail to find somebody who registers a <Promise<Interface>>. You would get the same runtime error as today when you fail to register a required interface. It wouldn't know whether it was because of a promise or not, but would naturally fall into the checking that is done already.

This implies then that a module, deep in some package, might change an internal (non-public) interface from sync to async, and suddenly require all parent modules/packages to have to change. That's pretty ugly, IMHO.

A cleaner solution might be to skip Promise when registering, instead leaving the type native, allowing the runtime registration to always map to types (async to sync). Then use a MaybePromise style type on all factories, and have get always be async. For libraries that want to use async registration factories, the ID generated could be modified (such as a prefix) so it never aligns with the non-async variants. This would ensure packages could never collide at runtime and fail (meaning, everything would have to be compiled with the same async vs. no-async option, or the runtime would not find the types).

This does have the side-effect that constructors would no longer be able to call get (see below for more on that), but might present a consistent DI implementation where the only differences would be a compile-time flag and await on get, making it easy to migrate/support both implementations and eliminate the problem of private interface changes from infecting other packages/modules.


I looked at your Notifier code, thanks. It seems similar to the other examples and demonstrates abstraction principles I am well versed in. But it does not address the use case I am referring to. I think I might be able to simplify this by restating the problem:

How can a factory function/method instantiate a class without using get, when the specific subclass can not be determined until the factory is executed?

Let's take a really simple example:

class Animal {}
class DogAnimal extends Animal {}
class CatAnimal extends Animal {}
class SnakeAnimal extends Animal {}

// public interface to show how AnimalFactory factory might be consumed
export class PetStore {
    constructor(private animalFactory: AnimalFactory) {}

    getAnimalById(id: 'dog' | 'cat' | 'snake'): Animal {
        return this.animalFactory.make(id);
    }
}

// singleton registered factory that needs to instantiate an Animal whose subclass isn't known until runtime
class AnimalFactory {
    make(id: string): Animal {
        return ???; // without using get, instantiate a DogAnimal, CatAnimal, or SnakeAnimal
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants