fix(upgrade): compile downgraded components synchronously (if possible)

packages/upgrade/src/common/src/downgrade_component.ts

@@ -11,13 +11,10 @@ import {ComponentFactory, ComponentFactoryResolver, Injector, NgZone, Type} from
 import {IAnnotatedFunction, IAttributes, IAugmentedJQuery, ICompileService, IDirective, IInjectorService, INgModelController, IParseService, IScope} from './angular1';
 import {$COMPILE, $INJECTOR, $PARSE, INJECTOR_KEY, LAZY_MODULE_REF, REQUIRE_INJECTOR, REQUIRE_NG_MODEL} from './constants';
 import {DowngradeComponentAdapter} from './downgrade_component_adapter';
-import {LazyModuleRef, UpgradeAppType, controllerKey, getDowngradedModuleCount, getTypeName, getUpgradeAppType, isFunction, validateInjectionKey} from './util';
+import {SyncPromise, Thenable, isThenable} from './promise_util';
+import {LazyModuleRef, UpgradeAppType, controllerKey, getDowngradedModuleCount, getTypeName, getUpgradeAppType, validateInjectionKey} from './util';
 
 
-interface Thenable<T> {
-  then(callback: (value: T) => any): any;
-}
-
 /**
  * @description
  *
@@ -199,12 +196,8 @@ export function downgradeComponent(info: {
               wrapCallback(() => doDowngrade(pInjector, mInjector))();
             };
 
-        if (isThenable(finalParentInjector) || isThenable(finalModuleInjector)) {
-          Promise.all([finalParentInjector, finalModuleInjector])
-              .then(([pInjector, mInjector]) => downgradeFn(pInjector, mInjector));
-        } else {
-          downgradeFn(finalParentInjector, finalModuleInjector);
-        }
+        ParentInjectorPromise.all([finalParentInjector, finalModuleInjector])
+            .then(([pInjector, mInjector]) => downgradeFn(pInjector, mInjector));
 
         ranAsync = true;
       }
@@ -218,42 +211,26 @@ export function downgradeComponent(info: {
 
 /**
  * Synchronous promise-like object to wrap parent injectors,
- * to preserve the synchronous nature of Angular 1's $compile.
+ * to preserve the synchronous nature of AngularJS's `$compile`.
  */
