zehavibarak commented 3 years ago

Bug Report

Affected Package

The issue is caused by package @angular/cli

Is this a regression?

no

Description

Custom component decorator ignored on AoT.

Minimal Reproduction

https://stackblitz.com/edit/angular-ivy-mupixg?file=src/app/app.component.ts

Exception or Error

Expected to have 1 message on reproduction, receives 0 on build prod with aot true.

Your Environment

Angular Version:


ngular CLI: 11.2.12
Node: 15.5.0
OS: win32 x64

Angular: 11.2.13
... animations, common, compiler, compiler-cli, core, elements
... forms, language-service, platform-browser
... platform-browser-dynamic, platform-server, router
Ivy Workspace: Yes

Package                            Version
------------------------------------------------------------
@angular-devkit/architect          0.1102.12
@angular-devkit/build-angular      0.1102.12
@angular-devkit/build-webpack      0.1102.12
@angular-devkit/core               11.2.12
@angular-devkit/schematics         11.2.12
@angular/cdk                       11.2.12
@angular/cli                       11.2.12
@angular/flex-layout               11.0.0-beta.33
@angular/material                  11.2.12
@angular/material-moment-adapter   11.2.12
@schematics/angular                11.2.12
@schematics/update                 0.1102.12
ng-packagr                         11.2.4
rxjs                               6.6.7
typescript                         4.1.5

alan-agius4 commented 3 years ago

I'm sorry, but we can't reproduce the problem following the instructions you provided.

alxhub commented 3 years ago

I'm going to close this issue, as it's not actionable for us - in a normal Angular build, the decorator works as expected.

If Azure is modifying the deployed application in a way that breaks some if its functionality, you may need to open an issue with them to figure out why. Alternatively, if this does indeed turn out to be an Angular bug and you can provide a reproduction that doesn't require deploying to Azure, feel free to open a new issue.

zehavibarak commented 3 years ago

This is an Angular cli issue. It has to do with webpack/terser tree shaking Angular does when optimization flag is true. This is a BUG.

zehavibarak commented 3 years ago

I'm going to close this issue, as it's not actionable for us - in a normal Angular build, the decorator works as expected.

If Azure is modifying the deployed application in a way that breaks some if its functionality, you may need to open an issue with them to figure out why. Alternatively, if this does indeed turn out to be an Angular bug and you can provide a reproduction that doesn't require deploying to Azure, feel free to open a new issue.

I ask you reopen this issue, as it is clearly a bug in angular-cli package which process the optimization. If you do not know how to procure this, please tag someone who might have a know-how of the code.

alan-agius4 commented 3 years ago

@zehavibarak, as mentioned previously, this appears to be related to Azure is minifying your JavaScript too aggressively.

Please feel free to open a new issue with a reproduction if you can reproduce this without using Azure.

zehavibarak commented 3 years ago

This is NOT Azure issue. Angular-cli optimizes the code locally prior to publishing it. It’s optimization flag is setting the tree shaking in motion.

alan-agius4 commented 3 years ago

This is NOT Azure issue. Angular-cli optimizes the code locally prior to publishing it. It’s optimization flag is setting the tree shaking in motion.

As shown above, I am unable to replicate this locally.

zehavibarak commented 3 years ago

npm run build -- --prod uses webpack to optimize code. Please tag someone who understand the code there.

zehavibarak commented 3 years ago

There should be sideEffects setting somewhere preventing cli from optimizing certain modules.

petebacondarwin commented 3 years ago

npm run build -- --prod uses webpack to optimize code. Please tag someone who understand the code there.

I think you mean @alan-agius4 who has already commented here.

zehavibarak commented 3 years ago

npm run build -- --prod uses webpack to optimize code. Please tag someone who understand the code there.

I think you mean @alan-agius4 who has already commented here.

And yet, he hasn't sown any effort to fix this.

zehavibarak commented 3 years ago

The scenario we're experiencing is sliglty different the simplified repro above. Package A declares the decorator and apply it to some components, package B reference package A and apply the decorator to its components.

If optimization is false, it works. if optimization is true, it strips the decorator from package B. If buildOptimizer is true, it strips the decorator from both A and B packages.

This is clearly an angular-cli issue.

