ngUpgraders / ng-forward

The default solution for those that want to write Angular 2.x style code in Angular 1.x
411 stars 36 forks source link

Issue with binding and constructor #81

Closed waeljammal closed 8 years ago

waeljammal commented 8 years ago

Hi,

So I seem to be having an issue where my values are not available when the constructor is invoked.

I have put break points in my constructor and in createDirectiveController and before $injector.invoke is called I can see my .named values in the instance however when my constructor is called the values are undefined.

<my-component data='hello'> (tried with [], [()] and same thing)

in my constructor I try to access this.data and it's undefined however it seems that babel's define-decorated-property-descriptor is wiping out the value due to this line:

descriptor.value = descriptor.initializer ? descriptor.initializer.call(target) : undefined;

So after hitting babel the constructor is called and values that use the getters/setters defined in inputs-builder get wiped out and are now undefined.

timkindberg commented 8 years ago

Did you add data as an input? e.g.

@Component({
  inputs: ['data']
})
class MyCmp {}
timkindberg commented 8 years ago

Can you create a plunkr?

waeljammal commented 8 years ago

A plunker of what though? I'm not sure if plunker can do babel/es6

However the class I have looks like the code below (rest of code excluded)

this.title is undefined however before $injector.invoke gets called the value is there and if you step in past the fn apply in angular you should hit babel's define-decorated-property-descriptor at which point the value of the object definition gets reset back to undefined.

    @Input('titleLabel')
    title;

    constructor($compile, $element, $transclude) { 
        console.log(this.title);
    }
waeljammal commented 8 years ago

Ok so I found a fix, afraid I can not create a PR as your code is embeded in my project and very heavily modified :)

However the fix for babel's requirement that property descriptors have initialisers is:

My Input export looks like so now and it works fine.

export function Input(publicName) {
    return function(proto, localName, descriptor) {
        descriptor.initializer = function() {
            return this[localName];
        };

        writeMapSingle(proto.constructor, localName, publicName, 'inputMap');
    };
}
timkindberg commented 8 years ago

It works for me... what version of babel?

waeljammal commented 8 years ago

Version 5.8.34

Also I use webpacks require.ensure however that should have no effect as it's being nulled by babel's runtime helpers.

timkindberg commented 8 years ago

Yeah so I had looked at this and I was not even getting a third param of 'descriptor' into my inner function there in Input. So I was getting an error where 'initializer' didn't exist on 'undefined'.

timkindberg commented 8 years ago

I'd thought maybe it needed a new babel version to cause the bug, but you are saying you are running 5.8... so I'm not sure why I would not see this bug. Any other tips or clues?

timkindberg commented 8 years ago

@waeljammal here is a plunkr with ng-forward set up in it. Can you please create a plunkr that reproduces the bug?

timkindberg commented 8 years ago

woops... here's the plunkr link: http://plnkr.co/edit/ktxXKHyHQ5DLcixe6kpO?p=preview

waeljammal commented 8 years ago

I have updated it, http://plnkr.co/edit/yc7dmVf7JoGD83qEDqcG?p=preview

However the version on there does not have the @Input decorator but I added it anyway so the plunker might not run atm. I have added comments where the issue occurs and why.

timkindberg commented 8 years ago

Yup you're right. I got the plunkr converted to Babel (because the bug wasn't present with TypeScript) and updated to alpha 10 and your input is indeed undefined in the constructor.

http://plnkr.co/edit/hmvnN7mEgPuZK8bEAe01?p=preview

MikeRyanDev commented 8 years ago

So to be clear, it is undefined in the constructor but eventually becomes defined?

Reason I ask is that in Angular 2, inputs are undefined in the constructor and become defined once the onInit lifecycle hook is called. While this behavior is caused by a bug in ng-forward, when fixing the bug we need to keep this mind.

waeljammal commented 8 years ago

until life cycle events are ready the constructor is the only other place we can use atm.

Thanks

timkindberg commented 8 years ago

Here's my attempt to fix it: http://plnkr.co/edit/hmvnN7mEgPuZK8bEAe01?p=preview

I inlined ng-forward but with the patch you provided above with the descriptor.initializer. It fixes your bug but breaks everything else...

timkindberg commented 8 years ago

But like @MikeRyan52 said, we may not even want it to function the way you are requesting. Perhaps Ill work on the life cycles next.

timkindberg commented 8 years ago

ngOnInit lifecycle is now available...