angular: Custom decorators not working using AoT and Ivy

šŸž bug report

Affected Package

The issue is caused by package @angular/compiler-cli (I guess)

Is this a regression?

I had never tried AoT with Ivy

Description

I have a custom property decorator like this:
export function Language(): PropertyDecorator {
    function DecoratorFactory(target: any): void {
        const targetNgOnInit: () => void = target.ngOnInit;
        function ngOnInit(this: any): void {
            
            // Just for example
            console.log('onInit');

            if (targetNgOnInit) {
                targetNgOnInit.apply(this);
            }
        }
        target.ngOnInit = ngOnInit;
    }
    return DecoratorFactory;
}

used as:

@Language() lang: string;

ngOnInit() {
  // For aot
}
  • With AoT compiler, it works (it writes onInit in console)
  • If I enable Ivy and disable AoT, it works
  • If I enable Ivy and AoT, it doesnā€™t work

šŸ”¬ Minimal Reproduction

angular-ivy-decorators

šŸ”„ Exception or Error

No errors during compilation or in console. The ngOnInit method in decorator function is ignored.

šŸŒ Your Environment

Angular Version:


Angular CLI: 8.1.1
Node: 10.14.2
OS: win32 x64
Angular: 8.1.1
... animations, cli, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.801.1
@angular-devkit/build-angular     0.801.1
@angular-devkit/build-optimizer   0.801.1
@angular-devkit/build-webpack     0.801.1
@angular-devkit/core              8.1.1
@angular-devkit/schematics        8.1.1
@ngtools/webpack                  8.1.1
@schematics/angular               8.1.1
@schematics/update                0.801.1
rxjs                              6.4.0
typescript                        3.4.5
webpack                           4.35.2

Anything else relevant? Tried without success also the next version of Angular.

Thanks

About this issue

  • Original URL
  • State: closed
  • Created 5 years ago
  • Reactions: 34
  • Comments: 61 (19 by maintainers)

Most upvoted comments

A 1-cent from me. Before Angular 9 I used a Memoize decorator across all my components in many apps and now it comes to the point that I canā€™t use it anymore.

  @Memoize()
  getPermission(account: Account) {
    return this.permissions.filter(p => account.permissions.includes(p.id));
  }

As one might see, the operation is quite heavy and I cache it after one run. Now after the production build (in a non-prod build it works fine) it simply throws the runtime error Reference error: Account is undefined. I spent quite a lot of time until it became clear that the decorator was the reason.

Sad, but looks like I need to find another way now, probably less magical.

However, this is a breaking change and itā€™s not mentioned anywhere. Maybe itā€™s worth to give at least some note that custom decorators will fail now on the componentsā€™ members

This post will probably get million times downvoted, however my personal finding is since I use early beta versions of Angular 2: do not ever extend angular components and do not use custom decorators on angular components and their members. This always led to the problems; during upgrades 2 -> @angular/cli -> 4 -> 5 -> 6 -> 7 -> 8 -> 9 I rewrote tons of components just because of these two things.

Here are similar AoT issues that are not fixed for years and it seems to not happen soon https://github.com/angular/angular/issues/23038 https://github.com/angular/angular/issues/16023 and many others.

I clearly understand the reasoning behind. The decorators are changing the content, behaviour and the lifecycle of the components; they can actually rewrite everything we know of the component in the runtime. Itā€™s really hard (impossible?) to catch this with the custom compiler and create a performant application. Itā€™s a very hard job that Angular team is already doing. Thank you for that!

Maybe problem is not in Angular. Probably we should change the way we think.

But, Angular team, please help us. The framework documentation is super-silent about all these issues. I will probably repeat myself here https://github.com/angular/angular/issues/31495#issuecomment-583767197 and ask for the easiest solution. Maybe politically itā€™s not the best for angular, or there are some religious beliefs against that, but please add those two clear lines to the componentsā€™ documentation:

  • we do not yet fully support components inheritance
  • we do not yet fully support custom decorators on components

And then the issues like this would be resolved. The StackOverflow answers would not propose to extend components. People would not be annoyed of rewriting their applications.

I am pretty sure everybody here will have no problems with using no decorators. The only thing that is missing: this should be properly communicated.

