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

Constructor injection for factories with arguments #24

Open
polerin opened this issue Aug 22, 2022 · 3 comments
Open

Constructor injection for factories with arguments #24

polerin opened this issue Aug 22, 2022 · 3 comments
Labels
enhancement New feature or request

Comments

@polerin
Copy link

polerin commented Aug 22, 2022

Current example of factories with arguments (and the related tests) seems to only allow for intializer injection into an already instantiated object, instead of allowing for a factory method that returns a new instance. This means it is impossible to inject the factory arguments as constructor arguments on the returned instance. I'm attempting to inject into a class who's parent I do not control, and need to inject dynamically created arguments into the constructor.

If there's a way to do this, can we update the documentation to reflect the method?

example:

import { token, Container, Factory } from 'brandi';

class OtherThing { } 

class UntouchableParent {
  protected dep: OtherThing;
  
  public constructor(dep: OtherThing) {
    this.dep = dep;
  }
}

class MyChild extends UntouchableParent {
  //...
}

const TOKENS = {
  myChildFactory: token<Factory<MyChild, [dep: OtherThing]>>('Factory<MyChild>')
};

const container = new Container();

container.bind(TOKENS.myChildFactory)
  .toFactory(MyChild, (dep: OtherThing): MyChild => new MyChild(dep));


const factoryInstance = container.get(TOKENS.myChildFactory);

const childInstance = factoryInstance(new OtherThing());
@vovaspace
Copy link
Owner

Hi!

Actually I don't like this design myself, but here's the problem I can't solve:

class MyChild {
  constructor(someService: SomeService: otherThing: OtherThing) {
    /* ... */
  }
}

const TOKENS = {
  someService: token<SomeService>('SomeService'),
  myChildFactory: token<Factory<MyChild, [dep: OtherThing]>>('Factory<MyChild>')
};

injected(MyChild, TOKENS.someService, /* ??? */);

const container = new Container();

container.bind(TOKENS.myChildFactory)
  .toFactory(MyChild, (dep: OtherThing): MyChild => new MyChild(/* ??? */, dep));

I mean, the problems start when you need to combine injected arguments and external ones.

It might look like:

container.bind(TOKENS.myChildFactory)
  .toFactory(
    MyChild,
    (dep: OtherThing): MyChild => new MyChild(container.get(TOKENS.someService), dep),
);

but I don't think it's the best design. What do you think?

@vovaspace vovaspace added the enhancement New feature or request label Oct 14, 2022
@polerin
Copy link
Author

polerin commented Nov 7, 2022

Sorry for the delayed response, I keep meaning to sit down and give this the attention it deserves. The bottom example is something that I'm actually using in some of my code currently, and while it sorta works there are several issues with it.

  • Currently, as far as I can tell, the constructing container is not supplied to the factory, so you have to rely on inherited scope. That makes it very difficult to use factories that are not in the binding file itself, leading to that file blowing out.. unless you then inject the container, which is a whole different mess.
  • It means that container aware factories are not really viable for use in dependency modules, as they will have no idea of the container they are being used from within. (ran into this one today)
  • This method also requires that you either hard code the tokens, or inject them. That gets really really cumbersome, especially if you're using this to build factories for a library.
  • It leads to a lot of not-so-DRY code which (while not exactly unusual for the construction layer) is annoying.
  • I'm not sure how it would live alongside the initialization pattern that is currently in place. I'm not a huge fan of that pattern as a default, but it definitely solves some problems with objects that don't support constructor injection.

I think there are a few potential things that could help in all of this:

  1. Supply the calling container to the factory at call time. I like this better than injecting it into an instance of the factory, as it allows child container bindings to be used, even if the factory was bound to a parent container or dependency module.
  2. Create something that acts as something between a factory and a container, in that it allows you to supply temporary bindings for use in a normal resolution request down the line.
  3. Add a runtime token type that specifies that the dependency will be supplied at runtime instead of being a registered token

So numbers 2 and 3 might look something like this

// [...]

injected(MyChild, TOKENS.someService, RuntimeToken('otherThing'), RunTimeToken('anotherOtherThing'));

const MyChildBindingResolver: BindingResolver<OtherThing, OtherThing> = (thing1: OtherThing, thing2: OtherThing) => { 'otherThing' : thing1, 'anotherOtherThing': thing2 };
  
// Potential binding syntax 1
container.bind(TOKENS.myChildFactory)
  .withBindingSource(MyChildBindingResolver)
  .toFactory(MyChild, Factory<MyChild>) ;  // unsure if this signature would need to change

// Potential binding syntax 2
container.bind(TOKENS.myChildFactory)
  .toFactory(MyChild, Factory<MyChild, [thing1: OtherThing, thing2: OtherThing]>, MyChildBindingResolver);

The idea being that you'd call the factory with arguments, but it'd pass those arguments to the binding resolver... though looking at it, that resolver might not be necessary if we are using the factory tuple with a Runtime token?

I'm reaching the edges of my TypeScript knowledge with this one, sorry... it's just a bit of functionality that is common in the DI systems I've used in other languages.

Thanks regardless!

@polerin
Copy link
Author

polerin commented Jan 9, 2023

just bumping, any movement on this?

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

No branches or pull requests

2 participants