Construtor injection is broken for TypeScript and es6

[AlexTugarev]

The constructor injection breaks after switching TypeScript's target from es5 to es6 in a specific case. Consider a constructor of a base class with an optional dependency and a default value, and the subclass' constructor to have more dependencies then the super constructor.

Expected Behavior

Constructor injection should have the same behavior regardless of target in TS (es5/es6), and dependencies are injected as specified.

Current Behavior

Constructor injection fails after switching to es6 target in that particular case, see repro. Only n dependencies are injected, where n is the number of super constructor arguments/dependencies.

Steps to Reproduce (for bugs)

This issue is reproducible with the following minimal example:

import { injectable, inject, Container, optional } from "inversify";
import "reflect-metadata";

// some dependencies
@injectable() export class DepA {a=1}
@injectable() export class DepB {b=1}
@injectable() export class DepC {c=1}

@injectable() export abstract class AbstractCls {
    constructor(@inject(DepA) a: DepA, @inject(DepB) @optional() b: DepB = {b: 0}) {
        console.log(`Expect {"a":1}, Actual: ${JSON.stringify(a)}`);
        console.log(`Expect {"b":1}, Actual: ${JSON.stringify(b)}`);
    }
}

@injectable() export class Cls extends AbstractCls {
    constructor(@inject(DepC) c: DepC, @inject(DepB) @optional() b: DepB = { b: 0}, @inject(DepA) a: DepA) {
        super(a, b);
    }
}

const container = new Container();
[DepA, DepB, DepC, Cls].forEach(i => container.bind(i).toSelf().inSingletonScope());
container.get(Cls);

// TS > es5
// [start] Expect {"a":1}, Actual: {"a":1}
// [start] Expect {"b":1}, Actual: {"b":1}

// TS > es6
// [start] Expect {"a":1}, Actual: undefined
// [start] Expect {"b":1}, Actual: {"b":0}

tsconfig.json

{
  "compilerOptions": {
    "target": "es6",
    "module": "commonjs",
    "lib": ["es6", "dom"],
    "sourceMap": true,
    "outDir": "dist",
    "strict": true,
    "moduleResolution": "node",
    "types": ["reflect-metadata"],
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true
  }
}

Context

In Theia we are planing to use es6 as target for TypeScript. At least one use of constructor injection led to a runtime error as dependencies were injected bogusly. We can workaround, by using same arguments for subclasses and adding property injection for additional dependencies. Nevertheless, the algorithm to compute the dependencies for the subclass seems to be broken after changing TypeScript's target to es6.

Your Environment

TypeScript 2.9.2
Node 8

[remojansen]

Hi @AlexTugarev @dcavanagh and I are looking into this. We need to be able to run all our tests in ES5 and ES6 so we need to do some changes to our CI. Will try to solve this as soon as we can.

[AlexTugarev]

Sounds good, thanks for the feedback!

[pbalaga]

Is there any progress about this problem? I still hit this when targeting ES6 or higher as well and using Inversify 5.0.5. I did some research on my end:

• Inversify determines the number of parameters it has to inject into a constructor by using Function.length - see source code.
• Function.length "excludes the rest parameter and only includes parameters before the first one with a default value". So Inversify thinks there are less parameters to inject than actually defined (params with a default value are not counted). As a result, the parameters with a default value are never injected and will never have a value different from default.
• IMO, class inheritance does not matter here.

[benedyktdryl]

@dcavanagh it seems like this might be a critical bug to be fixed, do you have any WIP for it?

[dcavanagh]

@benedyktdryl no I do not. Are you able to reproduce it?

[dcavanagh]

@benedyktdryl I am able to reproduce it.

[notaphplover]

@dcavanagh I've been playing around, here are my conslussions:

Consider the following test (thanks @AlexTugarev ):

import { expect } from "chai";
import { Container, inject, injectable, optional } from "../../src/inversify";


describe("Issue 928", () => {

  it('should get 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());
  });
});

How is this code compiled?

Compilation depends on the Typescript target. I'll share with you a small fragment of the class Cls:

On ES5:

var Cls = (function (_super) {
            __extends(Cls, _super);
            function Cls(c, b, a) {
                if (b === void 0) { b = { b: 0 }; }
                var _this = _super.call(this, a, b) || this;
                injectedC = c;
                return _this;
            }
            Cls = __decorate([
                inversify_1.injectable(),
                __param(0, inversify_1.inject(DepC)), __param(1, inversify_1.inject(DepB)), __param(1, inversify_1.optional()), __param(2, inversify_1.inject(DepA)),
                __metadata("design:paramtypes", [DepC, DepB, DepA])
            ], Cls);
            return Cls;
        }(AbstractCls));

On ES6:

let Cls = class Cls extends AbstractCls {
  constructor(c, b = { b: 0 }, a) {
    super(a, b);
    injectedC = c;
  }
Cls = __decorate([ 
  inversify_1.injectable(),
  __param(0, inversify_1.inject(DepC)), __param(1, inversify_1.inject(DepB)), __param(1, inversify_1.optional()), __param(2, inversify_1.inject(DepA)),
  __metadata("design:paramtypes", [DepC, DepB, DepA])
  ], Cls);
};

Keep in mind these functions are different.

• On ES5, Cls.length === 3.
• On ES6, Cls.length === 1 (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/length) for the details

Why does it matter?

If we look at getTargets at reflection_utils.ts:

    const keys = Object.keys(constructorArgsMetadata);
    const hasUserDeclaredUnknownInjections = (func.length === 0 && keys.length > 0);
    const iterations = (hasUserDeclaredUnknownInjections) ? keys.length : func.length;

On ES6 only one iteration is done instead of three, so only the first property is injected!

[notaphplover]

@dcavanagh I've created a draft PR which would solve this issue :)

[dcavanagh]

Closed by #1308