HI Team,

Please give some high priority to this issue since this is breaking issue for all those are using custom decorators

AutoSubscriptions is broken now when using Ivy!!! šŸ˜­šŸ˜­šŸ˜­

@Component({
  selector: 'app-root',
  templateUrl: './app.component.html',
  styleUrls: ['./app.component.less']
})
@AutoSubscriptions()
export class AppComponent implements OnInit, OnDestroy {
  httpResult: object;
  
  @AutoSubscription
  httpResult$: Observable<Object> = this.http.get('HTTP_URL').pipe(
    tap((httpResult: object) => this.httpResult = httpResult)
  );
  
  constructor(protected http: HttpClient) {
  }
  
  ngOnInit() {
  }
 
  ngOnDestroy() {
  }
}
 constructor.prototype[metadata.init] = AutoAutoSubscriptionsInit(originalInit);
 constructor.prototype[metadata.destroy] = AutoAutoSubscriptionsDestroy(originalDestroy);

Ansyn image analysis application is using the above mentioned auto-subscription decorators widely. And therefore this app is also blocked now from working with Ivy.

Solving the current issue is of high priority for us.

TIA

AutoSubscriptions is broken now when using Ivy!!! šŸ˜­šŸ˜­šŸ˜­

@Component({
  selector: 'app-root',
  templateUrl: './app.component.html',
  styleUrls: ['./app.component.less']
})
@AutoSubscriptions()
export class AppComponent implements OnInit, OnDestroy {
  httpResult: object;
  
  @AutoSubscription
  httpResult$: Observable<Object> = this.http.get('HTTP_URL').pipe(
    tap((httpResult: object) => this.httpResult = httpResult)
  );
  
  constructor(protected http: HttpClient) {
  }
  
  ngOnInit() {
  }
 
  ngOnDestroy() {
  }
}
 constructor.prototype[metadata.init] = AutoAutoSubscriptionsInit(originalInit);
 constructor.prototype[metadata.destroy] = AutoAutoSubscriptionsDestroy(originalDestroy);

Appears the reason here is that Angular places ngComponentDef generated code before all __decorate calls for class. ɵɵdefineComponent function takes onInit as a reference to Class.prototype.ngOnInit and overridding it later has no any effect.

https://github.com/angular/angular/blob/58be2ff88400d7dc876d357f8cc3eb01b0912aae/packages/core/src/render3/definition.ts#L264

dec

Hereā€™s a pseudo-code of what happens:

class Test {
  ngOnInit() {
    console.log('original')
  } 
}

const hook = Test.prototype.ngOnInit;

Test.prototype.ngOnInit = () => {
  console.log(1);
}

hook(); // prints 'original'

In case of JIT compilation componentDef is created lazily as soon as getComponentDef gets called. It happens later.

This is a truly awful problem. Please fix it quickly as the version up of the internal project cannot be done.

Any updates on this? We cannot move our project to ng9.

@Splaktar @pkozlowski-opensource Hello, it was written above that we need to specify a use case in order to increase the priority of this issue. Well as you can see, a typical use case is for decorators which override component life-cycle hooks handlers (ngOnInit, ngOnDestrory) in order to more easily and safely subscribe to and unsubscribe from various streams. As you can see above, this feature has been in demand. And it has ceased to work in Ivy. This is not a matter of ā€œregressionā€, but of (significant) ā€œconvenienceā€, and safely.

So can you raise the priority of this issue?

TIA

I beg to differ on this not being a regression. It worked in previous versions, Ivy breaks it - regression. Furthermore, Ivy gets in the way of JS nature/features, like overwriting the prototype method. Make no mistake, this is not limited to the decorators only.

Another popular example is a custom takeUntilDestroy rxjs operator, which is nothing else than a function accepting an instance of component class.

This however should not reduce the priority of this issue.


In my custom operator I ended up relying on private ɵcmp property instead of prototype, in hope Angular will eventually fix this:

