Investigate why pure_getters had to be re-added

[filipesilva]

Followup on #14287

[kzc]

Related issue: angular/angular#21880 (comment)

I'm not an Angular user but I remember looking at this pure_getters issue at the time using the original pre-uglify rollup output from angular/angular#21880 (comment) (which doesn't exist now). Using:

$ terser -V
terser 3.11.0

The minified size with pure_getters enabled:

$ cat ivy-bundle.js | terser -mc pure_getters,passes=3 --define ngDevMode=false | wc -c
    9872

The minified size with pure_getters disabled:

$ cat ivy-bundle.js | terser -mc passes=3 --define ngDevMode=false | wc -c
   24030

Property reads are considered to be side effects with pure_getters disabled. Declaring property reads of IIFE objects at module scope will retain those IIFEs even if they are not otherwise used/referenced.

Here's a workaround to obtain similar minified size with pure_getters disabled. For illustrative purposes and convenience I'm just patching the generated unminified bundle directly. The IIFE property reads are moved into functions to make DCE more effective with pure call annotations:

$ diff -u ivy-bundle.js ivy-bundle-patched.js
--- ivy-bundle.js	2019-05-01 16:22:51.000000000 -0400
+++ ivy-bundle-patched.js	2019-05-01 16:23:02.000000000 -0400
@@ -1957,18 +1957,21 @@
     };
     return ConnectableObservable;
 }(Observable));
-var connectableProto = ConnectableObservable.prototype;
-var connectableObservableDescriptor = {
-    operator: { value: null },
-    _refCount: { value: 0, writable: true },
-    _subject: { value: null, writable: true },
-    _connection: { value: null, writable: true },
-    _subscribe: { value: connectableProto._subscribe },
-    _isComplete: { value: connectableProto._isComplete, writable: true },
-    getSubject: { value: connectableProto.getSubject },
-    connect: { value: connectableProto.connect },
-    refCount: { value: connectableProto.refCount }
-};
+var connectableObservableDescriptor = /*@__PURE__*/ (function(){
+    var connectableProto = ConnectableObservable.prototype;
+    var connectableObservableDescriptor = {
+        operator: { value: null },
+        _refCount: { value: 0, writable: true },
+        _subject: { value: null, writable: true },
+        _connection: { value: null, writable: true },
+        _subscribe: { value: connectableProto._subscribe },
+        _isComplete: { value: connectableProto._isComplete, writable: true },
+        getSubject: { value: connectableProto.getSubject },
+        connect: { value: connectableProto.connect },
+        refCount: { value: connectableProto.refCount }
+    };
+    return connectableObservableDescriptor;
+}());
 var ConnectableSubscriber = /*@__PURE__*/ (function (_super) {
     __extends(ConnectableSubscriber, _super);
     function ConnectableSubscriber(destination, connectable) {
@@ -3070,13 +3073,12 @@
 var USE_VALUE = /*@__PURE__*/ getClosureSafeProperty({ provide: String, useValue: ɵ2 });
 var NG_TOKEN_PATH = 'ngTokenPath';
 var NG_TEMP_TOKEN_PATH = 'ngTempTokenPath';
-var NULL_INJECTOR = Injector.NULL;
 var NEW_LINE = /\n/gm;
 var NO_NEW_LINE = 'ɵ';
 var StaticInjector = /*@__PURE__*/ (/*@__PURE__*/ function () {
     function StaticInjector(providers, parent, source) {
         if (parent === void 0) {
-            parent = NULL_INJECTOR;
+            parent = Injector.NULL;
         }
         if (source === void 0) {
             source = null;
@@ -3283,7 +3285,7 @@
                     depRecord.token, childRecord, records, 
                     // If we don't know how to resolve dependency and we should not check parent for it,
                     // than pass in Null injector.
-                    !childRecord && !(options & 4 /* CheckParent */) ? NULL_INJECTOR : parent, options & 1 /* Optional */ ? null : Injector.THROW_IF_NOT_FOUND));
+                    !childRecord && !(options & 4 /* CheckParent */) ? Injector.NULL : parent, options & 1 /* Optional */ ? null : Injector.THROW_IF_NOT_FOUND));
                 }
             }
             record.value = value = useNew ? new ((_a = ((fn))).bind.apply(_a, [void 0].concat(deps)))() : fn.apply(obj, deps);

The size of the minified patched bundle with pure_getters disabled compares favorably to the original pure_getters version:

$ cat ivy-bundle-patched.js | terser -mc passes=3 --define ngDevMode=false | wc -c
    9979

This might be the source code in question:

https://github.com/ReactiveX/rxjs/blob/8c5d8319105415a015e36a58c5c6a34a6dafbf2b/src/internal/observable/ConnectableObservable.ts#L58-L70

https://github.com/angular/angular/blob/7d6f4885b25f36ab5262d868b0a4d762dea27a62/packages/core/src/di/injector.ts#L120

[filipesilva]

Whoa @kzc that's amazing work, thank you so much for looking into this!

I was toying with the idea of adding some linter rules to prevent property access on toplevel modules and your analysis further cements that idea really.

[filipesilva]

Tried removing all property accesses in the Angular packages and it doesn't look like anything was being retained by accident: angular/angular#30239

[filipesilva]

Removing this from the 8.0 milestone. We're removing the toplevel property access in Angular (angular/angular#29329) and RxJs (ReactiveX/rxjs#4769), then checking the pure_getters impact on a few projects, then trying to remove it altogether post 8.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}