Skip to content

Commit

Permalink
fix(upgrade): detect async downgrade component changes (#14039)
Browse files Browse the repository at this point in the history
This commit effectively reverts 7e0f02f but for `upgrade/static`
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.

See #13812

PR Close #14039
  • Loading branch information
petebacondarwin authored and mhevery committed Jan 27, 2017
1 parent 665dde2 commit 20b454c
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 7 deletions.
18 changes: 12 additions & 6 deletions modules/@angular/upgrade/src/aot/upgrade_module.ts
Expand Up @@ -170,12 +170,13 @@ export class UpgradeModule {
const injector = this.injector;
// Cannot use arrow function below because we need the context
const newWhenStable = function(callback: Function) {
originalWhenStable.call(this, function() {
originalWhenStable.call(testabilityDelegate, function() {
const ng2Testability: Testability = injector.get(Testability);
if (ng2Testability.isStable()) {
callback.apply(this, arguments);
callback();
} else {
ng2Testability.whenStable(newWhenStable.bind(this, callback));
ng2Testability.whenStable(
newWhenStable.bind(testabilityDelegate, callback));
}
});
};
Expand All @@ -201,9 +202,14 @@ export class UpgradeModule {
angular.element(element).data(controllerKey(INJECTOR_KEY), this.injector);

// Wire up the ng1 rootScope to run a digest cycle whenever the zone settles
const $rootScope = $injector.get('$rootScope');
this.ngZone.onMicrotaskEmpty.subscribe(
() => this.ngZone.runOutsideAngular(() => $rootScope.$evalAsync()));
// We need to do this in the next tick so that we don't prevent the bootup
// stabilizing
setTimeout(() => {
const $rootScope = $injector.get('$rootScope');
const subscription =
this.ngZone.onMicrotaskEmpty.subscribe(() => $rootScope.$digest());
$rootScope.$on('$destroy', () => { subscription.unsubscribe(); });
}, 0);
}
]);

Expand Down
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Component, Directive, ElementRef, Injector, NgModule, destroyPlatform} from '@angular/core';
import {Component, Directive, ElementRef, Injector, Input, NgModule, NgZone, SimpleChange, SimpleChanges, destroyPlatform} from '@angular/core';
import {async} from '@angular/core/testing';
import {BrowserModule} from '@angular/platform-browser';
import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
Expand Down Expand Up @@ -76,5 +76,86 @@ export function main() {
expect(log).toEqual(['1A', '1B', '1C', '2A', '2B', '2C', 'ng1a', 'ng1b']);
});
}));

it('should propagate changes to a downgraded component inside the ngZone', async(() => {
let appComponent: AppComponent;

@Component({selector: 'my-app', template: '<my-child [value]="value"></my-child>'})
class AppComponent {
value: number;
constructor() { appComponent = this; }
}

@Component({
selector: 'my-child',
template: '<div>{{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'); });

Promise.resolve().then(() => this.valueFromPromise = changes['value'].currentValue);
}
}

@NgModule({
declarations: [AppComponent, ChildComponent],
entryComponents: [AppComponent],
imports: [BrowserModule, UpgradeModule]
})
class Ng2Module {
ngDoBootstrap() {}
}

const ng1Module = angular.module('ng1', []).directive(
'myApp', downgradeComponent({component: AppComponent}));


const element = html('<my-app></my-app>');

bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then((upgrade) => {
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(() => {

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Feb 1, 2017

Contributor

@petebacondarwin can you please uncomment this and disable the test via xit. if you comment out code like this, it won't be refactored or reformatted next time we do those kind of changes on the whole repo.

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Feb 1, 2017

Author Member

@IgorMinar If I xit it will it pass CI checks?

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Feb 1, 2017

Member

As far as I remember leaving out xit should break the build

// @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('<my-app></my-app>');

// 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;
// });
// }));
});
}

0 comments on commit 20b454c

Please sign in to comment.