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

Using ContainerInstance in the service constructor causes TypeScript build breaks #859

Open
zavarka opened this issue Aug 23, 2022 · 1 comment

Comments

@zavarka
Copy link

zavarka commented Aug 23, 2022

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch [email protected] for the project I'm working on.

When instantiating services, the last parameter passed to the constructor is the container instance itself, allowing usage like this (copied from the TypeDI documentation):

@Service()
class CoffeeMaker {
  beanFactory: Factory;
  sugarFactory: Factory;
  waterFactory: Factory;

  constructor(container) {
    this.beanFactory = container.get('bean.factory');
    this.sugarFactory = container.get('sugar.factory');
    this.waterFactory = container.get('water.factory');
  }

Sometimes it is convenient to pass the container instance through various function calls instead of specific services individually, so the above pattern is helpful. But upgrading from [email protected] introduces the following TypeScript build break:

Service with "MaybeConstructable<ContainerInstance>" identifier was not found in the container. 
Register it before usage via explicitly calling the "Container.set" function or using the 
"@Service()" decorator.

The cause is that, when initializing the parameter types, typedi does not account for ContainerInstance being one of the possible types, and so the injection fails.

Here is the diff that solved my problem:

diff --git a/node_modules/typedi/cjs/container-instance.class.js b/node_modules/typedi/cjs/container-instance.class.js
index e473b1e..c0e4ce6 100644
--- a/node_modules/typedi/cjs/container-instance.class.js
+++ b/node_modules/typedi/cjs/container-instance.class.js
@@ -250,6 +250,8 @@ class ContainerInstance {
             });
             if (paramHandler)
                 return paramHandler.value(this);
+            if (paramType && paramType.name === ContainerInstance.name)
+                return this;
             if (paramType && paramType.name && !this.isPrimitiveParamType(paramType.name)) {
                 return this.get(paramType);
             }
diff --git a/node_modules/typedi/esm2015/container-instance.class.js b/node_modules/typedi/esm2015/container-instance.class.js
index 4b75cb4..891e094 100644
--- a/node_modules/typedi/esm2015/container-instance.class.js
+++ b/node_modules/typedi/esm2015/container-instance.class.js
@@ -247,6 +247,8 @@ export class ContainerInstance {
             });
             if (paramHandler)
                 return paramHandler.value(this);
+            if (paramType && paramType.name === ContainerInstance.name)
+                return this;
             if (paramType && paramType.name && !this.isPrimitiveParamType(paramType.name)) {
                 return this.get(paramType);
             }
diff --git a/node_modules/typedi/esm5/container-instance.class.js b/node_modules/typedi/esm5/container-instance.class.js
index 56df7eb..0c9e9fc 100644
--- a/node_modules/typedi/esm5/container-instance.class.js
+++ b/node_modules/typedi/esm5/container-instance.class.js
@@ -261,6 +261,8 @@ var ContainerInstance = /** @class */ (function () {
             });
             if (paramHandler)
                 return paramHandler.value(_this);
+            if (paramType && paramType.name === ContainerInstance.name)
+                return _this;
             if (paramType && paramType.name && !_this.isPrimitiveParamType(paramType.name)) {
                 return _this.get(paramType);
             }

This issue body was partially generated by patch-package.

@freshgum-bubbles
Copy link

A more robust implementation would be to check paramType === ContainerInstance -- yes, this is definitely something which should be included by default! It's in my fork as HostContainer -- bear in mind that what you're doing here is the service locator pattern, which is generally discouraged in DI.

Here's a repro of a simpler solution: just use Container.set 🤷 https://stackblitz.com/edit/typescript-uhx5bs?file=index.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants