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

Calling injected with a token instead of constructor #19

Open
ngerritsen opened this issue Feb 18, 2022 · 2 comments
Open

Calling injected with a token instead of constructor #19

ngerritsen opened this issue Feb 18, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@ngerritsen
Copy link

ngerritsen commented Feb 18, 2022

Currently when defining arguments using injected, one has to pass in the constructor as a first argument. This kinda removes a lot of the flexibility a DI container can give you. Let's say we want to do the following:

const TOKENS = {
  readCredentials: token<string>("readCredentials"),
  writeCredentials: token<string>("writeCredentials"),
  readConnection: token<DatabaseConnection>("readConnection"),
  writeConnection: token<DatabaseConnection>("writeConnection"),
  listTodosUseCase: token<ListTodosUseCase>("listTodosUseCase"),
  saveTodoUseCase: token<SaveTodoUseCase>("saveTodoUseCase"),
};

class DatabaseConnection {
  constructor(private credentials: string) {}
}

class ListTodosUseCase {
  constructor(private connection: DatabaseConnection) {}
}

class SaveTodoUseCase {
  constructor(private connection: DatabaseConnection) {}
}

container.bind(TOKENS.readCredentials).toConstant(config.db.credentials.read);
container.bind(TOKENS.writeCredentials).toConstant(config.db.credentials.write);
container.bind(TOKENS.readConnection).toInstance(DatabaseConnection);
container.bind(TOKENS.writeConnection).toInstance(DatabaseConnection);

/**
 * Perfect so far, we can give the different use cases the appropriate connections.
 */
injected(ListTodosUseCase, TOKENS.readConnection);
injected(SaveTodoUseCase, TOKENS.writeConnection);

/**
 * Here we run into an issue, we can't inject different connection strings into the same constructor.
 */
injected(DatabaseConnection, TOKENS.readCredentials);
injected(DatabaseConnection, TOKENS.writeCredentials);

You could say well, just create a read and write connection class and let them implement the same interface. But this defeats the purpose. The implementation of these classes will be the same, resulting in code duplication.

A great thing about some DI containers is that they give you flexibility to have the same class with different arguments injected, registered under different keys. (that's why the injected function should probably also not be in the same file as the class definition, this removes that flexibility).

I first thought conditional bindings with tags would solve this, but they are also bound directly to the constructor. That just moves the issue one level up.

My proposed solution is as follows, let's define the injected arguments per token instead of per constructor:

injected(TOKENS.listTodosUseCase, TOKENS.readConnection);
injected(TOKENS.saveTodoUseCase, TOKENS.writeConnection);

injected(TOKENS.readConnection, TOKENS.readCredentials);
injected(TOKENS.writeConnection, TOKENS.writeCredentials);

I can imagine, having the injected function as a separate function is then not even necessary anymore. We could do something like:

container.bind(TOKENS.readCredentials).toConstant(config.db.credentials.read);
container.bind(TOKENS.writeCredentials).toConstant(config.db.credentials.write);

container
  .bind(TOKENS.readConnection)
  .toInstance(DatabaseConnection);
  .withArguments(TOKENS.readCredentials);

container
  .bind(TOKENS.writeConnection)
  .toInstance(DatabaseConnection)
  .withArguments(TOKENS.writeCredentials);

I think this would greatly improve the flexibility of this library. Let me know what you think!

@vovaspace
Copy link
Owner

vovaspace commented Feb 19, 2022

Hello!

Thanks for the suggestion, this is a very interesting thing. I have understood the problem and will think about how to solve it.

You said "A great thing about some DI containers is that they give you flexibility to…", it would be interesting to look at the implementation in other libraries, possibly in other languages.

@vovaspace vovaspace added the enhancement New feature or request label Feb 19, 2022
@ngerritsen
Copy link
Author

ngerritsen commented Feb 22, 2022

Hello!

Thanks for the suggestion, this is a very interesting thing. I have understood the problem and will think about how to solve it.

You said "A great thing about some DI containers is that they give you flexibility to…", it would be interesting to look at the implementation in other libraries, possibly in other languages.

You could look at PHPLeague's Container library and also Symfony's DI Container allows this behaviour. I happen to also have my own DI container that has this: https://ngerritsen.gitlab.io/containor.

PS. I made a PR to awesome-nodejs to add our DI libraries

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