Only emit ctorParameters when the class has a constructor

[deadalusai]

Fix DecoratorClassVisitor to only emit ctorParameters when the class has a constructor.

This more closely matches the behavior of TSC which only emits the "design:paramtypes" metadata
when the class actually has a constructor. For example, compiling the following TypeScript snippet:

// compile with --target ES5 --experimentalDecorators --emitDecoratorMetadata
@Inject
class Bar {

}

@Inject
class Baz {
    constructor() {}
}

@Inject
class Foo {
    constructor(bar: Bar) {}
}

class Faz {
    constructor(@Inject bar: Bar) {}
}

Emits this JavaScript:

var Bar = /** @class */ (function () {
    function Bar() {
    }
    Bar = __decorate([
        Inject
    ], Bar);
    return Bar;
}());
var Baz = /** @class */ (function () {
    function Baz() {
    }
    Baz = __decorate([
        Inject,
        __metadata("design:paramtypes", [])
    ], Baz);
    return Baz;
}());
var Foo = /** @class */ (function () {
    function Foo(bar) {
    }
    Foo = __decorate([
        Inject,
        __metadata("design:paramtypes", [Bar])
    ], Foo);
    return Foo;
}());
var Faz = /** @class */ (function () {
    function Faz(bar) {
    }
    Faz = __decorate([
        __param(0, Inject),
        __metadata("design:paramtypes", [Bar])
    ], Faz);
    return Faz;
}());

[mprobst]

@deadalusai can you share what problem exactly this fixes?

I'm not sure if we have code (e.g. in Angular) that might depend on these empty constructors params being produced. Evan might know.

[deadalusai]

Hi Martin. Sorry for the curt PR - I meant to have a PR up for @angular/angular with links for context, but ran out of time yesterday.

I'm working to resolve an issue I found compiling though ng-packagr, where the constructor function generated for a subclass with an implicit constructor does not play nicely with the regex used to detect such a thing in Angular's ReflectionCapabilities._ownParameters.

Specifically, ng-packagr generates a "pass-through" constructor which does not match the regex on line 71, e.g.

function FooComponent () {
    __super.apply(this, __tslib.__spread(arguments));
}

Angular is expecting something like:

function FooComponent () {
    __super.apply(this, arguments);
}

tsickle has generated the static ctorParameters, but set it to an empty array. The reason this works is because Angular already had a work-around (the regex check) for a similar TypeScript issue (since resolved).

The problem is that the work-around is currently the "official" way to detect this edge case. My goal here is to bring tsickles behavior in line with tsc (do not generate ctorParameters metadata when the class does not have an explicit constructor). The next change will be to clean up the logic in Angular's ReflectionCapabilities to eliminate the reliance on that brittle regex check:

// Pseudocode
function getConstructorParams (typeConstructor) {
    // legacy regex check, for old versions of tsc
    if (DELEGATE_CTOR.test ... etc) {
        return null;
    }

    // check for tsickle metadata
    if (typeConstructor.ctorParameters) {
        return typeConstructor.ctorParameters;
    }

    let metadata = this._reflect.getOwnMetadata('design:paramtypes', typeConstructor);
    if (metadata) {
        return metadata;
    }

    // pass-through constructor, or no metadata available
    return null;
}

// the type either has a real constructor...
let parameters = getConstructorParams(constructorType);
if (!parameters && parentType !== Object) {
    // ...or we should use the parent constructor
    parameters = getConstructorParams(parentType);
}
return parameters || [];

The regex would be left behind as a legacy check to support code generated with old versions of tsc or tsickle, but would no longer be part of the happy path.

(The regex check has already grown from one to three different regexes in the last few minor versions of Angular)

[deadalusai]

Just realized my examples above are missing the most important case of a subclass with an implicit constructor. Here's what tsc currently does:

@Injectable
class Base {
    constructor (d: Dependency) {}
}

@Injectable
class Sub extends Base {
}

Generating:

var Base = /** @class */ (function () {
    function Base(d) {
    }
    Base = __decorate([
        Injectable,
        __metadata("design:paramtypes", [Dependency])
    ], Base);
    return Base;
}());
var Sub = /** @class */ (function (_super) {
    __extends(Sub, _super);
    function Sub() {
        return _super !== null && _super.apply(this, arguments) || this;
    }
    Sub = __decorate([
        Injectable
        // NOTE: no "design:paramtypes" metadata for the subclass
    ], Sub);
    return Sub;
}(Base));

