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

Factory aware of target class that resolves the factory's type. #56

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

perf2711
Copy link

This pull request enables factory for resolving dependencies to see which class is using the factory to resolve the dependency.

For example, when you use factory to resolve Logger class, which contains parameter name in its constructor, you can provide parent's class name to automatically name the logger.

Example of final usage:

export class Logger {
  constructor(private name: string) {}

  public info(message: string) {
    console.log(`[${this.name}] ${message}`);
  }
}

// New "target" argument in factory, which contains the constructor of parent class (or undefined, if resolved directly from container)
const loggerFactory: FactoryFunction<Logger> = (_, target) => new Logger(target ? target.name : 'Default');
container.register(Logger, { useFactory: loggerFactory });

@injectable()
export class TestClass {
  constructor(logger: Logger) {
    logger.info('test') // [TestClass] test
  }
}

@msftclas
Copy link

msftclas commented Sep 25, 2019

CLA assistant check
All CLA requirements met.

@@ -39,7 +39,9 @@ export default interface DependencyContainer {
instance: T
): DependencyContainer;
resolve<T>(token: InjectionToken<T>): T;
resolve<T>(token: InjectionToken<T>, parent?: constructor<any>): T;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove the optionality of the parent param since we have the above overload. (same with resolveAll(...))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the optionality, and updated predicateAwareClassFactory to match.

@asciidisco
Copy link

Is there anything a non-committer like me can do to help get this merged?
It would help us a lot to get rid of some boilerplate code, not only as described in the logging example above, but also for injecting configuration into our classes.

@Xapphire13
Copy link
Collaborator

@asciidisco, hang tight; I will give this another look over this week. Thanks for your patients =]

@MeltingMosaic
Copy link
Collaborator

I think that passing a constructor to the method leaves room for weird abuses, so instead I think it might be better to pass metadata about the constructor instead - would that still work for your scenario? I've had Hyrum's Law on my mind a lot lately, and I'm trying to keep the surface area low.

@asciidisco
Copy link

asciidisco commented Dec 4, 2019

I don't own neither the PR nor would I consider myself a domain expert in DI/IoC topics nor Typescript, so I don't know how much weight you should put on my thoughts.

Back in the day when I started as a developer in the web sphere, things like you mentioned with method leaves room for weird abuses were actually a good thing, as they made things possible that the domain owners of libraries & browser vendors couldn't forsee. Therefore I wouldn't consider it a bad thing per se, that the implementation that is presented in this PR could present more powers to the user. On the other side, I use tsyringe (and not one of the other DI libraries that are around) because of its narrowed scope & clean API.

Summed up, I'd be happy with the approach of passing just the metadata that @MeltingMosaic proposed, as it would enable the use cases I have.

I'd even go that far & say that the metadata approach would not limit the proposed functionality to be used in factory functions only. From my point of view, it would be an interesting approach to inject the metadata into constructor methods that especially "ask" for them, withiout using a factory.

I'm thinking of something like this:

import { autoInjectable, SomeMagicMetadataInterface } from 'tsyringe'

@autoInjectable()
export class Logger {
  private name: string
  constructor({ name }: SomeMagicMetadataInterface) {
    this.name = name
  }

  public info(message: string) {
    console.log(`[${this.name}] ${message}`);
  }
}

@autoInjectable()
export class TestClass {
  constructor(private logger: Logger) {
    logger.info('test') // [TestClass] test
  }
}

Don't know if this is a bit far fetched, but it could potentially reduce boilerplate code a bit further.

@Xapphire13
Copy link
Collaborator

I'm still a little torn on which approach is best for this; I can see on one hand that a metadata object is just the right amount of context that the problem needs, but on the other do see how passing the target itself could enable some pretty cool stuff in the right hands.

I'm thinking maybe for now to go the route of the metadata object, starting with what we need (the targetName). And if in the future we need more (such as targetConstructor), then we can add as needed as a non-breaking change.

With regards to your idea of allowing the metadata object be injected into a class, I'm not so sure I would want to allow this; I can see this leading to code that's written in a super tight-coupled way to the DI framework, when the whole point of DI is to promote loose-coupling.

@Xapphire13
Copy link
Collaborator

@perf2711, are you still working on this?

@perf2711
Copy link
Author

perf2711 commented Nov 9, 2020

@Xapphire13 sorry, no, not at the moment.

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

Successfully merging this pull request may close these issues.

5 participants