From 49430a3b3c3ef4122b953848a73c90355a9b1e8a Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Fri, 13 Jan 2017 20:19:57 +0000 Subject: [PATCH] fix(upgrade): detect async downgrade component changes (#13812) This commit effectively reverts 7e0f02f96e59863dff563cb7036c21aa58220a67 as it was an invalid fix for #6385, that created a more significant bug, which was that changes were not always being detected. Angular 1 digests should be run inside the ngZone to ensure that async changes are detected. We don't know how to fix #6385 without breaking change detection at this stage. That issue is triggered by async operations, such as `setTimeout`, being triggered inside scope watcher functions. One could argue that watcher functions should be pure and not do work such as triggering async operations. It is possible that the original use case could be supported by moving the debounce logic into the watch listener function, which is only called if the watched value actually changes. Closes #10660, #12318, #12034 PR Close #13812 --- .../@angular/upgrade/src/upgrade_adapter.ts | 11 +-- modules/@angular/upgrade/test/upgrade_spec.ts | 83 ++++++++++++++++++- 2 files changed, 88 insertions(+), 6 deletions(-) diff --git a/modules/@angular/upgrade/src/upgrade_adapter.ts b/modules/@angular/upgrade/src/upgrade_adapter.ts index d19f1dd83bc9c..d4f4b2de176e7 100644 --- a/modules/@angular/upgrade/src/upgrade_adapter.ts +++ b/modules/@angular/upgrade/src/upgrade_adapter.ts @@ -577,10 +577,6 @@ export class UpgradeAdapter { }) .then((ref: NgModuleRef) => { this.moduleRef = ref; - let subscription = this.ngZone.onMicrotaskEmpty.subscribe({ - next: (_: any) => this.ngZone.runOutsideAngular(() => rootScope.$evalAsync()) - }); - rootScope.$on('$destroy', () => { subscription.unsubscribe(); }); this.ngZone.run(() => { if (rootScopePrototype) { rootScopePrototype.$apply = original$applyFn; // restore original $apply @@ -591,7 +587,12 @@ export class UpgradeAdapter { } }); }) - .then(() => this.ng2BootstrapDeferred.resolve(ng1Injector), onError); + .then(() => this.ng2BootstrapDeferred.resolve(ng1Injector), onError) + .then(() => { + let subscription = + this.ngZone.onMicrotaskEmpty.subscribe({next: () => rootScope.$digest()}); + rootScope.$on('$destroy', () => { subscription.unsubscribe(); }); + }); }) .catch((e) => this.ng2BootstrapDeferred.reject(e)); } diff --git a/modules/@angular/upgrade/test/upgrade_spec.ts b/modules/@angular/upgrade/test/upgrade_spec.ts index d1d4be2f81067..9f4dacee0ed0b 100644 --- a/modules/@angular/upgrade/test/upgrade_spec.ts +++ b/modules/@angular/upgrade/test/upgrade_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {ChangeDetectorRef, Class, Component, EventEmitter, NO_ERRORS_SCHEMA, NgModule, SimpleChange, SimpleChanges, Testability, destroyPlatform, forwardRef} from '@angular/core'; +import {ChangeDetectorRef, Class, Component, EventEmitter, Input, NO_ERRORS_SCHEMA, NgModule, NgZone, SimpleChange, SimpleChanges, Testability, destroyPlatform, forwardRef} from '@angular/core'; import {async, fakeAsync, flushMicrotasks, tick} from '@angular/core/testing'; import {BrowserModule} from '@angular/platform-browser'; import {platformBrowserDynamic} from '@angular/platform-browser-dynamic'; @@ -186,6 +186,87 @@ export function main() { ref.dispose(); }); })); + + + it('should propagate changes to a downgraded component inside the ngZone', async(() => { + let appComponent: AppComponent; + let upgradeRef: UpgradeAdapterRef; + + @Component({selector: 'my-app', template: ''}) + class AppComponent { + value: number; + constructor() { appComponent = this; } + } + + @Component({ + selector: 'my-child', + template: '
{{valueFromPromise}}', + }) + class ChildComponent { + valueFromPromise: number; + @Input() + set value(v: number) { expect(NgZone.isInAngularZone()).toBe(true); } + + constructor(private zone: NgZone) {} + + ngOnChanges(changes: SimpleChanges) { + if (changes['value'].isFirstChange()) return; + + this.zone.onMicrotaskEmpty.subscribe(() => { + expect(element.textContent).toEqual('5'); + upgradeRef.dispose(); + }); + + Promise.resolve().then(() => this.valueFromPromise = changes['value'].currentValue); + } + } + + @NgModule({declarations: [AppComponent, ChildComponent], imports: [BrowserModule]}) + class Ng2Module { + } + + const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module)); + const ng1Module = angular.module('ng1', []).directive( + 'myApp', adapter.downgradeNg2Component(AppComponent)); + + const element = html(''); + + adapter.bootstrap(element, ['ng1']).ready((ref) => { + upgradeRef = ref; + appComponent.value = 5; + }); + })); + + // This test demonstrates https://github.com/angular/angular/issues/6385 + // which was invalidly fixed by https://github.com/angular/angular/pull/6386 + // it('should not trigger $digest from an async operation in a watcher', async(() => { + // @Component({selector: 'my-app', template: ''}) + // class AppComponent { + // } + + // @NgModule({declarations: [AppComponent], imports: [BrowserModule]}) + // class Ng2Module { + // } + + // const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module)); + // const ng1Module = angular.module('ng1', []).directive( + // 'myApp', adapter.downgradeNg2Component(AppComponent)); + + // const element = html(''); + + // adapter.bootstrap(element, ['ng1']).ready((ref) => { + // let doTimeout = false; + // let timeoutId: number; + // ref.ng1RootScope.$watch(() => { + // if (doTimeout && !timeoutId) { + // timeoutId = window.setTimeout(function() { + // timeoutId = null; + // }, 10); + // } + // }); + // doTimeout = true; + // }); + // })); }); describe('downgrade ng2 component', () => {