alan-agius4 commented 3 years ago

@zehavibarak, please provide a minimal reproduction that demonstrates the issue. As in the previous reproduction shared custom decorators worked.

Without a reproduction we are unable to investigate this issue. A good way to make a minimal repro is to create a new app via ng new repro-app and adding the minimum possible code to show the problem. Then you can push this repository to github and link it in a new issue providing all the information needed to reproduce.

zehavibarak commented 3 years ago

I've updated the repro to use the same package we're using (bizdoc.core).

alxhub commented 3 years ago

The reproduction linked in the issue (https://stackblitz.com/edit/angular-ivy-mupixg?file=src/app/app.component.ts) is currently broken. Even after fixing a dependency problem with @angular/compiler-cli, the project has a number of build errors:

Error: node_modules/bizdoc.core/src/app/core/controls/address.input.d.ts:20:17 - error TS2503: Cannot find namespace 'google'.

20     _addresses: google.maps.places.QueryAutocompletePrediction[];
                   ~~~~~~
node_modules/bizdoc.core/src/app/core/controls/address.input.d.ts:49:24 - error TS2503: Cannot find namespace 'google'.

49     displayFn(address: google.maps.places.QueryAutocompletePrediction): string | google.maps.places.QueryAutocompletePrediction;
                          ~~~~~~
node_modules/bizdoc.core/src/app/core/controls/address.input.d.ts:49:82 - error TS2503: Cannot find namespace 'google'.

49     displayFn(address: google.maps.places.QueryAutocompletePrediction): string | google.maps.places.QueryAutocompletePrediction;
                                                                                    ~~~~~~
node_modules/bizdoc.core/src/app/core/controls/address.input.d.ts:1:23 - error TS2688: Cannot find type definition file for 'googlemaps'.

1 /// <reference types="googlemaps" />
                        ~~~~~~~~~~
src/app/app.component.ts:2:18 - error TS2305: Module '"../../node_modules/bizdoc.core/bizdoc.core"' has no exported member 'REPOSITORY'.
m
2 import { BizDoc, REPOSITORY } from 'bizdoc.core';
                   ~~~~~~~~~~
src/app/app.module.ts:9:41 - error TS2304: Cannot find name 'BizDocModule'.

9   imports: [BrowserModule, FormsModule, BizDocModule],
                                          ~~~~~~~~~~~~

Looking at the last one, BizDocModule is never imported within src/app/app.module.ts, so this is clearly an app that doesn't build.

@zehavibarak, if you feel this is still a problem, please create a Github repository (not a Stackblitz) with a minimal reproduction, with locked dependency versions (a yarn.lock or package-lock.json file), and steps to reproduce/observe the problem. There may well be a bug here, but it's impossible to know without a working reproduction that we can dig into.

zehavibarak commented 3 years ago

zehavibarak/angular-decorator-app (github.com)https://github.com/zehavibarak/angular-decorator-app

alan-agius4 commented 3 years ago

@zehavibarak, https://github.com/zehavibarak/angular-decorator-app url doesn't exist.

zehavibarak commented 3 years ago

You sure?

alan-agius4 commented 3 years ago

Screenshot 2021-05-12 at 16 00 29

zehavibarak commented 3 years ago

Made it public. Should work now.

alxhub commented 3 years ago

The issue doesn't reproduce for me.

Building with ng build --aot --build-optimizer=true I can see that XyzComponent in the output does have the @BizDoc decorator applied via __decorate still. Build optimizer is doing its job here and not removing a decorator it doesn't understand. At runtime, the count shows 30, and listing out the keys shows app1 is included.

Building with ng serve --prod shows similar results - count drops to 1 (likely due to Terser now tree-shaking a bunch of decorated things in bizdoc.core, I didn't look into it) and listing the keys shows app1 is the only registered key.

To the extent that I understand bizdoc.core and the decorator's operation, this is working as intended for Angular.

Also, for this tiny application, including bizdoc.core exceeds the default CLI bundle budget (1MB) by almost 8MB 😱

zehavibarak commented 3 years ago

The count dropped to 1, meaning Angular stripped decorator from package A. How is this not an Angular issue?

zehavibarak commented 3 years ago

Please mark this issue as open @alan-agius4

alan-agius4 commented 3 years ago

The drop count to 1 is the correct outcome when running a production build because unused components from bizdoc.core are being removed which leaves only a single component XyzComponent decorated and inserted into the registry.

Therefore, this is working as expected.

zehavibarak commented 3 years ago

a) The components are created at runtime, not by template selector (factory resolver, injection). b) Try build using optimization true, it is expected to drop the xyz, resulting in count 0.

