Investigate why pure_getters had to be re-added #14316

Activity

1. filipesilva

   added

   area: @angular-devkit/build-angular

   freq1: lowOnly reported by a handful of users who observe it rarelyOnly reported by a handful of users who observe it rarely

   severity3: broken

   type: bug/fix

   on Apr 30, 2019
2. ngbot

   added this to the Backlog milestone on Apr 30, 2019
3. filipesilva

   modified the milestones: Backlog, 8.0 on Apr 30, 2019
4. kzc commented on May 1, 2019

   

   kzc

   on May 1, 2019

   More actions

   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

   👍React with 👍2Reacted by Nadav Sinai and Sebastian Müller
5. filipesilva commented on May 2, 2019

   

   filipesilva

   on May 2, 2019

   Last edited by filipesilva

   ContributorAuthor

   More actions

   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.
6. filipesilva commented on May 2, 2019

   

   filipesilva

   on May 2, 2019

   ContributorAuthor

   More actions

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

   self-assigned this

   on May 3, 2019
8. filipesilva commented on May 16, 2019

   

   filipesilva

   on May 16, 2019

   ContributorAuthor

   More actions

   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.
9. filipesilva

   modified the milestones: 8.0, Backlog on May 16, 2019
10. alan-agius4

    modified the milestones: Backlog, 9.0.x on Sep 16, 2019
11. vikerman

    closed this as completedin #15607on Sep 24, 2019
12. vikerman

    added a commit that references this issue on Sep 24, 2019

    fix(@angular-devkit/build-angular): remove pure_getters

    ...

    This issue will close once commit d9f0bd5 is merged into the 'main' branch.

    d9f0bd5
13. angular-automatic-lock-bot commented on Oct 25, 2019

    

    angular-automatic-lock-botBot

    on Oct 25, 2019

    More actions

    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.}
14. angular-automatic-lock-bot

    locked and limited conversation to collaborators on Oct 25, 2019
15. mostafaibr

    added a commit that references this issue on May 29, 2026

    fix(@angular-devkit/build-angular): remove pure_getters

    ...

    This issue will close once commit c5bee77 is merged into the 'master' branch.

    c5bee77