-class ParentInjectorPromise {
-  // TODO(issue/24571): remove '!'.
-  private injector !: Injector;
+class ParentInjectorPromise extends SyncPromise<Injector> {
   private injectorKey: string = controllerKey(INJECTOR_KEY);
-  private callbacks: ((injector: Injector) => any)[] = [];
 
   constructor(private element: IAugmentedJQuery) {
+    super();
+
     // Store the promise on the element.
     element.data !(this.injectorKey, this);
   }
 
-  then(callback: (injector: Injector) => any) {
-    if (this.injector) {
-      callback(this.injector);
-    } else {
-      this.callbacks.push(callback);
-    }
-  }
-
-  resolve(injector: Injector) {
-    this.injector = injector;
-
+  resolve(injector: Injector): void {
     // Store the real injector on the element.
     this.element.data !(this.injectorKey, injector);
 
     // Release the element to prevent memory leaks.
     this.element = null !;
 
-    // Run the queued callbacks.
-    this.callbacks.forEach(callback => callback(injector));
-    this.callbacks.length = 0;
+    // Resolve the promise.
+    super.resolve(injector);
   }
 }
-
-function isThenable<T>(obj: object): obj is Thenable<T> {
-  return isFunction((obj as any).then);
-}

packages/upgrade/src/common/src/promise_util.ts

@@ -0,0 +1,65 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {isFunction} from './util';
+
+export interface Thenable<T> { then(callback: (value: T) => any): any; }
+
+export function isThenable<T>(obj: unknown): obj is Thenable<T> {
+  return !!obj && isFunction((obj as any).then);
+}
+
+/**
+ * Synchronous, promise-like object.
+ */
+export class SyncPromise<T> {
+  protected value: T|undefined;
+  private resolved = false;
+  private callbacks: ((value: T) => unknown)[] = [];
+
+  static all<T>(valuesOrPromises: (T|Thenable<T>)[]): SyncPromise<T[]> {
+    const aggrPromise = new SyncPromise<T[]>();
+
+    let resolvedCount = 0;
+    const results: T[] = [];
+    const resolve = (idx: number, value: T) => {
+      results[idx] = value;
+      if (++resolvedCount === valuesOrPromises.length) aggrPromise.resolve(results);
+    };
+
+    valuesOrPromises.forEach((p, idx) => {
+      if (isThenable(p)) {
+        p.then(v => resolve(idx, v));
+      } else {
+        resolve(idx, p);
+      }
+    });
+
+    return aggrPromise;
+  }
+
+  resolve(value: T): void {
+    // Do nothing, if already resolved.
+    if (this.resolved) return;
+
+    this.value = value;
+    this.resolved = true;
+
+    // Run the queued callbacks.
+    this.callbacks.forEach(callback => callback(value));
+    this.callbacks.length = 0;
+  }
+
+  then(callback: (value: T) => unknown): void {
+    if (this.resolved) {
+      callback(this.value !);
+    } else {
+      this.callbacks.push(callback);
+    }
+  }
+}

packages/upgrade/src/common/test/promise_util_spec.ts

@@ -0,0 +1,120 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {SyncPromise, isThenable} from '../src/promise_util';
+
+describe('isThenable()', () => {
+  it('should return false for primitive values', () => {
+    expect(isThenable(undefined)).toBe(false);
+    expect(isThenable(null)).toBe(false);
+    expect(isThenable(false)).toBe(false);
+    expect(isThenable(true)).toBe(false);
+    expect(isThenable(0)).toBe(false);
+    expect(isThenable(1)).toBe(false);
+    expect(isThenable('')).toBe(false);
+    expect(isThenable('foo')).toBe(false);
+  });
+
+  it('should return false if `.then` is not a function', () => {
+    expect(isThenable([])).toBe(false);
+    expect(isThenable(['then'])).toBe(false);
+    expect(isThenable(function() {})).toBe(false);
+    expect(isThenable({})).toBe(false);
+    expect(isThenable({then: true})).toBe(false);
+    expect(isThenable({then: 'not a function'})).toBe(false);
+
+  });
+
+  it('should return true if `.then` is a function', () => {
+    expect(isThenable({then: function() {}})).toBe(true);
+    expect(isThenable({then: () => {}})).toBe(true);
+    expect(isThenable(Object.assign('thenable', {then: () => {}}))).toBe(true);
+  });
+});
+
+describe('SyncPromise', () => {
+  it('should call all callbacks once resolved', () => {
+    const spy1 = jasmine.createSpy('spy1');
+    const spy2 = jasmine.createSpy('spy2');
+
+    const promise = new SyncPromise<string>();
+    promise.then(spy1);
+    promise.then(spy2);
+
+    expect(spy1).not.toHaveBeenCalled();
+    expect(spy2).not.toHaveBeenCalled();
+
+    promise.resolve('foo');
+
+    expect(spy1).toHaveBeenCalledWith('foo');
+    expect(spy2).toHaveBeenCalledWith('foo');
+  });
+
+  it('should call callbacks immediately if already resolved', () => {
+    const spy = jasmine.createSpy('spy');
+
+    const promise = new SyncPromise<string>();
+    promise.resolve('foo');
+    promise.then(spy);
+
+    expect(spy).toHaveBeenCalledWith('foo');
+  });
+
+  it('should ignore subsequent calls to `resolve()`', () => {
+    const spy = jasmine.createSpy('spy');
+
+    const promise = new SyncPromise<string>();
+
+    promise.then(spy);
+    promise.resolve('foo');
+    expect(spy).toHaveBeenCalledWith('foo');
+
+    spy.calls.reset();
+
+    promise.resolve('bar');
+    expect(spy).not.toHaveBeenCalled();
+
+    promise.then(spy);
+    promise.resolve('baz');
+
+    expect(spy).toHaveBeenCalledTimes(1);
+    expect(spy).toHaveBeenCalledWith('foo');
+  });
+
+  describe('.all()', () => {
+    it('should return a `SyncPromise` instance',
+       () => { expect(SyncPromise.all([])).toEqual(jasmine.any(SyncPromise)); });
+
+    it('should resolve immediately if the provided values are not thenable', () => {
+      const spy = jasmine.createSpy('spy');
+
+      const promise = SyncPromise.all(['foo', 1, {then: false}, []]);
+      promise.then(spy);
+
+      expect(spy).toHaveBeenCalledWith(['foo', 1, {then: false}, []]);
+    });
+
+    it('should wait for any thenables to resolve', async() => {
+      const spy = jasmine.createSpy('spy');
+
+      const v1 = 'foo';
+      const v2 = new SyncPromise<string>();
+      const v3 = Promise.resolve('baz');
+      const promise = SyncPromise.all([v1, v2, v3]);
+
+      promise.then(spy);
+      expect(spy).not.toHaveBeenCalled();
+
+      v2.resolve('bar');
+      expect(spy).not.toHaveBeenCalled();
+
+      await v3;
+      expect(spy).toHaveBeenCalledWith(['foo', 'bar', 'baz']);
+    });
+  });
+});

packages/upgrade/static/test/integration/downgrade_component_spec.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ChangeDetectionStrategy, ChangeDetectorRef, Compiler, Component, ComponentFactoryResolver, Directive, ElementRef, EventEmitter, Injector, Input, NgModule, NgModuleRef, OnChanges, OnDestroy, Output, SimpleChanges, destroyPlatform} from '@angular/core';
+import {ChangeDetectionStrategy, Compiler, Component, Directive, ElementRef, EventEmitter, Injector, Input, NgModule, NgModuleRef, OnChanges, OnDestroy, Output, SimpleChanges, destroyPlatform} from '@angular/core';
 import {async, fakeAsync, tick} from '@angular/core/testing';
 import {BrowserModule} from '@angular/platform-browser';
 import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
@@ -736,6 +736,35 @@ withEachNg1Version(() => {
          });
        }));
 
+    it('should be compiled synchronously, if possible', async(() => {
+         @Component({selector: 'ng2A', template: '<ng-content></ng-content>'})
+         class Ng2ComponentA {
+         }
+
+         @Component({selector: 'ng2B', template: '{{ \'Ng2 template\' }}'})
+         class Ng2ComponentB {
+         }
+
+         @NgModule({
+           declarations: [Ng2ComponentA, Ng2ComponentB],
+           entryComponents: [Ng2ComponentA, Ng2ComponentB],
+           imports: [BrowserModule, UpgradeModule],
+         })
+         class Ng2Module {
+           ngDoBootstrap() {}
+         }
+
+         const ng1Module = angular.module_('ng1', [])
+                               .directive('ng2A', downgradeComponent({component: Ng2ComponentA}))
+                               .directive('ng2B', downgradeComponent({component: Ng2ComponentB}));
+
+         const element = html('<ng2-a><ng2-b></ng2-b></ng2-a>');
+
+         bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then(() => {
+           expect(element.textContent).toBe('Ng2 template');
+         });
+       }));
+
     it('should work with ng2 lazy loaded components', async(() => {
          let componentInjector: Injector;