fix(upgrade): populate upgraded component's view before creating the controller (#14289)

Previously, the relative order of the AngularJS compiling/linking operations was
not similar to AngularJS's, resulting in inconsistent behavior for upgraded
components (which made upgrading to Angular less straight forward).

This commit fixes it, by following the compiling/linking process of AngularJS
more closely.

Main differences:

- The components view is already populated when the controller is instantiated
  (and subsequent hooks are called).
- The correct DOM content is available when running the `$onChanges`, `$onInit`,
  `$doCheck` hooks. Previously, the "content children" were still present, not
  the "view children".
- The same for pre-linking.
- The template is compiled in the correct DOM context (e.g. has access to
  ancestors). Previously, it was compiled in isolation, inside a dummy element.

For reference, here is the order of operations:

**Before**

1. Compile template
2. Instantiate controller
3. Hook: $onChanges
4. Hook: $onInit
5. Hook: $doCheck
6. Pre-linking
7. Collect content children
8. Insert compiled template
9. Linking
10. Post-linking
11. Hook: $postLink

**After**

1. Collect content children
2. Insert template
3. Compile template
4. Instantiate controller
5. Hook: $onChanges
6. Hook: $onInit
7. Hook: $doCheck
8. Pre-linking
9. Linking
10. Post-linking
11. Hook: $postLink

Fixes #13912

modules/@angular/upgrade/src/common/angular1.ts