if (instanceConstructor.ɵcmp) {
  // Using private API until the corresponding Angular issues are resolved.
  // Issues:  https://github.com/angular/angular/issues/31495, https://github.com/angular/angular/issues/30497
  // PR:      https://github.com/angular/angular/pull/35464
  destroy = <Function>instanceConstructor.ɵcmp.onDestroy ?? noop;
  instanceConstructor.ɵcmp.onDestroy = __takeUntilDestroy__destroy__;
} else {
  destroy = <Function>instanceConstructor.prototype.ngOnDestroy ?? noop;
  instanceConstructor.prototype.ngOnDestroy = __takeUntilDestroy__destroy__;
}

Angularā€™s compiler btw has no say in the location of ngComponentDef vs the __decorate call - this is the way TypeScript naturally compiles static fields.

But reading the TypeScript spec Class decorators:

A Class Decorator is declared just before a class declaration. The class decorator is applied to the constructor of the class and can be used to observe, modify, or replace a class definition.

In my case, Iā€™m using a class decorator and doing same thing as this ticket, replacing ngOnInit with my own. And it does not work, exactly because the issue specified in this ticket.

Iā€™m not expert to tell this is due to core or ivy, but to me, this behavior has been defined clearly in TypeScript, but Angularā€™s lifecycle hooks cached too early to prevent class decorator working as TypeScript defined.

Hence, this is a bug somewhere in Angular. Would be great if this can be prioritized.

I think my scenario is a little bit different.

Take a look at this: https://github.com/abadakhshan/issue31495/blob/master/src/app/modules/share/general-list/general-list.component.ts

I have a class(GeneralListComponent ) with a decorator(@DynamicForm). This class is not used anywhere and it is going to be used dynamically.

In this situation, the decorator does not work( It is removed by the compiler), but if I add any code that uses this class( for example this line), everything is ok!

This post will probably get million times downvoted, however my personal finding is since I use early beta versions of Angular 2: do not ever extend angular components and do not use custom decorators on angular components and their members. This always led to the problems; during upgrades 2 -> @angular/cli -> 4 -> 5 -> 6 -> 7 -> 8 -> 9 I rewrote tons of components just because of these two things.

Here are similar AoT issues that are not fixed for years and it seems to not happen soon #23038 #16023 and many others.

I clearly understand the reasoning behind. The decorators are changing the content, behaviour and the lifecycle of the components; they can actually rewrite everything we know of the component in the runtime. Itā€™s really hard (impossible?) to catch this with the custom compiler and create a performant application. Itā€™s a very hard job that Angular team is already doing. Thank you for that!

Maybe problem is not in Angular. Probably we should change the way we think.

But, Angular team, please help us. The framework documentation is super-silent about all these issues. I will probably repeat myself here #31495 (comment) and ask for the easiest solution. Maybe politically itā€™s not the best for angular, or there are some religious beliefs against that, but please add those two clear lines to the componentsā€™ documentation:

  • we do not yet fully support components inheritance
  • we do not yet fully support custom decorators on components

And then the issues like this would be resolved. The StackOverflow answers would not propose to extend components. People would not be annoyed of rewriting their applications.

I am pretty sure everybody here will have no problems with using no decorators. The only thing that is missing: this should be properly communicated.

Angular canā€™t have a philosophy of ā€œDecorators for me, but not for theeā€. The entire library works by the elegance that decorators provide - @Input(), @Output() honestly there wouldnā€™t even be an Angular if it werenā€™t for decorators. Remember Atscript, and how that was discarded because Typescript jumped on the decorators bandwagon?

This is a failure of the Ivy team anticipating that many users are more savvy, and use custom decorators. Hopefully a mechanism can be found which will allow us to still use these incredibly powerful tools, with Ivy.

This issue makes using Ivy impossible; We have used custom decorators extensively throughout our application, and the fact that they canā€™t override lifecycle hooks is a real issue.

I had the same problem as @smnbbrv, for me workaround was to set emitDecoratorMetadata to false in tsconfig

@bryanrideshark

The entire library works by the elegance that decorators provide - @Input(), @Output() honestly there wouldnā€™t even be an Angular if it werenā€™t for decorators. Remember Atscript, and how that was discarded because Typescript jumped on the decorators bandwagon?

