angular-ts-decorators icon indicating copy to clipboard operation
angular-ts-decorators copied to clipboard

Issues with class inheritance and @ViewParent

Open MartijnWelker opened this issue 5 years ago • 0 comments

I have 2 components which both extend the same base class. One of the 2 classes has a @ViewParent, when running the code in the metadata the @ViewParent is actually also required on the base class. I'm suspecting this is because of using an object reference where a copy should be used in addRequireToMetadata. (I think addBindingToMetadata suffers the same bug, but haven't tested it yet)

Example:

// This component is required by the base class so the base class' `require` is stored as metadata
@Component({
    selector: 'test-component',
})
export class TestComponent {
}

// Abstract class which has a `@ViewParent`, the require metadata will 
// be stored and later polluted by `ComponentA`'s `@ViewParent`
export abstract class ChildClass {
    @ViewParent('testComponent') private testComponentCtrl: TestComponent;
}

// When this class is registered, it will try to register the `ngModel` ViewParent 
// but with the way `addRequireToMetadata` and `Reflect` works 
// it will get the metadata object from `ChildClass` and register the require on there
@Component({
    selector: 'component-a'
    template: '<div>A</div>`
})
class ComponentA extends ChildClass {
    @ViewParent('ngModel') private ngModelCtrl: ng.INgModelController;
}

// This component doesn't require `ngModel`, but because `ChildClass` is polluted it now also requires `ngModel`
@Component({
    selector: 'component-b',
    template: '<div>B</div>
})
class ComponentB extends ChildClass {
}

addRequireToMetadata code:

/** @internal */
function addRequireToMetadata(target, key, controller) {
    var targetConstructor = target.constructor;

    // `getMetadata` will call `OrdinaryHasMetadata` which in turn will call `OrdinaryGetPrototypeOf` returning the 
    // require of `ChildClass` instead of `ComponentA` because `ComponentA`'s prototype is `ChildClass`
    var require = getMetadata(metadataKeys.require, targetConstructor) || {};

    // Here the object reference of the require of `ChildClass` is now used instead 
    // of a copy, so ChildClass will now require `ngModel`
    // What should happen is that if require already exists, a copy of 
    // the object should be made and used for writing
    require[key] = controller;

    // Metadata is stored correctly under `ComponentA`, but the `ChildClass` require 
    // object reference is already polluted
    defineMetadata(metadataKeys.require, require, targetConstructor);
}

Proposed fix:

function addRequireToMetadata(target: any, key: string, controller: string) {
  const targetConstructor = target.constructor;
  const definedMetadata = getMetadata(metadataKeys.require, targetConstructor);
  
  // Make a copy of the already stored metadata, this will fix the bug if it's actually the prototype's metadata
  const require = definedMetadata !== undefined ? {...definedMetadata} : {};

  require[key] = controller;

  defineMetadata(metadataKeys.require, require, targetConstructor);
}

MartijnWelker avatar May 01 '19 08:05 MartijnWelker