@@ -113,6 +113,7 @@ export interface ICloneAttachFunction {
 export type IAugmentedJQuery = Node[] & {
   bind?: (name: string, fn: () => void) => void;
   data?: (name: string, value?: any) => any;
+  text?: () => string;
   inheritedData?: (name: string, value?: any) => any;
   contents?: () => IAugmentedJQuery;
   parent?: () => IAugmentedJQuery;

modules/@angular/upgrade/src/static/upgrade_component.ts

@@ -93,10 +93,13 @@ export class UpgradeComponent implements OnInit, OnChanges, DoCheck, OnDestroy {
 
   private directive: angular.IDirective;
   private bindings: Bindings;
-  private linkFn: angular.ILinkFn;
 
-  private controllerInstance: IControllerInstance = null;
+  private controllerInstance: IControllerInstance;
-  private bindingDestination: IBindingDestination = null;
+  private bindingDestination: IBindingDestination;
+
+  // We will be instantiating the controller in the `ngOnInit` hook, when the first `ngOnChanges`
+  // will have been already triggered. We store the `SimpleChanges` and "play them back" later.
+  private pendingChanges: SimpleChanges;
 
   private unregisterDoCheckWatcher: Function;
 
@@ -128,7 +131,6 @@ export class UpgradeComponent implements OnInit, OnChanges, DoCheck, OnDestroy {
 
     this.directive = this.getDirective(name);
     this.bindings = this.initializeBindings(this.directive);
-    this.linkFn = this.compileTemplate(this.directive);
 
     // We ask for the AngularJS scope from the Angular injector, since
     // we will put the new component scope onto the new injector for each component
@@ -137,6 +139,15 @@ export class UpgradeComponent implements OnInit, OnChanges, DoCheck, OnDestroy {
     // QUESTION 2: Should we make the scope accessible through `$element.scope()/isolateScope()`?
     this.$componentScope = $parentScope.$new(!!this.directive.scope);
 
+    this.initializeOutputs();
+  }
+
+  ngOnInit() {
+    // Collect contents, insert and compile template
+    const contentChildNodes = this.extractChildNodes(this.element);
+    const linkFn = this.compileTemplate(this.directive);
+
+    // Instantiate controller
     const controllerType = this.directive.controller;
     const bindToController = this.directive.bindToController;
     if (controllerType) {
@@ -144,17 +155,14 @@ export class UpgradeComponent implements OnInit, OnChanges, DoCheck, OnDestroy {
           controllerType, this.$componentScope, this.$element, this.directive.controllerAs);
     } else if (bindToController) {
       throw new Error(
-          `Upgraded directive '${name}' specifies 'bindToController' but no controller.`);
+          `Upgraded directive '${this.directive.name}' specifies 'bindToController' but no controller.`);
     }
 
+    // Set up outputs
     this.bindingDestination = bindToController ? this.controllerInstance : this.$componentScope;
+    this.bindOutputs();
 
-    this.setupOutputs();
+    // Require other controllers
-  }
-
-  ngOnInit() {
-    const attrs: angular.IAttributes = NOT_SUPPORTED;
-    const transcludeFn: angular.ITranscludeFunction = NOT_SUPPORTED;
     const directiveRequire = this.getDirectiveRequire(this.directive);
     const requiredControllers =
         this.resolveRequire(this.directive.name, this.$element, directiveRequire);
@@ -166,10 +174,18 @@ export class UpgradeComponent implements OnInit, OnChanges, DoCheck, OnDestroy {
       });
     }
 
+    // Hook: $onChanges
+    if (this.pendingChanges) {
+      this.forwardChanges(this.pendingChanges);
+      this.pendingChanges = null;
+    }
+
+    // Hook: $onInit
     if (this.controllerInstance && isFunction(this.controllerInstance.$onInit)) {
       this.controllerInstance.$onInit();
     }
 
+    // Hook: $doCheck
     if (this.controllerInstance && isFunction(this.controllerInstance.$doCheck)) {
       const callDoCheck = () => this.controllerInstance.$doCheck();
 
@@ -177,42 +193,35 @@ export class UpgradeComponent implements OnInit, OnChanges, DoCheck, OnDestroy {
       callDoCheck();
     }
 
+    // Linking
     const link = this.directive.link;
     const preLink = (typeof link == 'object') && (link as angular.IDirectivePrePost).pre;
     const postLink = (typeof link == 'object') ? (link as angular.IDirectivePrePost).post : link;
+    const attrs: angular.IAttributes = NOT_SUPPORTED;
+    const transcludeFn: angular.ITranscludeFunction = NOT_SUPPORTED;
     if (preLink) {
       preLink(this.$componentScope, this.$element, attrs, requiredControllers, transcludeFn);
     }
 
-    const childNodes: Node[] = [];
+    const attachChildNodes: angular.ILinkFn = (scope, cloneAttach) =>
-    let childNode: Node;
+        cloneAttach(contentChildNodes);
-    while (childNode = this.element.firstChild) {
+    linkFn(this.$componentScope, null, {parentBoundTranscludeFn: attachChildNodes});
-      this.element.removeChild(childNode);
-      childNodes.push(childNode);
-    }
-
-    const attachElement: angular.ICloneAttachFunction =
-        (clonedElements, scope) => { this.$element.append(clonedElements); };
-    const attachChildNodes: angular.ILinkFn = (scope, cloneAttach) => cloneAttach(childNodes);
-
-    this.linkFn(this.$componentScope, attachElement, {parentBoundTranscludeFn: attachChildNodes});
 
     if (postLink) {
       postLink(this.$componentScope, this.$element, attrs, requiredControllers, transcludeFn);
     }
 
+    // Hook: $postLink
     if (this.controllerInstance && isFunction(this.controllerInstance.$postLink)) {
       this.controllerInstance.$postLink();
     }
   }
 
   ngOnChanges(changes: SimpleChanges) {
-    // Forward input changes to `bindingDestination`
+    if (!this.bindingDestination) {
-    Object.keys(changes).forEach(
+      this.pendingChanges = changes;
-        propName => this.bindingDestination[propName] = changes[propName].currentValue);
+    } else {
-
+      this.forwardChanges(changes);
-    if (isFunction(this.bindingDestination.$onChanges)) {
-      this.bindingDestination.$onChanges(changes);
     }
   }
 
@@ -324,6 +333,18 @@ export class UpgradeComponent implements OnInit, OnChanges, DoCheck, OnDestroy {
     return bindings;
   }
 
+  private extractChildNodes(element: Element): Node[] {
+    const childNodes: Node[] = [];
+    let childNode: Node;
+
+    while (childNode = element.firstChild) {
+      element.removeChild(childNode);
+      childNodes.push(childNode);
+    }
+
+    return childNodes;
+  }
+
   private compileTemplate(directive: angular.IDirective): angular.ILinkFn {
     if (this.directive.template !== undefined) {
       return this.compileHtml(getOrCall(this.directive.template));
@@ -403,33 +424,43 @@ export class UpgradeComponent implements OnInit, OnChanges, DoCheck, OnDestroy {
     }
   }
 
-  private setupOutputs() {
+  private initializeOutputs() {
-    // Set up the outputs for `=` bindings
+    // Initialize the outputs for `=` and `&` bindings
-    this.bindings.twoWayBoundProperties.forEach(propName => {
+    this.bindings.twoWayBoundProperties.concat(this.bindings.expressionBoundProperties)
-      const outputName = this.bindings.propertyToOutputMap[propName];
+        .forEach(propName => {
-      (this as any)[outputName] = new EventEmitter();
+          const outputName = this.bindings.propertyToOutputMap[propName];
-    });
+          (this as any)[outputName] = new EventEmitter();
+        });
+  }
 
-    // Set up the outputs for `&` bindings
+  private bindOutputs() {
+    // Bind `&` bindings to the corresponding outputs
     this.bindings.expressionBoundProperties.forEach(propName => {
       const outputName = this.bindings.propertyToOutputMap[propName];
-      const emitter = (this as any)[outputName] = new EventEmitter();
+      const emitter = (this as any)[outputName];
 
-      // QUESTION: Do we want the ng1 component to call the function with `<value>` or with
-      //           `{$event: <value>}`. The former is closer to ng2, the latter to ng1.
       this.bindingDestination[propName] = (value: any) => emitter.emit(value);
     });
   }
 
+  private forwardChanges(changes: SimpleChanges) {
+    // Forward input changes to `bindingDestination`
+    Object.keys(changes).forEach(
+        propName => this.bindingDestination[propName] = changes[propName].currentValue);
+
+    if (isFunction(this.bindingDestination.$onChanges)) {
+      this.bindingDestination.$onChanges(changes);
+    }
+  }
+
   private notSupported(feature: string) {
     throw new Error(
         `Upgraded directive '${this.name}' contains unsupported feature: '${feature}'.`);
   }
 
   private compileHtml(html: string): angular.ILinkFn {
-    const div = document.createElement('div');
+    this.element.innerHTML = html;
-    div.innerHTML = html;
+    return this.$compile(this.element.childNodes);
-    return this.$compile(div.childNodes);
   }
 }
 

modules/@angular/upgrade/test/static/integration/upgrade_component_spec.ts

@@ -1021,6 +1021,54 @@ export function main() {
          }));
     });
 
