ngParty / ng-metadata

Angular 2 decorators and utils for Angular 1.x
https://hotell.gitbooks.io/ng-metadata/content/
MIT License
355 stars 46 forks source link

Component Class Inheritance & Decorator bugs #203

Open mtraynham opened 7 years ago

mtraynham commented 7 years ago

As a preamble on how I came across these bugs, I have a form that uses a variety of components, from string inputs to selects, etc. Some of the components may be more custom than others, but they all share a set of input/output bindings and may share some @Host components.

I tried this with both core-js and reflect-metadata with the same results.

The two issues I found with Plunkr and examples on how to recreate:


@Input (and potentially @Output) decorators are not inherited if the child component class specifies it's own @Input.

The code below has:

  1. an AbstractComponent with @Input a,
  2. a child class AComponent which correctly inherits that @Input a
  3. a child class BComponent which creates @Input b, and does not inherit @Input a

The expected output of this code should be a string, a a b. The actual output is just a b

Here is this bugs Plunkr example (all code is in app.module.ts).

import {Component, Input, NgModule} from 'ng-metadata/core';
import {platformBrowserDynamic} from 'ng-metadata/platform-browser-dynamic';

abstract class AbstractComponent {
    @Input('@') public a: string;
}

@Component({
    selector: 'aComponent',
    template: '<span>{{$ctrl.a}}</span>'
})
export class AComponent extends AbstractComponent  { }

@Component({
    selector: 'bComponent',
    template: '<span>{{$ctrl.a}} {{$ctrl.b}}</span>'
})
export class BComponent extends AbstractComponent  {
    @Input('@') public b: string;
}

@Component({
    selector: 'app',
    template: `
        <a-component a="a"></a-component>
        <b-component a="a" b="b"></b-component>
    `
})
export class AppComponent { }

@NgModule({
    declarations: [AComponent, BComponent, AppComponent],
    exports: [AppComponent],
    bootstrap: [AppComponent]
})
export default class AppModule { }

platformBrowserDynamic().bootstrapModule(AppModule);

@Host (and potentially @Self/@SkipSelf) constructor decorators incorrectly get applied to the parent class, adding parameter information to all child classes.

The code below has:

  1. an AbstractComponent with constructor @Inject('form') @Host of an IFormController,
  2. a child class AComponent which also @Inject('$http'), the IHttpService
  3. a child class BComponent which also @Inject('app') @Host the parent AppComponent.

I believe the BComponent's @Host on the second parameter is getting applied to every second parameter of any child of AbstractComponent and thus the $http services becomes a required directive.

Here is this bugs Plunkr example (all code is in app.module.ts).

This actually breaks Angular with an error logged as:

VM71 angular.js:14525 Error: [$compile:ctreq] Controller '$http', required by directive 'aComponent', can't be found!
http://errors.angularjs.org/1.6.4/$compile/ctreq?p0=%24http&p1=aComponent
    at eval (VM71 angular.js:66)
    at getControllers (VM71 angular.js:9896)
    at getControllers (VM71 angular.js:9903)
    at nodeLinkFn (VM71 angular.js:9796)
    at compositeLinkFn (VM71 angular.js:9055)
    at nodeLinkFn (VM71 angular.js:9809)
    at compositeLinkFn (VM71 angular.js:9055)
    at nodeLinkFn (VM71 angular.js:9809)
    at compositeLinkFn (VM71 angular.js:9055)
    at nodeLinkFn (VM71 angular.js:9809)
import {IFormController, IHttpService} from 'angular';
import {Component, Host, Inject, NgModule} from 'ng-metadata/core';
import {platformBrowserDynamic} from 'ng-metadata/platform-browser-dynamic';

abstract class AbstractComponent {
    private formController: IFormController;

    constructor (@Inject('form') @Host() formController: IFormController) {
        this.formController = formController;
    }
}

@Component({
    selector: 'aComponent'
})
export class AComponent extends AbstractComponent  {
    private $http: IHttpService;

    constructor (@Inject('form') @Host() formController: IFormController,
                 @Inject('$http') $http: IHttpService) {
        super(formController);
        this.$http = $http;
    }
}

@Component({
    selector: 'bComponent'
})
export class BComponent extends AbstractComponent  {
    private app: AppComponent;

    constructor (@Inject('form') @Host() formController: IFormController,
                 @Inject('app') @Host() app: AppComponent) {
        super(formController);
        this.app = app;
    }
}

@Component({
    selector: 'app',
    template: `
        <form>
            <a-component></a-component>
            <b-component></b-component>
        </form>
    `
})
export class AppComponent { }

@NgModule({
    declarations: [AComponent, BComponent, AppComponent],
    exports: [AppComponent],
    bootstrap: [AppComponent]
})
export default class AppModule { }

platformBrowserDynamic().bootstrapModule(AppModule);
mtraynham commented 7 years ago

I've been looking more at this. Angular 2 had an issue and relevant commit for this as well, which seems like it makes no upstream changes to either reflect library.

For propMetadata, they are checking if the class extends another class and merging all property metadata fields here.

For parameters, It looks like they are collecting the annotations themselves for only the child class and seperately for the parent here, deduping as necessary.

Here's a simplified version of debugging this...

import 'core-js/es7/reflect';
import {reflector} from 'ng-metadata/src/core/reflection/reflection';
import {Input} from 'ng-metadata/src/core/directives/decorators';
import {Host, Inject} from 'ng-metadata/src/core/di/decorators';

abstract class AbstractPropertyComponent {
    @Input() public a: string;
}

export class APropertyComponent extends AbstractPropertyComponent  {
    @Input() public b: string;
}

const aPropMetadata: {[p: string]: any[]} = reflector.propMetadata(APropertyComponent);
console.log(aPropMetadata);

abstract class AbstractConstructorComponent {
    private form: any;
    constructor (@Inject('form') @Host() form: any) {
        this.form = form;
    }
}

class AConstructorComponent extends AbstractConstructorComponent {
    private $http: any;
    constructor (@Inject('form') @Host() form: any,
                 @Inject('$http') $http: any) {
        super(form);
        this.$http = $http;
    }
}

class BConstructorComponent extends AbstractConstructorComponent {
    private app: any;
    constructor (@Inject('form') @Host() form: any,
                 @Inject('app') @Host() app: any) {
        super(form);
        this.app = app;
    }
}

const aConstructorAnnotations: any[][] = reflector.parameters(AConstructorComponent);
console.log(aConstructorAnnotations);
const bConstructorAnnotations: any[][] = reflector.parameters(BConstructorComponent);
console.log(bConstructorAnnotations);