I'd like tsickle to do the same.

[mprobst]

@mhevery can you figure out who's a good person to coordinate this change? I don't quite understand the interaction and timelines of the components involved. Can you assign a reviewer?

[deadalusai]

The Angular PR can be found here: angular/angular#22642

In the end I do not think this approach will work satisfactorily, however I will leave the PRs up for the time being as another dev may be able to see a way to detect and handle the edge case.

Might have found a solution for the problem I had. Feedback welcome.

[mhevery]

This is either @evmar or @mprobst

[mhevery]

So I looked into it, and the PRs seems reasonable, but it seems like we would have to coordinate angular/angular#22642 with this PR. This seems like an awful lot of work, along with supporting the legacy. Could we just fix ng-packagr/ng-packagr#679 to generate code which would pass current angular checks?

[mhevery]

I gave it some more thought and my recommendation would be to do nothing and wait for angular/angular#21706 to land which will make the whole ReflectionCapabilities obsolete, since that will completely remove the need for tsickle.

[trotyl]

@mhevery It’s only dropped in core, but requiring JitCompiler to provide the same functionality . And in Ivy this issue would become more severe due to the official support for consuming AOT'd libs in JIT.

[mhevery]

@trotyl I am not sure I understand your comment. Can you expand on it?

[trotyl]

@mhevery In Ivy the metadata constructor dependencies would be reflected in .n()/.factory() definition properties, like:

@Injectable()
class BaseComp {
  constructor(myService: MyService) { }
}

@Component({ ... })
class MyComp extends BaseComp { }

Would become:

class MyComp extends BaseComp {
  static ngComponentDef = defineComponent({
    factory: () => new MyComp(inject(MyService))
  })
}

So the ReflectionCapabilities in core (runtime) can indeed be removed. But then compiler must be able to determine whether some class has deps defined in ancestor classes.

If we don't have RegExp match any more and without this change in tsickle, then:

In AOT mode, nothing changed in metadata resolution since everything controlled by compiler-cli;
In JIT mode, libraries are preprocessed by ngc and deps are recorded in ctorParameters static property.

Imagine that the classes above comes from lib, with the format:

class BaseComp { }
BaseComp.ctorParameters = () => [ { type: MyService } ]

class MyComp extends BaseComp { }
MyComp.ctorParameters = () => []

Then in a JIT'ed Ivy app:

import { MyComp } from 'my-lib' 

class CustomComp extends MyComp { }

According to the inheritance semantics the deps for CustomComp should be [MyService], but once found empty [] on its base class MyComp, it would then think about MyComp has a deps overwrite, then make the deps to be [].

This case may only happen to back patch mode since I'm not sure how does pure Ivy lib looks like, metadata inheritance in Ivy seems not implemented yet, but for supporting JIT usage, there still needs to be record stands for ctorParameters in emitted JavaScript files.

[deadalusai]

tsickle could inherit ctorParams on a derived class explicitly. I.e.

class MyComp extends BaseComp { }
MyComp.ctorParameters = BaseComp.ctorParameters;

Which (for an implicit constructor) should always be correct? This would eliminate the need to walk up the class hierarchy entirely. Unfortunately tsc does not do this for design:paramtypes (though I think it should).

[mhevery]

I see, thanks for the explanation, I see it better now. In which case I think I am for landing this. after angular/angular#22642 lands.

[mhevery]

Could we get this rebased on latest master?

[deadalusai]

@mhevery I can rebase this today. Were you interested in this PR making tsickle emit the base classes' ctorParameters` for types with implicit constructors? E.g.

class BaseComp { }
BaseComp.ctorParameters = () => [ { type: MyService } ]

class MyComp extends BaseComp { }
MyComp.ctorParameters = BaseComp.ctorParameters;

The behavior would be different to tsc again, but I think more useful.

[deadalusai]

Rebased 👍

[mprobst]

Merged, thanks for the contribution.

[LinboLen]

does it releated to https://github.com/angular/angular/blob/master/packages/core/src/reflection/reflection_capabilities.ts#L63-L74 ?