+    describe('compiling', () => {
+      it('should compile the ng1 template in the correct DOM context', async(() => {
+           let grandParentNodeName: string;
+
+           // Define `ng1Component`
+           const ng1ComponentA: angular.IComponent = {template: 'ng1A(<ng1-b></ng1-b>)'};
+           const ng1DirectiveB: angular.IDirective = {
+             compile: tElem => grandParentNodeName = tElem.parent().parent()[0].nodeName
+           };
+
+           // Define `Ng1ComponentAFacade`
+           @Directive({selector: 'ng1A'})
+           class Ng1ComponentAFacade extends UpgradeComponent {
+             constructor(elementRef: ElementRef, injector: Injector) {
+               super('ng1A', elementRef, injector);
+             }
+           }
+
+           // Define `Ng2ComponentX`
+           @Component({selector: 'ng2-x', template: 'ng2X(<ng1A></ng1A>)'})
+           class Ng2ComponentX {
+           }
+
+           // Define `ng1Module`
+           const ng1Module = angular.module('ng1', [])
+                                 .component('ng1A', ng1ComponentA)
+                                 .directive('ng1B', () => ng1DirectiveB)
+                                 .directive('ng2X', downgradeComponent({component: Ng2ComponentX}));
+
+           // Define `Ng2Module`
+           @NgModule({
+             imports: [BrowserModule, UpgradeModule],
+             declarations: [Ng1ComponentAFacade, Ng2ComponentX],
+             entryComponents: [Ng2ComponentX],
+           })
+           class Ng2Module {
+             ngDoBootstrap() {}
+           }
+
+           // Bootstrap
+           const element = html(`<ng2-x></ng2-x>`);
+
+           bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then(() => {
+             expect(grandParentNodeName).toBe('NG2-X');
+           });
+         }));
+    });
+
     describe('controller', () => {
       it('should support `controllerAs`', async(() => {
            // Define `ng1Directive`
@@ -1254,6 +1302,60 @@ export function main() {
              expect(multiTrim(element.textContent)).toBe('WORKS GREAT');
            });
          }));
+
+      it('should insert the compiled content before instantiating the controller', async(() => {
+           let compiledContent: string;
+           let getCurrentContent: () => string;
+
+           // Define `ng1Component`
+           const ng1Component: angular.IComponent = {
+             template: 'Hello, {{ $ctrl.name }}!',
+             controller: class {
+               name = 'world';
+
+               constructor($element: angular.IAugmentedJQuery) {
+                 getCurrentContent = () => $element.text();
+                 compiledContent = getCurrentContent();
+               }
+             }
+           };
+
+           // Define `Ng1ComponentFacade`
+           @Directive({selector: 'ng1'})
+           class Ng1ComponentFacade extends UpgradeComponent {
+             constructor(elementRef: ElementRef, injector: Injector) {
+               super('ng1', elementRef, injector);
+             }
+           }
+
+           // Define `Ng2Component`
+           @Component({selector: 'ng2', template: '<ng1></ng1>'})
+           class Ng2Component {
+           }
+
+           // Define `ng1Module`
+           const ng1Module = angular.module('ng1Module', [])
+                                 .component('ng1', ng1Component)
+                                 .directive('ng2', downgradeComponent({component: Ng2Component}));
+
+           // Define `Ng2Module`
+           @NgModule({
+             imports: [BrowserModule, UpgradeModule],
+             declarations: [Ng1ComponentFacade, Ng2Component],
+             entryComponents: [Ng2Component]
+           })
+           class Ng2Module {
+             ngDoBootstrap() {}
+           }
+
+           // Bootstrap
+           const element = html(`<ng2></ng2>`);
+
+           bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then(() => {
+             expect(multiTrim(compiledContent)).toBe('Hello, {{ $ctrl.name }}!');
+             expect(multiTrim(getCurrentContent())).toBe('Hello, world!');
+           });
+         }));
     });
 
     describe('require', () => {
@@ -1785,13 +1887,8 @@ export function main() {
              scope: {inputB: '<'},
              bindToController: true,
              controllerAs: '$ctrl',
-             controller: class {
+             controller:
-               constructor($scope: angular.IScope) {
+                 class {$onChanges(changes: SimpleChanges) { controllerOnChangesB(changes); }}
-                 Object.getPrototypeOf($scope)['$onChanges'] = scopeOnChanges;
-               }
-
-               $onChanges(changes: SimpleChanges) { controllerOnChangesB(changes); }
-             }
            };
 
            // Define `Ng1ComponentFacade`
@@ -1828,7 +1925,10 @@ export function main() {
            const ng1Module = angular.module('ng1Module', [])
                                  .directive('ng1A', () => ng1DirectiveA)
                                  .directive('ng1B', () => ng1DirectiveB)
-                                 .directive('ng2', downgradeComponent({component: Ng2Component}));
+                                 .directive('ng2', downgradeComponent({component: Ng2Component}))
+                                 .run(($rootScope: angular.IRootScopeService) => {
+                                   Object.getPrototypeOf($rootScope)['$onChanges'] = scopeOnChanges;
+                                 });
 
            // Define `Ng2Module`
            @NgModule({