Yeah, Angular wouldnā€™t be probably a thing without decorators(or comments or something like that), because the compiler needs them. The problem is Angular itself using decorators as annotations only and this is the difference(AtScript and Traceur implemented annotations only as well) https://twitter.com/IgorMinar/status/1231310929179971584. I personally see it as must-have for compilers(Angular), but not that much for developers. Also as you can see from the decorators proposal(still Stage 2) and v8 team feedback https://github.com/tc39/proposal-decorators/issues/302, it might not be standardized at the end so Iā€™m not sure whether end developers should be using them extensively for the reason before itā€™s known.

But I understand you, itā€™s BC or regression, so should be supported by the Angular team. I also know metaprogramming is a thing, so no offense.

While trying to find alternative way to implement what I did with decorator, actually, I discovered that the impact of this behavior much broader, not only limiting to decorator.

Here is what Iā€™m trying to do in activate event handler of router-outlet, to rewrite the ngOnInit.

  onRouterOutletActivate(component: any) {
    const oldOnInit = component.ngOnInit;
    component.ngOnInit = function (this: any) {
      console.log('triggered');

      if (oldOnInit != null) {
        oldOnInit.bind(this)();
      }
    };
  }

While enabling Ivy, the original ngOnInit triggered, instead of my new one. It works while disabling Ivy ("enableIvy": false).

What I can see, the real issue is that lifecycle hooks for component is NOT overwritable anymore while enabling Ivy, not just impacting the decorators.

Expose component features and weā€™ll be good šŸ™‚

Any update on this issue? Is there a workaround so far?

@kumaresan-subramani just use ngx-observable-lifecycle if you can not get it to work.

Given a class decorator

function Mixin(value) {
    return function(target) {
        target.prototype.ngOnInit = function () {
            console.log("ngOnInit", value)
        }
    }
}

@Component({ ... })
@Mixin({ value: 123 })
export class AppComponent {}

This workaround should work in Ivy

const Extend = Mixin({ value: 123 })

@Component({ ... })
export class AppComponent {
    static mixins = Extend(AppComponent)
}

For ViewEngine compatibility, you also need to add a stub for ngOnInit

@Component({ ... })
export class AppComponent {
    public ngOnInit?()
    static mixins = Extend(AppComponent)
}

Assigning to a static property on the class forces the mixin to be executed before the runtime snapshots the class.

@wsdt This was already fixed in Angular v10.0.5.

Using custom decorators with AoT and Ivy is also high priority for us.

@khiami you can track progress in PR #35464

See above, there is a PR on this problem.

Please take a look at https://github.com/angular/angular/pull/35464. Hopefully this might resolve the issue for you?

It appears the issue I was running into was the same one that @abadakhshan describes here. My app makes heavy use of dynamically instantiated components that often times arenā€™t referenced by the app other than in the NgModule declarations. It appears the reason my custom decorator isnā€™t working is because those components are being tree-shaken from the build. So in my situation this isnā€™t a defect of angular, but rather an education on Ivyā€¦

@Splaktar @pkozlowski-opensource Hello, it was written above that we need to specify a use case in order to increase the priority of this issue. Well as you can see, a typical use case is for decorators which override component life-cycle hooks handlers (ngOnInit, ngOnDestrory) in order to more easily and safely subscribe to and unsubscribe from various streams. As you can see above, this feature has been in demand. And it has ceased to work in Ivy. This is not a matter of ā€œregressionā€, but of (significant) ā€œconvenienceā€, and safely.

So can you raise the priority of this issue?

TIA

I fail to see why there should be two classes of developers. Library authors shouldnā€™t get more elegant tools just because they work on the library part of the codebase.

https://github.com/tc39/proposal-decorators#why-prioritize-the-features-of-legacy-decorators-like-classes-over-other-features-that-decorators-could-provide

Decorators are here to stay in Javascript. The syntax will change, and certainly weā€™ll all be faced with a refactor once it becomes a fully complete spec. But we will always have access to this lovely syntax, and thatā€™s for the good of everyone.

It appears that @mhevery is taking this seriously. Thatā€™s much appreciated.

https://github.com/angular/angular/issues/30497 https://github.com/angular/angular/pull/35464

@kumaresan-subramani From within your empty lifecycle methods, you should delegate into the mixin field.