build-optimizer doesn't make Ivy's ngInjectorDef static property assignment available for DCE

[IgorMinar]

🐞 Bug report

Command (mark with an x)

- [ ] new
- [x] build
- [ ] serve
- [ ] test
- [ ] e2e
- [ ] generate
- [ ] add
- [ ] update
- [ ] lint
- [ ] xi18n
- [ ] run
- [ ] config
- [ ] help
- [ ] version
- [ ] doc

Is this a regression?

Yes, the previous version in which this bug was not present was: ....

no

Description

A clear and concise description of the problem...

While debugging Ivy payload size issues, I noticed that DeprecatedI18NPipesModule was not being removed from hello world cli app, but only in the Ivy mode. Digging deeper I noticed that the build pipeline doesn't make ngInjectorDef assignments avalable for DCE.

Snippet from the pre-terser code:

common_DeprecatedI18NPipesModule.ngInjectorDef = /*@__PURE__*/ ɵɵdefineInjector({
  factory: function DeprecatedI18NPipesModule_Factory(t) {
    return new (t || common_DeprecatedI18NPipesModule)();
  },
  providers: [{ provide: DEPRECATED_PLURAL_FN, useValue: common_0$2 }] });

Terser won't remove this because it's a static property assignment. So while the right handside is marked as pure, the left handside might have a side effect and therefore will be retained.

I think this is a general problem we have in the pipeline where we generally expect that the static properties will be assigned within the PURE IIFE wrapped around a class, but in this case the assignment happens outside of that IIFE likely because of two other function calls (ɵɵsetNgModuleScope and setClassMetadata) that are emitted in between the class body and the static assignment:

 let common_DeprecatedI18NPipesModule = /*@__PURE__*/ (() => {
                class DeprecatedI18NPipesModule {
                }
                DeprecatedI18NPipesModule.ngModuleDef = ɵɵdefineNgModule({ type: DeprecatedI18NPipesModule });
                return DeprecatedI18NPipesModule;
        })();

/*@__PURE__*/ /*@__PURE__*/ ɵɵsetNgModuleScope(common_DeprecatedI18NPipesModule, {
                declarations: [common_DeprecatedDecimalPipe,
                        common_DeprecatedPercentPipe,
                        common_DeprecatedCurrencyPipe,
                        common_DeprecatedDatePipe], exports: [common_DeprecatedDecimalPipe,
                                common_DeprecatedPercentPipe,
                                common_DeprecatedCurrencyPipe,
                                common_DeprecatedDatePipe]
        });

/*@__PURE__*/ /*@__PURE__*/ setClassMetadata(common_DeprecatedI18NPipesModule, [{
                type: NgModule,
                args: [{
                        declarations: [COMMON_DEPRECATED_I18N_PIPES],
                        exports: [COMMON_DEPRECATED_I18N_PIPES],
                        providers: [{ provide: DEPRECATED_PLURAL_FN, useValue: common_0$2 }]
                }]
        }], null, null);


       common_DeprecatedI18NPipesModule.ngInjectorDef = /*@__PURE__*/ ɵɵdefineInjector({
                factory: function DeprecatedI18NPipesModule_Factory(t) {
                        return new (t || common_DeprecatedI18NPipesModule)();
                },
                providers:
                [{ provide: DEPRECATED_PLURAL_FN, useValue: common_0$2 }] });

Because the static property assignment is now outside of the PURE class IIFE, the static property assignment is ineligible for DCE.

To fix this we have two options:

1. ensure that the static property assignment is located within the class body by changing the order in which these things are emitted by ngtsc
2. wrap the static property assignment in a new PURE IIFE:

/*@__PURE__*/ (function() { common_DeprecatedI18NPipesModule.ngInjectorDef = /*@__PURE__*/ ɵɵdefineInjector({
  factory: function DeprecatedI18NPipesModule_Factory(t) {
    return new (t || common_DeprecatedI18NPipesModule)();
  },
  providers: [{ provide: DEPRECATED_PLURAL_FN, useValue: common_0$2 }] });
})();

note the IIFE around the assignment marked as pure.

I'm slightly leaning towards the options number two just because it's more generic and could catch other issues.

🔬 Minimal Reproduction

yarn global add @angular/cli@next
ng new size-test --enable-ivy
cd size-test
ng build --prod

review bundles and you'll see DeprecatedI18NPipesModule retained due to the pattern described above.

🔥 Exception or Error

🌍 Your Environment

Angular CLI: 8.2.0-rc.0
Node: 10.11.0
OS: darwin x64
Angular: 8.2.0-rc.0
... animations, cli, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.802.0-rc.0
@angular-devkit/build-angular     0.802.0-rc.0
@angular-devkit/build-optimizer   0.802.0-rc.0
@angular-devkit/build-webpack     0.802.0-rc.0
@angular-devkit/core              8.2.0-rc.0
@angular-devkit/schematics        8.2.0-rc.0
@ngtools/webpack                  8.2.0-rc.0
@schematics/angular               8.2.0-rc.0
@schematics/update                0.802.0-rc.0
rxjs                              6.4.0
typescript                        3.5.3
webpack                           4.38.0

Anything else relevant?

[filipesilva]

Yes this is a problem.

I'm not sure option 2 (wrap the static property assignment in a new PURE IIFE) would work though. A pure function call with no arguments might just always get dropped. Will verify.

We can also specialcase something.ngInjectorDef. We might also be able to just pull ɵɵsetNgModuleScope, setClassMetadata and common_DeprecatedI18NPipesModule.ngInjectorDef into the original class IIFE.

[filipesilva]

Note: to fully reproduce this, also modify node_modules/\@angular-devkit/build-angular/src/angular-cli-files/models/webpack-configs/common.js to beautify and not mangle output:

      // Add these two lines
      terserOptions.mangle = false;
      terserOptions.output.beautify = true;
        extraMinimizers.push(new TerserPlugin({
            sourceMap: scriptsSourceMap,
            parallel: true,
            cache: true,
            terserOptions,
        }));

This makes the output readable.

[IgorMinar]

upon further discussion, we should be able to fix this by emitting the calls in the right order from ngtsc/ngcc - that's the option 1 I proposed above. @alxhub is looking into this.

once we emit the code in the right order, the existing build-optimizer passes should be able to correctly optimize the code.