Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ref #203 - Inheritance issues #207

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mtraynham
Copy link

Please see #203 for the overall issue. I'll summarize changes here:

propMetadata

Previously class property metadata was not being inherited by child classes from parents. For example:

class A { @Input() public foo: any; }
class B extends A { @Input() public bar: any; }

If you requested propMetadata for class B, you would only get bar. The first commit of this overlays all parent properties. A simple Object.assign should suffice, because we should expect all parent keys to typically be different from the child keys.

class A { @Input() public foo: any; }
class B extends A { public foo: any; } // this is not really appropriate

parameters

Previously class constructor decorators were being initialized with their parent constructor parameters. Reflect.getMetadata will return the parent class stored item if the child does not yet exist in the cache. Because of this, the ParamDecorator would get the parent array reference for the first time seeing a child class. It would then manipulate the same reference, and set it back in Reflect with the child's class key.

After all decorates were processed, the Reflect cache would end up with the same constructor decorator array reference for a parent and all it's children, albeit it may be stored at different keys (AConstructor, BConstructor, CConstructor, etc...). This means that any class within a hierarchy has ultimately added to an ever growing single set of decorators. A child could end up with 5 or 6 different @Inject tokens on a single parameter.

After thinking about it for a while, I don't think initializing with the parent is correct. The constructor of a child could be expressed in a completely different layout from the parent.

class A { constructor (@Inject('form') form: any, @Inject('$http') $http: any) { .. } }
class B extends A { constructor ($http: any, form: any) { .. } }

It seems more appropriate to store the class by itself, and if the constructor was empty, it does access it's parents.

@aciccarello
Copy link
Collaborator

Nice PR @mtraynham 👍 . That metadata stuff is a bit complicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants