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

(AOT): Angular does not invoke lifecycle listeners if typescript mixins are used #19145

Open
kemsky opened this Issue Sep 11, 2017 · 6 comments

Comments

Projects
None yet
6 participants
@kemsky

kemsky commented Sep 11, 2017

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

Assume we have base class, mixin and component class:

class Base implements OnInit {
    constructor(){}
    ngOnInit():void {}
}

type Constructor<T> = new(...args: any[]) => T;

function Tagged<T extends Constructor<{}>>(base: T) {
    return class extends base {
        value: string;
        constructor(...args: any[]) {
            super(...args);
            this.value= "test";
        }
    }
}

export class Component extends Tagged(Base) {
    public item:any;

    constructor()  {
        super();
    }
}

Component's ngOnInit listener will not be called in AOT.

Workaround:

export class Component extends Tagged(Base) {
    public item:any;

    constructor() {
        super();
    }

   ngOnInit():void {
         super.ngOnInit();
   }
}

Expected behavior

ngOnInit should be called.

Environment


Angular version: 4.4.0-RC.0
TypeScript: 2.4.2

@kemsky kemsky changed the title from Angular does not invoke lifecycle listeners if typescript mixins are used to (AOT): Angular does not invoke lifecycle listeners if typescript mixins are used Sep 11, 2017

@alexeagle

This comment has been minimized.

Contributor

alexeagle commented Sep 16, 2017

@chuckjaz looks like the static reflector doesn't understand some part of the mixin syntax (maybe return class extends base) to grasp the type inheritance

@chuckjaz

This comment has been minimized.

Member

chuckjaz commented Oct 27, 2017

The issue is that that static reflector does not use the type checker of TypeScript. Currently, if there is an expression in the extends clause we ignore it. This means we have no idea that the base class implements onInit nor an easy way to calculate it as it would require us to ask the type-checker.

We avoid using the type-checker because we need to be able to produce factory functions before type checker or we would have to do two full type-checks.

We should consider recognizing the pattern function f<T extends Constructor<{}>>(base: T) { as meaning we should just ignore the f and treat its parameter as the base class.

However, this would mean that we would not catch things like,

function CoolInitMixin<T extends Constructor<{}>>(base: T) {
    return class extends base {
        constructor(...args: any[]) {
            super(...args);
        }
        ngOnInit() {
         // something cool.
       }
    }
}

@ngbot ngbot bot added this to the Backlog milestone Jan 23, 2018

@chaosmonster

This comment has been minimized.

chaosmonster commented Feb 12, 2018

Just leave this here as @trotyl made me aware of this issue.
I think this is what alex thought about and it won't work in aot. Do you think this can be fixed?

https://gist.github.com/chaosmonster/f2576bbd1bc9a50282a2b3b492195b29

@trotyl

This comment has been minimized.

Contributor

trotyl commented Feb 12, 2018

@chaosmonster I guess this tracker is only for life-cycle hooks problem, what I meant is the mixin pattern not generally supported in AOT, but some basic functionality may be able to work already.

I'd suggest to raise separated issues when founding other concrete problems. 😃

EDIT: updated the original comment to make it clear.

@AndriiDidkivsky

This comment has been minimized.

AndriiDidkivsky commented Jul 25, 2018

Any updates for this? Well-supported mixins would be a powerful feature.

@trotyl

This comment has been minimized.

Contributor

trotyl commented Jul 25, 2018

This is already fixed by Ivy. But not likely to be fixed on current view engine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment