Fix injection of optional properties (#1308)

* fix: update getTargets to correclty get constructorTargets when a constructor has optional arguments * chore: update ci with es6 ci test jobs * chore: add typescript config that targets es6 * style: add missing line feed * style: add missing line feed * docs: update changelog * refactor: move e6 tsconfig with the other ts config files * refactor tests for es6 es config updated for es6 ci for es6 test Co-authored-by: Dan Cavanagh <[email protected]>
inversify · Apr 17, 2021 · d72de9b · d72de9b
1 parent 784fec7
commit d72de9b
Show file tree

Hide file tree

Showing 12 changed files with 136 additions and 70 deletions.
diff --git a/.github/workflows/node.js.yml b/.github/workflows/node.js.yml
@@ -31,6 +31,10 @@ jobs:
     strategy:
       matrix:
         node-version: [10.x, 12.x, 14.x, 15.x]
+        ts-project: [src/tsconfig.json, src/tsconfig-es6.json]
+
+    env:
+      TS_NODE_PROJECT: ${{ matrix.ts-project }}
 
     steps:
     - uses: actions/checkout@v2

diff --git a/.gitignore b/.gitignore
@@ -38,6 +38,7 @@ dts
 lib
 temp
 es
+es6
 amd
 
 type_definitions/inversify/*.js

diff --git a/.vscode/launch.json b/.vscode/launch.json
@@ -10,21 +10,18 @@
             "name": "Mocha Tests",
             "program": "${workspaceRoot}/node_modules/mocha/bin/_mocha",
             "args": [
+                "-r",
+                "ts-node/register",
                 "--require",
                 "reflect-metadata",
-                "-u",
-                "tdd",
                 "--timeout",
                 "999999",
                 "--colors",
-                "${workspaceRoot}/test/**/*.test.js"
+                "${workspaceRoot}/test/**/*.test.ts"
             ],
-            "sourceMaps": true,
-            "outFiles": [
-                "${workspaceRoot}/test",
-                "${workspaceRoot}/src"
-            ],
-            "internalConsoleOptions": "openOnSessionStart"
+            "console": "integratedTerminal",
+            "internalConsoleOptions": "neverOpen",
+            "protocol": "inspector"
         },
         {
             "type": "node",
@@ -49,4 +46,4 @@
             "console": "internalConsole",
         }
     ]
-}
+}
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 - Fix `Target.isTagged()` to exclude `optional` from tag injections #1190.
 - Update `toConstructor`, `toFactory`, `toFunction`, `toAutoFactory`, `toProvider` and `toConstantValue` to have singleton scope #1297.
+- Fix injection on optional properties when targeting ES6 #928
 
 ## [5.0.1] - 2018-10-17
 ### Added 

