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

Problem with bindings of child classes being set on the parent class resulting in polluted bindings #95

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

Conversation

coolblades
Copy link

We have been using this for 3 months now, and like it so far - it is our intermediate step to go from ng1.5 to ng7.
We have create a base class with @input and many child classes off that and each child class has its own @input.
We started to see the instance of the child class contained many extra properties that are not part it.
I traced the code to what I have updated in my pull request:
The Reflection returns object back and it gets the meta of the child class's parent first, and it will find the parent class' binding object and returning that. And in ts-decorator it add a binding to the object returned (parent's object) and store back to itself. Since object is by reference, this mean the child and parent is sharing the same object. So my change is to clone the object returned (shallow) and add new binding to that and store the new object back.

I tested locally with our code and that solve the issues we were seeing.

I went ahead and update any of the decorators that is placed inside of a class. Please review it and see if it is ok.

Btw, I tried to build it with npm run install, but no luck. Not sure how you build it :)

Jonathan

@coveralls
Copy link

coveralls commented Apr 19, 2019

Coverage Status

Coverage increased (+0.2%) to 69.257% when pulling b1b6057 on coolblades:master into 681ceec on vsternbach:master.

@coolblades
Copy link
Author

coolblades commented Apr 19, 2019

hold on, I encountered an issue, going to try debug it.

@coolblades
Copy link
Author

coolblades commented Apr 19, 2019

false alarm, error in our code we had binding name started with dataXXX and it got lost when it become data-xxx. Previously without ts-decorator, angular was more forgiving.

@MartijnWelker
Copy link

I actually ran into the same issuer over at #96 and came to the same fix in #97

@coolblades
Copy link
Author

Martijn, great mind think a like :) I am glad we have the same solution, hopefully it will get merged.

@vsternbach
Copy link
Owner

Sorry, was a bit overloaded at work lately. Seems that @MartijnWelker solution is a bit more elegant, although it seems to be a bug there

@MartijnWelker
Copy link

@vsternbach I actually changed the fix I had in my pull request over at rentmanpublic@d58e488, if you want I can open a pull request with those changes.

@coolblades
Copy link
Author

coolblades commented May 22, 2019

Just want to make sure the change can be used in IE11, not sure spread operator is valid for IE11, which we need to support.

@MartijnWelker
Copy link

@coolblades I assumed the typescript compilation process would take care of that, but it's a good idea to test that 👍

@vsternbach
Copy link
Owner

there's no any problem in using es6+ features since it's compiled to es5 anyway

@vsternbach
Copy link
Owner

vsternbach commented May 23, 2019

Afaik from angular extending classes are not getting any of the inputs and outputs from the parent class, so I think I'll be changing that here as well, so each call to getMetadata will be replaced with getOwnMetadata, so this will be kind of braking change here, but then this code will be easier to port to angular
UPDATE: actually now I am not sure anymore what should be the behaviour: angular/angular@f5c8e09

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.

4 participants