-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Comments
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 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 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. |
@cmidgley I think the 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 There's also another optimisation I think this might possibly unlock.. I imagine that using this library one would probably have something like a |
Lastly, sure, you couldn't use async |
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 ( You have to use Perhaps my earlier use case of constructors doing conditional 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. |
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 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 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 When the compiler finds the Similarly, when the compiler reaches And finally, when the 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 Lastly, calls to 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 // 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 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. |
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 |
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. |
Sorry for the long delay - rather busy right now. In your example, you have this factory set as a class injector for container.registerSingleton<Config>(async () => Config.init(process.env.CONFIG_DIR)); Whenever injection forms objects, it is always done with someArray.forEach((x) => newArray.push(container.get<SomeClassThatMightDependOnConfig>()) For that to be constructed, I would have to await it when using 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? |
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 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 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 constructor(private readonly config: Config) {} Now, when the compiler is building service factories, it knows that return new Cat(await container.get<Promise<Config>>('configServiceId')); Note that 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 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, 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 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 |
I generally understand what you are saying (except for a key point, below), but I do think that means that And, if 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 ( 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 Of course, having a promise to an object passed into a constructor is another mess, as constructors can't 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 |
As for 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. 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 Now, when you're using the container, whenever you call 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 So I think that summarises your questions regarding the
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. 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. |
OK - that helps a lot. The registration would be for a I think this is the crux of the issue:
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
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 appears to work correctly, at least up to the point of calling This also means the ability to get a Shifting gears to implementation ideas... If One possibility is a compile-time flag (maybe in Another approach could be a second 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? |
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 Regarding your opinion that
Well, no, not really. I'm not entirely sure what kind of usecase you're solving with the // 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 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 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 Regarding the implementation ideas, I believe I already answered that at the start of this comment - |
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
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 |
Aha, wow, didn't realise that. Well, that does complicate things.. It doesn't mean that |
That's essentially my second proposal but perhaps tuned slightly to infer the return type of get based on if a 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 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 This does have the side-effect that constructors would no longer be able to call 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 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
}
} |
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 likereturn 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:
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 thatcontainer.get<ServiceTwo>()
is attempting to access an async service without specifying thePromise
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!
The text was updated successfully, but these errors were encountered: