refactor(core): cleanup code with side-effects which was preventing tree-shaking (#30580)

PR Close #30580

WORKSPACE

@@ -15,8 +15,8 @@ load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
 # Fetch rules_nodejs so we can install our npm dependencies
 http_archive(
     name = "build_bazel_rules_nodejs",
-    sha256 = "1db950bbd27fb2581866e307c0130983471d4c3cd49c46063a2503ca7b6770a4",
-    urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/0.29.0/rules_nodejs-0.29.0.tar.gz"],
+    sha256 = "395b7568f20822c13fc5abc65b1eced637446389181fda3a108fdd6ff2cac1e9",
+    urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/0.29.2/rules_nodejs-0.29.2.tar.gz"],
 )
 
 # Check the bazel version and download npm dependencies

integration/bazel/WORKSPACE

@@ -5,8 +5,8 @@ load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
 # Fetch rules_nodejs so we can install our npm dependencies
 http_archive(
     name = "build_bazel_rules_nodejs",
-    sha256 = "1db950bbd27fb2581866e307c0130983471d4c3cd49c46063a2503ca7b6770a4",
-    urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/0.29.0/rules_nodejs-0.29.0.tar.gz"],
+    sha256 = "395b7568f20822c13fc5abc65b1eced637446389181fda3a108fdd6ff2cac1e9",
+    urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/0.29.2/rules_nodejs-0.29.2.tar.gz"],
 )
 
 # Fetch sass rules for compiling sass files

integration/side-effects/snapshots/core/esm2015.js

@@ -1,147 +1,3 @@
-import { Subject, Subscription } from "rxjs";
+import "rxjs";
 
 import "rxjs/operators";
-
-function getGlobal() {
-    const __globalThis = "undefined" !== typeof globalThis && globalThis;
-    const __window = "undefined" !== typeof window && window;
-    const __self = "undefined" !== typeof self && "undefined" !== typeof WorkerGlobalScope && self instanceof WorkerGlobalScope && self;
-    const __global = "undefined" !== typeof global && global;
-    return __globalThis || __global || __window || __self;
-}
-
-const _global = getGlobal();
-
-let _symbolIterator = null;
-
-function getSymbolIterator() {
-    if (!_symbolIterator) {
-        const Symbol = _global["Symbol"];
-        if (Symbol && Symbol.iterator) _symbolIterator = Symbol.iterator; else {
-            const keys = Object.getOwnPropertyNames(Map.prototype);
-            for (let i = 0; i < keys.length; ++i) {
-                const key = keys[i];
-                if ("entries" !== key && "size" !== key && Map.prototype[key] === Map.prototype["entries"]) _symbolIterator = key;
-            }
-        }
-    }
-    return _symbolIterator;
-}
-
-if ("undefined" === typeof ngI18nClosureMode) _global["ngI18nClosureMode"] = "undefined" !== typeof goog && "function" === typeof goog.getMsg;
-
-function flatten(list, dst) {
-    if (void 0 === dst) dst = list;
-    for (let i = 0; i < list.length; i++) {
-        let item = list[i];
-        if (Array.isArray(item)) {
-            if (dst === list) dst = list.slice(0, i);
-            flatten(item, dst);
-        } else if (dst !== list) dst.push(item);
-    }
-    return dst;
-}
-
-class EventEmitter extends Subject {
-    constructor(isAsync = false) {
-        super();
-        this.__isAsync = isAsync;
-    }
-    emit(value) {
-        super.next(value);
-    }
-    subscribe(generatorOrNext, error, complete) {
-        let schedulerFn;
-        let errorFn = err => null;
-        let completeFn = () => null;
-        if (generatorOrNext && "object" === typeof generatorOrNext) {
-            schedulerFn = this.__isAsync ? value => {
-                setTimeout(() => generatorOrNext.next(value));
-            } : value => {
-                generatorOrNext.next(value);
-            };
-            if (generatorOrNext.error) errorFn = this.__isAsync ? err => {
-                setTimeout(() => generatorOrNext.error(err));
-            } : err => {
-                generatorOrNext.error(err);
-            };
-            if (generatorOrNext.complete) completeFn = this.__isAsync ? () => {
-                setTimeout(() => generatorOrNext.complete());
-            } : () => {
-                generatorOrNext.complete();
-            };
-        } else {
-            schedulerFn = this.__isAsync ? value => {
-                setTimeout(() => generatorOrNext(value));
-            } : value => {
-                generatorOrNext(value);
-            };
-            if (error) errorFn = this.__isAsync ? err => {
-                setTimeout(() => error(err));
-            } : err => {
-                error(err);
-            };
-            if (complete) completeFn = this.__isAsync ? () => {
-                setTimeout(() => complete());
-            } : () => {
-                complete();
-            };
-        }
-        const sink = super.subscribe(schedulerFn, errorFn, completeFn);
-        if (generatorOrNext instanceof Subscription) generatorOrNext.add(sink);
-        return sink;
-    }
-}
-
-class QueryList {
-    constructor() {
-        this.dirty = true;
-        this._results = [];
-        this.changes = new EventEmitter();
-        this.length = 0;
-    }
-    map(fn) {
-        return this._results.map(fn);
-    }
-    filter(fn) {
-        return this._results.filter(fn);
-    }
-    find(fn) {
-        return this._results.find(fn);
-    }
-    reduce(fn, init) {
-        return this._results.reduce(fn, init);
-    }
-    forEach(fn) {
-        this._results.forEach(fn);
-    }
-    some(fn) {
-        return this._results.some(fn);
-    }
-    toArray() {
-        return this._results.slice();
-    }
-    [getSymbolIterator()]() {
-        return this._results[getSymbolIterator()]();
-    }
-    toString() {
-        return this._results.toString();
-    }
-    reset(resultsTree) {
-        this._results = flatten(resultsTree);
-        this.dirty = false;
-        this.length = this._results.length;
-        this.last = this._results[this.length - 1];
-        this.first = this._results[0];
-    }
-    notifyOnChanges() {
-        this.changes.emit(this);
-    }
-    setDirty() {
-        this.dirty = true;
-    }
-    destroy() {
-        this.changes.complete();
-        this.changes.unsubscribe();
-    }
-}

integration/side-effects/snapshots/core/esm5.js

@@ -3,15 +3,3 @@ import "tslib";
 import "rxjs";
 
 import "rxjs/operators";
-
-function getGlobal() {
-    var __globalThis = "undefined" !== typeof globalThis && globalThis;
-    var __window = "undefined" !== typeof window && window;
-    var __self = "undefined" !== typeof self && "undefined" !== typeof WorkerGlobalScope && self instanceof WorkerGlobalScope && self;
-    var __global = "undefined" !== typeof global && global;
-    return __globalThis || __global || __window || __self;
-}
-
-var _global = getGlobal();
-
-if ("undefined" === typeof ngI18nClosureMode) _global["ngI18nClosureMode"] = "undefined" !== typeof goog && "function" === typeof goog.getMsg;

modules/benchmarks/src/expanding_rows/index.html

@@ -14,7 +14,6 @@
   <benchmark-root>loading...</benchmark-root>
 
   <script>
-    window.ngDevMode = location.hash == '#ngDevMode';
     addEventListener('DOMContentLoaded', () => {
       // DevServer has automatic bootstrap code, so if we already have <scripts> than we don't need to bootstrap
       var alreadyBootstraped = document.querySelectorAll('script').length > 1; // 1 for ourselves

packages/core/src/linker/query_list.ts

@@ -12,6 +12,9 @@ import {EventEmitter} from '../event_emitter';
 import {flatten} from '../util/array_utils';
 import {getSymbolIterator} from '../util/symbol';
 
+function symbolIterator<T>(this: QueryList<T>): Iterator<T> {
+  return ((this as any as{_results: Array<T>})._results as any)[getSymbolIterator()]();
+}
 
 /**
  * An unmodifiable list of items that Angular keeps up to date when the state
@@ -50,6 +53,16 @@ export class QueryList<T>/* implements Iterable<T> */ {
   // TODO(issue/24571): remove '!'.
   readonly last !: T;
 
+  constructor() {
+    // This function should be declared on the prototype, but doing so there will cause the class
+    // declaration to have side-effects and become not tree-shakable. For this reason we do it in
+    // the constructor.
+    // [getSymbolIterator()](): Iterator<T> { ... }
+    const symbol = getSymbolIterator();
+    const proto = QueryList.prototype as any;
+    if (!proto[symbol]) proto[symbol] = symbolIterator;
+  }
+
   /**
    * See
    * [Array.map](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map)
@@ -99,8 +112,6 @@ export class QueryList<T>/* implements Iterable<T> */ {
    */
   toArray(): T[] { return this._results.slice(); }
 
-  [getSymbolIterator()](): Iterator<T> { return (this._results as any)[getSymbolIterator()](); }
-
   toString(): string { return this._results.toString(); }
 
   /**

packages/core/src/util/global.ts

@@ -15,20 +15,16 @@ declare var global: any /** TODO #9100 */;
 // Not yet available in TypeScript: https://github.com/Microsoft/TypeScript/pull/29332
 declare var globalThis: any /** TODO #9100 */;
 
-function getGlobal(): any {
-  const __globalThis = typeof globalThis !== 'undefined' && globalThis;
-  const __window = typeof window !== 'undefined' && window;
-  const __self = typeof self !== 'undefined' && typeof WorkerGlobalScope !== 'undefined' &&
-      self instanceof WorkerGlobalScope && self;
-  const __global = typeof global !== 'undefined' && global;
+const __globalThis = typeof globalThis !== 'undefined' && globalThis;
+const __window = typeof window !== 'undefined' && window;
+const __self = typeof self !== 'undefined' && typeof WorkerGlobalScope !== 'undefined' &&
+    self instanceof WorkerGlobalScope && self;
+const __global = typeof global !== 'undefined' && global;
 
-  // Always use __globalThis if available, which is the spec-defined global variable across all
-  // environments, then fallback to __global first, because in Node tests both __global and
-  // __window may be defined and _global should be __global in that case.
-  return __globalThis || __global || __window || __self;
-}
-
-const _global = getGlobal();
+// Always use __globalThis if available, which is the spec-defined global variable across all
+// environments, then fallback to __global first, because in Node tests both __global and
+// __window may be defined and _global should be __global in that case.
+const _global = __globalThis || __global || __window || __self;
 
 /**
  * Attention: whenever providing a new value, be sure to add an

packages/core/src/util/ng_i18n_closure_mode.ts

@@ -19,9 +19,12 @@ if (typeof ngI18nClosureMode === 'undefined') {
   // These property accesses can be ignored because ngI18nClosureMode will be set to false
   // when optimizing code and the whole if statement will be dropped.
   // Make sure to refer to ngI18nClosureMode as ['ngI18nClosureMode'] for closure.
-  // tslint:disable-next-line:no-toplevel-property-access
-  global['ngI18nClosureMode'] =
-      // TODO(FW-1250): validate that this actually, you know, works.
-      // tslint:disable-next-line:no-toplevel-property-access
-      typeof goog !== 'undefined' && typeof goog.getMsg === 'function';
+  // NOTE: we need to have it in IIFE so that the tree-shaker is happy.
+  (function() {
+    // tslint:disable-next-line:no-toplevel-property-access
+    global['ngI18nClosureMode'] =
+        // TODO(FW-1250): validate that this actually, you know, works.
+        // tslint:disable-next-line:no-toplevel-property-access
+        typeof goog !== 'undefined' && typeof goog.getMsg === 'function';
+  })();
 }

packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json

@@ -158,9 +158,21 @@
   {
     "name": "ViewEncapsulation"
   },
+  {
+    "name": "__global"
+  },
+  {
+    "name": "__globalThis"
+  },
+  {
+    "name": "__self"
+  },
   {
     "name": "__values"
   },
+  {
+    "name": "__window"
+  },
   {
     "name": "_currentNamespace"
   },
@@ -359,9 +371,6 @@
   {
     "name": "getElementDepthCount"
   },
-  {
-    "name": "getGlobal"
-  },
   {
     "name": "getGuardMask"
   },

packages/core/test/bundling/hello_world/bundle.golden_symbols.json

@@ -125,9 +125,21 @@
   {
     "name": "ViewEncapsulation"
   },
+  {
+    "name": "__global"
+  },
+  {
+    "name": "__globalThis"
+  },
+  {
+    "name": "__self"
+  },
   {
     "name": "__values"
   },
+  {
+    "name": "__window"
+  },
   {
     "name": "_global"
   },
@@ -251,9 +263,6 @@
   {
     "name": "getDirectiveDef"
   },
-  {
-    "name": "getGlobal"
-  },
   {
     "name": "getHighestElementOrICUContainer"
   },

packages/core/test/bundling/injection/bundle.golden_symbols.json

@@ -95,9 +95,6 @@
   {
     "name": "_currentInjector"
   },
-  {
-    "name": "_global"
-  },
   {
     "name": "catchInjectorError"
   },
@@ -119,9 +116,6 @@
   {
     "name": "getClosureSafeProperty"
   },
-  {
-    "name": "getGlobal"
-  },
   {
     "name": "getInheritedInjectableDef"
   },

packages/core/test/bundling/todo/bundle.golden_symbols.json

@@ -302,15 +302,27 @@
   {
     "name": "__forward_ref__"
   },
+  {
+    "name": "__global"
+  },
+  {
+    "name": "__globalThis"
+  },
   {
     "name": "__read"
   },
+  {
+    "name": "__self"
+  },
   {
     "name": "__spread"
   },
   {
     "name": "__values"
   },
+  {
+    "name": "__window"
+  },
   {
     "name": "_activeStylingMapApplyFn"
   },
@@ -809,9 +821,6 @@
   {
     "name": "getErrorLogger"
   },
-  {
-    "name": "getGlobal"
-  },
   {
     "name": "getGuardMask"
   },

tools/public_api_guard/core/core.d.ts

@@ -1136,6 +1136,7 @@ export declare class QueryList<T> {
     readonly first: T;
     readonly last: T;
     readonly length: number;
+    constructor();
     destroy(): void;
     filter(fn: (item: T, index: number, array: T[]) => boolean): T[];
     find(fn: (item: T, index: number, array: T[]) => boolean): T | undefined;