zehavibarak commented 3 years ago

There needs to be a way to exclude a package or module from this code.

alxhub commented 3 years ago

You can likely run your builds with --build-optimizer=false (or set buildOptimizer: false in angular.json), which will prevent build-optimizer from refactoring your code for tree-shaking.

build-optimizer has a fundamental assumption that decorators do not have global side effects, which is critical to how it works and cannot be disabled. Disabling build-optimizer will prevent tree-shaking of decorated classes - which is exactly what you want for this pattern.

It's worth noting that this assumption is backed up by the TC39 Decorator Proposal spec:

Decorators should affect the thing they're decorating, and avoid confusing/non-local effects.

zehavibarak commented 3 years ago

@alxhub the above repro show the decorator code is never called thus has nothing to do to the spec you mentioned.

The issue now is not the decorator as it is the removal of components not found in template or directly referenced from code.

Consider this code:

const comp = MyComp;

class MyApp {
  ngOnInit() {
    const factory = this_factoryResolver.resolveComponentFactory(comp);
    this._viewContainer.createComponent(factory);
  }
}

If I understand @alan-agius4, build-optimizer will drop MyComp since it is never used.

Build optimizer should to my opinion have a way to exclude a module and / or package.

zehavibarak commented 3 years ago

@alxhub again, the decorator code is never called, as the repro shows

alxhub commented 3 years ago

The issue now is not the decorator as it is the removal of components not found in template or directly referenced from code.

This is called tree-shaking - the removal of code such as components that's not directly referenced by an application. It's a design goal of Angular that components which are not found in a template and not directly referenced in code are removed.

That is, if you write:

class Foo {...}

and your code never references Foo, the optimizer is free to remove Foo entirely from the built application bundle. That optimization is normal and desirable.

But, what happens if you have a decorated class?

@Decorator
class Foo {...}

Can the optimizer safely make the same decision? It depends on what @Decorator does:

if @Decorator only mutates Foo, and doesn't have any other effects, then clearly yes
if @Decorator has some observable side effect, like making network requests, console.log'ing, or saving a reference to Foo somewhere (like a global REGISTRY), then arguably no.

A major problem with this situation is that optimizers only analyze code, they don't execute it, so it's very difficult for them to tell the above two cases apart. Any function call inside the decorator may perform global operations like the ones above.

It's arguable that the intent of the decorator spec was to allow for the first case (mutating Foo only) and not for non-local effects, as per the language mentioned earlier:

Decorators should affect the thing they're decorating, and avoid confusing/non-local effects.

Angular is designed with this interpretation in mind, and the production optimization performed by our CLI relies on this interpretation of decorators.

This is, however, complicated by the fact that decorators are not currently supported in some browsers directly, and so they're downleveled by TypeScript into JavaScript code that uses TypeScript's __decorate helper function instead. This code is much harder for an optimizer like Terser to follow, so Angular built the build-optimizer transformation which restructures downleveled decorators into a form that doesn't appear to the optimizer as globally side-effectful.

Disabling build-optimizer is a workaround - it ensures that at least for the current downleveling that TypeScript does, the optimizer won't be able to understand the form of decorated classes and will be forced to retain them. This is not guaranteed to work forever, but it does today.

build-optimizer could conceivably allow exclusion of particular libraries, and you should feel free to argue for that as a feature request. I don't think the case for such a feature is substantially compelling, however, because there are far better and more idiomatic Angular patterns for registering components into an index of some kind, which don't require global side effects.

For example, using a registry service that's injected into NgModule constructors and performing the registration there, or using a multi: true provider and DI. The idea here is that registration depends on a direct reference from the application (including the library's NgModule in your AppModule), and doesn't happen just because the library code happens to be included in the build.