From eef7d8aa11baf394d9448bbffe24ed700480a96a Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Tue, 26 Sep 2017 03:07:12 +0300 Subject: [PATCH] fix(upgrade): call `ngOnInit()` after `ngOnChanges()` (on components with inputs) Fixes #18913 --- .../src/common/downgrade_component_adapter.ts | 17 +- .../integration/downgrade_module_spec.ts | 177 +++++++++++++++++- 2 files changed, 178 insertions(+), 16 deletions(-) diff --git a/packages/upgrade/src/common/downgrade_component_adapter.ts b/packages/upgrade/src/common/downgrade_component_adapter.ts index 906bd21869..74509791f5 100644 --- a/packages/upgrade/src/common/downgrade_component_adapter.ts +++ b/packages/upgrade/src/common/downgrade_component_adapter.ts @@ -99,7 +99,7 @@ export class DowngradeComponentAdapter { })(input.prop); attrs.$observe(input.attr, observeFn); - // Use `$watch()` (in addition to `$observe()`) in order to initialize the input in time + // Use `$watch()` (in addition to `$observe()`) in order to initialize the input in time // for `ngOnChanges()`. This is necessary if we are already in a `$digest`, which means that // `ngOnChanges()` (which is called by a watcher) will run before the `$observe()` callback. let unwatch: Function|null = this.componentScope.$watch(() => { @@ -138,8 +138,7 @@ export class DowngradeComponentAdapter { (this.component).ngOnChanges(inputChanges !); } - // If opted out of propagating digests, invoke change detection - // when inputs change + // If opted out of propagating digests, invoke change detection when inputs change. if (!propagateDigest) { detectChanges(); } @@ -150,10 +149,16 @@ export class DowngradeComponentAdapter { this.componentScope.$watch(this.wrapCallback(detectChanges)); } - // Attach the view so that it will be dirty-checked. + // If necessary, attach the view so that it will be dirty-checked. + // (Allow time for the initial input values to be set and `ngOnChanges()` to be called.) if (needsNgZone || !propagateDigest) { - const appRef = this.parentInjector.get(ApplicationRef); - appRef.attachView(this.componentRef.hostView); + let unwatch: Function|null = this.componentScope.$watch(() => { + unwatch !(); + unwatch = null; + + const appRef = this.parentInjector.get(ApplicationRef); + appRef.attachView(this.componentRef.hostView); + }); } } diff --git a/packages/upgrade/test/static/integration/downgrade_module_spec.ts b/packages/upgrade/test/static/integration/downgrade_module_spec.ts index 71012a6942..52e2da16dd 100644 --- a/packages/upgrade/test/static/integration/downgrade_module_spec.ts +++ b/packages/upgrade/test/static/integration/downgrade_module_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {ApplicationRef, Component, Inject, Injector, Input, NgModule, NgZone, OnChanges, OnDestroy, StaticProvider, ViewRef, destroyPlatform} from '@angular/core'; +import {AfterContentChecked, AfterContentInit, AfterViewChecked, AfterViewInit, ApplicationRef, Component, DoCheck, Inject, Injector, Input, NgModule, NgZone, OnChanges, OnDestroy, OnInit, StaticProvider, ViewRef, 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'; @@ -16,7 +16,7 @@ import {$ROOT_SCOPE, INJECTOR_KEY, LAZY_MODULE_REF} from '@angular/upgrade/src/c import {LazyModuleRef} from '@angular/upgrade/src/common/util'; import {downgradeComponent, downgradeModule} from '@angular/upgrade/static'; -import {html} from '../test_helpers'; +import {html, multiTrim} from '../test_helpers'; export function main() { @@ -306,6 +306,162 @@ export function main() { }); })); + it('should run the lifecycle hooks in the correct order', async(() => { + const logs: string[] = []; + let rootScope: angular.IRootScopeService; + + @Component({ + selector: 'ng2', + template: ` + {{ value }} + + + ` + }) + class Ng2Component implements AfterContentChecked, + AfterContentInit, AfterViewChecked, AfterViewInit, DoCheck, OnChanges, OnDestroy, + OnInit { + @Input() value = 'foo'; + + ngAfterContentChecked() { this.log('AfterContentChecked'); } + ngAfterContentInit() { this.log('AfterContentInit'); } + ngAfterViewChecked() { this.log('AfterViewChecked'); } + ngAfterViewInit() { this.log('AfterViewInit'); } + ngDoCheck() { this.log('DoCheck'); } + ngOnChanges() { this.log('OnChanges'); } + ngOnDestroy() { this.log('OnDestroy'); } + ngOnInit() { this.log('OnInit'); } + + private log(hook: string) { logs.push(`${hook}(${this.value})`); } + } + + @NgModule({ + declarations: [Ng2Component], + entryComponents: [Ng2Component], + imports: [BrowserModule], + }) + class Ng2Module { + ngDoBootstrap() {} + } + + const bootstrapFn = (extraProviders: StaticProvider[]) => + platformBrowserDynamic(extraProviders).bootstrapModule(Ng2Module); + const lazyModuleName = downgradeModule(bootstrapFn); + const ng1Module = + angular.module('ng1', [lazyModuleName]) + .directive('ng2', downgradeComponent({component: Ng2Component, propagateDigest})) + .run(($rootScope: angular.IRootScopeService) => { + rootScope = $rootScope; + rootScope.value = 'bar'; + }); + + const element = + html('
Content
'); + angular.bootstrap(element, [ng1Module.name]); + + setTimeout(() => { // Wait for the module to be bootstrapped. + setTimeout(() => { // Wait for `$evalAsync()` to propagate inputs. + const button = element.querySelector('button') !; + + // Once initialized. + expect(multiTrim(element.textContent)).toBe('bar Content'); + expect(logs).toEqual([ + // `ngOnChanges()` call triggered directly through the `inputChanges` $watcher. + 'OnChanges(bar)', + // Initial CD triggered directly through the `detectChanges()` or `inputChanges` + // $watcher (for `propagateDigest` true/false respectively). + 'OnInit(bar)', + 'DoCheck(bar)', + 'AfterContentInit(bar)', + 'AfterContentChecked(bar)', + 'AfterViewInit(bar)', + 'AfterViewChecked(bar)', + ...(propagateDigest ? + [ + // CD triggered directly through the `detectChanges()` $watcher (2nd + // $digest). + 'DoCheck(bar)', + 'AfterContentChecked(bar)', + 'AfterViewChecked(bar)', + ] : + []), + // CD triggered due to entering/leaving the NgZone (in `downgradeFn()`). + 'DoCheck(bar)', + 'AfterContentChecked(bar)', + 'AfterViewChecked(bar)', + ]); + logs.length = 0; + + // Change inputs and run `$digest`. + rootScope.$apply('value = "baz"'); + expect(multiTrim(element.textContent)).toBe('baz Content'); + expect(logs).toEqual([ + // `ngOnChanges()` call triggered directly through the `inputChanges` $watcher. + 'OnChanges(baz)', + // `propagateDigest: true` (3 CD runs): + // - CD triggered due to entering/leaving the NgZone (in `inputChanges` + // $watcher). + // - CD triggered directly through the `detectChanges()` $watcher. + // - CD triggered due to entering/leaving the NgZone (in `detectChanges` + // $watcher). + // `propagateDigest: false` (2 CD runs): + // - CD triggered directly through the `inputChanges` $watcher. + // - CD triggered due to entering/leaving the NgZone (in `inputChanges` + // $watcher). + 'DoCheck(baz)', + 'AfterContentChecked(baz)', + 'AfterViewChecked(baz)', + 'DoCheck(baz)', + 'AfterContentChecked(baz)', + 'AfterViewChecked(baz)', + ...(propagateDigest ? + [ + 'DoCheck(baz)', + 'AfterContentChecked(baz)', + 'AfterViewChecked(baz)', + ] : + []), + ]); + logs.length = 0; + + // Run `$digest` (without changing inputs). + rootScope.$digest(); + expect(multiTrim(element.textContent)).toBe('baz Content'); + expect(logs).toEqual( + propagateDigest ? + [ + // CD triggered directly through the `detectChanges()` $watcher. + 'DoCheck(baz)', + 'AfterContentChecked(baz)', + 'AfterViewChecked(baz)', + // CD triggered due to entering/leaving the NgZone (in the above $watcher). + 'DoCheck(baz)', + 'AfterContentChecked(baz)', + 'AfterViewChecked(baz)', + ] : + []); + logs.length = 0; + + // Trigger change detection (without changing inputs). + button.click(); + expect(multiTrim(element.textContent)).toBe('qux Content'); + expect(logs).toEqual([ + 'DoCheck(qux)', + 'AfterContentChecked(qux)', + 'AfterViewChecked(qux)', + ]); + logs.length = 0; + + // Destroy the component. + rootScope.$apply('hideNg2 = true'); + expect(logs).toEqual([ + 'OnDestroy(qux)', + ]); + logs.length = 0; + }); + }); + })); + it('should detach hostViews from the ApplicationRef once destroyed', async(() => { let ng2Component: Ng2Component; @@ -339,17 +495,18 @@ export function main() { const $injector = angular.bootstrap(element, [ng1Module.name]); const $rootScope = $injector.get($ROOT_SCOPE) as angular.IRootScopeService; - // Wait for the module to be bootstrapped. - setTimeout(() => { - const hostView: ViewRef = - (ng2Component.appRef.attachView as jasmine.Spy).calls.mostRecent().args[0]; + setTimeout(() => { // Wait for the module to be bootstrapped. + setTimeout(() => { // Wait for the hostView to be attached (during the `$digest`). + const hostView: ViewRef = + (ng2Component.appRef.attachView as jasmine.Spy).calls.mostRecent().args[0]; - expect(hostView.destroyed).toBe(false); + expect(hostView.destroyed).toBe(false); - $rootScope.$apply('hideNg2 = true'); + $rootScope.$apply('hideNg2 = true'); - expect(hostView.destroyed).toBe(true); - expect(ng2Component.appRef.detachView).toHaveBeenCalledWith(hostView); + expect(hostView.destroyed).toBe(true); + expect(ng2Component.appRef.detachView).toHaveBeenCalledWith(hostView); + }); }); }));