Fix injection of optional properties

[notaphplover]

Description

This PR updates the planner to get the right bindings when a constructor has optional parameters and adds ES6 ci test jobs to ensure the library works fine when ES6 is the Typescript compilation target.

Related Issue

#928

Motivation and Context

A constructor with injected rest parameters won't be injected with those rest parameters injection. Typescript users who targets ECMAScript 6 could have issues because of this.

How Has This Been Tested?

A test case have been added.
New ci test jobs have been added.

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.

[PodaruDragos]

do we need unknown here ?

[notaphplover]

I think yes. If you try to run the tests targeting ES6 and you do not change typings, you get the folloing error:

ReferenceError: Cannot access 'Circular' before initialization
      at Context.<anonymous> (test/bugs/issue_543.test.ts:24:46)
      at processImmediate (internal/timers.js:456:21)

I think we can change the type because at the end it's sugar syntax. What we really want to check is we are able to detect a circular dependency but maybe I'm missing something

[dcavanagh]

I was working on this yesterday and rewrote the tests I will try to add just that part

[PodaruDragos]

this looks fine to me, but maybe we can find a way to actually keep types as types there in tests.
I guess we can do that later

[notaphplover]

this looks fine to me, but maybe we can find a way to actually keep types as types there in tests.
I guess we can do that later

Yeah, I'd love to find a way. I've been compiling the code targeting es6 modules and targeting es5:

The following code fragment:

@injectable()
class Child2 {
    public circ: Circular;
    public constructor(
        @inject(TYPE.Circular) circ: Circular
    ) {
        this.circ = circ;
    }
}
...
@injectable()
class Circular {
    public irrelevant: unknown;
    public child: unknown;
    public constructor(
        @inject(TYPE.Irrelevant) irrelevant: Irrelevant,
        @inject(TYPE.Child) child: Child
    ) {
        this.irrelevant = irrelevant;
        this.child = child;
    }
}

Is compiled into this fragment: (ES5)

var Child2 = (function () {
    function Child2(circ) {
        this.circ = circ;
    }
    Child2 = __decorate([
        inversify_1.injectable(),
        __param(0, inversify_1.inject(TYPE.Circular)),
        __metadata("design:paramtypes", [Circular])
    ], Child2);
    return Child2;
}());
...
var Circular = (function () {
    function Circular(irrelevant, child) {
        this.irrelevant = irrelevant;
        this.child = child;
    }
    Circular = __decorate([
        __param(0, inversify_1.inject(TYPE.Irrelevant)),
        __param(1, inversify_1.inject(TYPE.Child)),
        __metadata("design:paramtypes", [Irrelevant, Child])
    ], Circular);
    return Circular;
}());

So we are "fine" thanks to the var scope (even if metadata is not being decorated properly).

Let's see the same compiled fragment on es6:

let Child2 = class Child2 {
    constructor(circ) {
        this.circ = circ;
    }
};
Child2 = __decorate([
    inversify_1.injectable(),
    __param(0, inversify_1.inject(TYPE.Circular)),
    __metadata("design:paramtypes", [Circular])
], Child2);
...
let Circular = class Circular {
    constructor(irrelevant, child) {
        this.irrelevant = irrelevant;
        this.child = child;
    }
};
Circular = __decorate([
    __param(0, inversify_1.inject(TYPE.Irrelevant)),
    __param(1, inversify_1.inject(TYPE.Child)),
    __metadata("design:paramtypes", [Irrelevant, Child])
], Circular);

let scope is being used instead, so it won't work.

Since we need to emit class metadata and one class must be the first one (splitting classes in multiple files wouln't work the problem since we would be creating a circular dependency), I wonder if we can solve this :( without the unknown workaround.

[notaphplover]

❤️

[notaphplover]

this looks fine to me, but maybe we can find a way to actually keep types as types there in tests.
I guess we can do that later

At the @dcavanagh was able to find a way though interfaces 😃

[dcavanagh]

Fixes #928