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

Add .toDynamicValueWithDeps into BindingToSyntax #1506

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

Conversation

SamakaCD
Copy link

@SamakaCD SamakaCD commented Apr 13, 2023

toDynamicValueWithDeps method binds an abstraction to a dynamic value with required dependencies from the container in a declarative way.

Description

I've addded toDynamicValueWithDeps method into BindingToSyntax class which basically uses toDynamicValue method and context it provides under the hood of the new method.

Related Issue

None.

Motivation and Context

Let's say we have the following classes:

abstract class AbstractShuriken {}

abstract class AbstractKatana {}

@injectable()
class Shuriken implements AbstractShuriken {}

@injectable()
class Katana implements AbstractKatana {}

class Ninja {
  public constructor(public shuriken: AbstractShuriken, public katana: AbstractKatana) {}
}

I would like to create a binding for Ninja class which depends on abstract shuriken and katana. Without the new method we would do something like this:

container.bind(Ninja).toDynamicValue((context) => {
  const shuriken = context.container.get(AbstractShuriken)
  const katana = context.container.get(AbstractKatana)
  return new Ninja(shuriken, katana)
})

The code might be even longer, especially when it's required to inject more dependencies. To make binding notation shorter we could introduce a method which accepts a list of dependencies and a factory which toDynamicValueWithDeps method actually does:

container.bind(Ninja).toDynamicValueWithDeps(
  [AbstractShuriken, AbstractKatana] as const,
  ([shuriken, katana]) => new Ninja(shuriken, katana)
)

Looking forward, it should be possible to remove as const for dependencies using const type parameters when the library will use TypeScript 5.

How Has This Been Tested?

I've added should be able to resolve all dependencies using toDynamicValueWithDeps test case into container.test.ts.

Types of changes

  • Updated docs / Refactor code / Added a tests case (non-breaking change)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated the changelog.

toDynamicValueWithDeps method binds an abstraction to a dynamic value with required dependencies from the container in a declarative way.
@PodaruDragos
Copy link
Member

This add a new method to the binding, so I am a bit reticent about this. code looks good though.
Might need more opinions on this.

PS: you could always just build this on your own project, TBH

@SamakaCD
Copy link
Author

SamakaCD commented Apr 13, 2023

you could always just build this on your own project, TBH

And that's right 🙂. Initially, I tried to extend ContainerModule class to replace the original binding syntax. But the result was somewhat cumbersome 😅

Anyway, it is up to you whether to include such a binding extension.

@SamakaCD
Copy link
Author

Improved generic constraints for toDynamicValueWithDeps. Also, updated the "Motivation and Context" section to highlight that it's not required to decorate binding class / service identifier with @injectable().

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.

None yet

2 participants