Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(upgrade): detect async downgrade component changes #14039

Closed
wants to merge 1 commit into from
Closed

fix(upgrade): detect async downgrade component changes #14039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 12 additions & 6 deletions modules/@angular/upgrade/src/aot/upgrade_module.ts
View file
Open in desktop
Expand Up @@ -168,12 +168,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 @@ -199,9 +200,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(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be possible that we fail to propagate a change that happen in a microtask which completes after the $digest invoked from angular.bootstrap, but before we subscribe to onMicrotaskEmpty.

For example, I would expect a test like the following to fail:

    it('should propagate changes made asynchronously during bootstrap', fakeAsync(() => {
         const ng1Component: angular.IComponent = {
           template: '{{ $ctrl.value }}',
           bindings: {value: '<'}
         };

         @Directive({selector: 'ng1'})
         class Ng1ComponentFacade extends UpgradeComponent {
           @Input() value: string;
           constructor(elementRef: ElementRef, injector: Injector) {
             super('ng1', elementRef, injector);
           }
         }

         @Component({selector: 'ng2', template: '<ng1 [value]="value"></ng1>'})
         class Ng2Component {
           value: string;
           constructor() {
             Promise.resolve().then(() => this.value = 'foo');
           }
         }

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

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


         const element = html('<ng2></ng2>');

         bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then(upgrade => {
           tick();
           expect(element.textContent.trim()).toBe('foo');
         });
       }));

UPDATE:
Actually, it turns out the test passes, because there is "stuff" that goes on during the bootstrap phase that cause onMicrotaskEmpty to emit. So, we'll be fine 😃

const $rootScope = $injector.get('$rootScope');
const subscription =
this.ngZone.onMicrotaskEmpty.subscribe(() => $rootScope.$digest());
$rootScope.$on('$destroy', () => { subscription.unsubscribe(); });
}, 0);
}
]);

Expand Down
83 changes: 82 additions & 1 deletion modules/@angular/upgrade/test/aot/integration/change_detection_spec.ts
View file
Open in desktop
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(() => {
// @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;
// });
// }));
});
}