diff --git a/package.json b/package.json
@@ -6,10 +6,11 @@
   "jsnext:main": "es/inversify.js",
   "types": "lib/inversify.d.ts",
   "scripts": {
-    "build": "npm run build:lib && npm run build:amd && npm run build:es",
+    "build": "npm run build:lib && npm run build:amd && npm run build:es && npm run build:es6",
     "build:lib": "tsc -p src/tsconfig.json",
     "build:amd": "tsc -p src/tsconfig-amd.json",
     "build:es": "tsc -p src/tsconfig-es.json",
+    "build:es6": "tsc -p src/tsconfig-es6.json",
     "clean": "rm -r amd es lib",
     "pretest": "tslint --project .",
     "test": "nyc --require ts-node/register mocha test/**/*.test.ts --reporter spec --retries 3 --require 'node_modules/reflect-metadata/Reflect.js' --exit",

diff --git a/src/planning/reflection_utils.ts b/src/planning/reflection_utils.ts
@@ -34,7 +34,9 @@ function getTargets(
 
     const keys = Object.keys(constructorArgsMetadata);
     const hasUserDeclaredUnknownInjections = (func.length === 0 && keys.length > 0);
-    const iterations = (hasUserDeclaredUnknownInjections) ? keys.length : func.length;
+    const hasOptionalParameters = keys.length > func.length;
+
+    const iterations = (hasUserDeclaredUnknownInjections || hasOptionalParameters) ? keys.length : func.length;
 
     // Target instances that represent constructor arguments to be injected
     const constructorTargets = getConstructorArgsAsTargets(

diff --git a/src/tsconfig-es6.json b/src/tsconfig-es6.json
@@ -0,0 +1,10 @@
+{
+    "include": [
+        "./**/*.ts"
+    ],
+    "extends": "../tsconfig.json",
+    "compilerOptions": {
+        "target": "es6",
+        "outDir": "../es6"
+    },
+}
diff --git a/test/bugs/issue_543.test.ts b/test/bugs/issue_543.test.ts
@@ -14,39 +14,44 @@ describe("Issue 543", () => {
             Root: Symbol.for("Root")
         };
 
+        interface IIrrelevant {}
+        interface ICircular{}
+        interface IChild {}
+        interface IChild2 {}
+
         @injectable()
-        class Irrelevant {}
+        class Irrelevant implements IIrrelevant {}
 
         @injectable()
-        class Child2 {
-            public circ: Circular;
+        class Child2 implements IChild2 {
+            public circ: ICircular;
             public constructor(
-                @inject(TYPE.Circular) circ: Circular
+                @inject(TYPE.Circular) circ: ICircular
             ) {
                 this.circ = circ;
             }
         }
 
         @injectable()
-        class Child {
-            public irrelevant: Irrelevant;
-            public child2: Child2;
+        class Child implements IChild {
+            public irrelevant: IIrrelevant;
+            public child2: IChild2;
             public constructor(
-                @inject(TYPE.Irrelevant) irrelevant: Irrelevant,
-                @inject(TYPE.Child2) child2: Child2
+                @inject(TYPE.Irrelevant) irrelevant: IIrrelevant,
+                @inject(TYPE.Child2) child2: IChild2
             ) {
                 this.irrelevant = irrelevant;
                 this.child2 = child2;
             }
         }
 
         @injectable()
-        class Circular {
-            public irrelevant: Irrelevant;
-            public child: Child;
+        class Circular implements Circular {
+            public irrelevant: IIrrelevant;
+            public child: IChild;
             public constructor(
-                @inject(TYPE.Irrelevant) irrelevant: Irrelevant,
-                @inject(TYPE.Child) child: Child
+                @inject(TYPE.Irrelevant) irrelevant: IIrrelevant,
+                @inject(TYPE.Child) child: IChild
             ) {
                 this.irrelevant = irrelevant;
                 this.child = child;
@@ -55,11 +60,11 @@ describe("Issue 543", () => {
 
         @injectable()
         class Root {
-            public irrelevant: Irrelevant;
-            public circ: Circular;
+            public irrelevant: IIrrelevant;
+            public circ: ICircular;
             public constructor(
-                @inject(TYPE.Irrelevant) irrelevant1: Irrelevant,
-                @inject(TYPE.Circular) circ: Circular
+                @inject(TYPE.Irrelevant) irrelevant1: IIrrelevant,
+                @inject(TYPE.Circular) circ: ICircular
             ) {
                 this.irrelevant = irrelevant1;
                 this.circ = circ;

diff --git a/test/bugs/issue_549.test.ts b/test/bugs/issue_549.test.ts
@@ -10,21 +10,24 @@ describe("Issue 549", () => {
             BDynamicValue: Symbol.for("BDynamicValue")
         };
 
+        interface IA {}
+        interface IB {}
+
         @injectable()
         class A {
-            public b: B;
+            public b: IB;
             public constructor(
-                @inject(TYPE.BDynamicValue)  b: B
+                @inject(TYPE.BDynamicValue)  b: IB
             ) {
                 this.b = b;
             }
         }
 
         @injectable()
         class B {
-            public a: A;
+            public a: IA;
             public constructor(
-                @inject(TYPE.ADynamicValue) a: A
+                @inject(TYPE.ADynamicValue) a: IA
             ) {
                 this.a = a;
             }

diff --git a/test/bugs/issue_928.test.ts b/test/bugs/issue_928.test.ts
@@ -0,0 +1,42 @@
+import { expect } from "chai";
+import { Container, inject, injectable, optional } from "../../src/inversify";
+
+
+describe("Issue 928", () => {
+
+  it('should inject the right instances', () => {
+
+    let injectedA: unknown;
+    let injectedB: unknown;
+    let injectedC: unknown;
+
+    // some dependencies
+    @injectable() class DepA { a = 1 }
+    @injectable() class DepB { b = 1 }
+    @injectable() class DepC { c = 1 }
+
+    @injectable() abstract class AbstractCls {
+      constructor(@inject(DepA) a: DepA, @inject(DepB) @optional() b: DepB = {b: 0}) {
+        injectedA = a;
+        injectedB = b;
+      }
+    }
+
+    @injectable() class Cls extends AbstractCls {
+      constructor(@inject(DepC) c: DepC, @inject(DepB) @optional() b: DepB = { b: 0 }, @inject(DepA) a: DepA) {
+        super(a, b);
+
+        injectedC = c;
+      }
+    }
+
+    const container = new Container();
+    [DepA, DepB, DepC, Cls].forEach(i => container.bind(i).toSelf().inSingletonScope());
+
+    container.get(Cls);
+
+    expect(injectedA).to.deep.eq(new DepA());
+    expect(injectedB).to.deep.eq(new DepB());
+    expect(injectedC).to.deep.eq(new DepC());
+  });
+});
diff --git a/test/node/exceptions.test.ts b/test/node/exceptions.test.ts
@@ -6,39 +6,39 @@ describe("Node", () => {
 
     it("Should throw if circular dependencies found", () => {
 
-        interface A {}
-        interface B {}
-        interface C {}
-        interface D {}
+        interface IA {}
+        interface IB {}
+        interface IC {}
+        interface ID {}
 
         @injectable()
-        class A implements A {
-            public b: B;
-            public c: C;
+        class A implements IA {
+            public b: IB;
+            public c: IC;
             public constructor(
-                @inject("B")  b: B,
-                @inject("C")  c: C
+                @inject("B")  b: IB,
+                @inject("C")  c: IC,
             ) {
                 this.b = b;
                 this.c = c;
             }
         }
 
         @injectable()
-        class B implements B {}
+        class B implements IB {}
 
         @injectable()
-        class C implements C {
-            public d: D;
-            public constructor(@inject("D") d: D) {
+        class C implements IC {
+            public d: ID;
+            public constructor(@inject("D") d: ID) {
                 this.d = d;
             }
         }
 
         @injectable()
-        class D implements D {
-            public a: A;
-            public constructor(@inject("A") a: A) {
+        class D implements ID {
+            public a: IA;
+            public constructor(@inject("A") a: IA) {
                 this.a = a;
             }
         }

diff --git a/test/planning/planner.test.ts b/test/planning/planner.test.ts
@@ -120,41 +120,41 @@ describe("Planner", () => {
 
     it("Should throw when circular dependencies found", () => {
 
-        interface A { }
-        interface B { }
-        interface C { }
-        interface D { }
+        interface IA { }
+        interface IB { }
+        interface IC { }
+        interface ID { }
 
         @injectable()
-        class D implements D {
-            public a: A;
+        class D implements ID {
+            public a: IA;
             public constructor(
-                @inject("A") a: A
+                @inject("A") a: IA
             ) { // circular dependency
                 this.a = a;
             }
         }
 
         @injectable()
-        class C implements C {
-            public d: D;
+        class C implements IC {
+            public d: ID;
             public constructor(
-                @inject("D") d: D
+                @inject("D") d: ID
             ) {
                 this.d = d;
             }
         }
 
         @injectable()
-        class B implements B { }
+        class B implements IB { }
 
         @injectable()
-        class A implements A {
-            public b: B;
-            public c: C;
+        class A implements IA {
+            public b: IB;
+            public c: IC;
             public constructor(
-                @inject("B") b: B,
-                @inject("C") c: C
+                @inject("B") b: IB,
+                @inject("C") c: IC
             ) {
                 this.b = b;
                 this.c = c;
@@ -167,10 +167,10 @@ describe("Planner", () => {
         const dId = "D";
 
         const container = new Container();
-        container.bind<A>(aId).to(A);
-        container.bind<B>(bId).to(B);
-        container.bind<C>(cId).to(C);
-        container.bind<D>(dId).to(D);
+        container.bind<IA>(aId).to(A);
+        container.bind<IB>(bId).to(B);
+        container.bind<IC>(cId).to(C);
+        container.bind<ID>(dId).to(D);
 
         const throwErrorFunction = () => {
             container.get(aId);