From b3e63c09aba9822d9f3ab129c727814cd96386c9 Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Wed, 3 May 2017 18:18:40 +0300 Subject: [PATCH] fix(upgrade): initialize all inputs in time for `ngOnChanges()` Previously, non-bracketed inputs (e.g. `xyz="foo"`) on downgraded components were initialized using `attrs.$observe()` (which uses `$evalAsync()` under the hood), while bracketed inputs (e.g. `[xyz]="'foo'"`) were initialized using `$watch()`. If the downgraded component was created during a `$digest` (e.g. by an `ng-if` watcher), the non-bracketed inputs were not initialized in time for the initial call to `ngOnChanges()` and `ngOnInit()`. This commit fixes it by using `$watch()` to initialize all inputs. `$observe()` is still used for subsequent updates on non-bracketed inputs, because it is more performant. Fixes #16212 --- .../src/common/downgrade_component_adapter.ts | 22 +++++-- packages/upgrade/src/common/util.ts | 7 +++ .../src/dynamic/upgrade_ng1_adapter.ts | 5 +- packages/upgrade/test/dynamic/upgrade_spec.ts | 59 +++++++++++++++++- .../integration/downgrade_component_spec.ts | 60 ++++++++++++++++++- 5 files changed, 143 insertions(+), 10 deletions(-) diff --git a/packages/upgrade/src/common/downgrade_component_adapter.ts b/packages/upgrade/src/common/downgrade_component_adapter.ts index e949bc6506..4e6aa21d29 100644 --- a/packages/upgrade/src/common/downgrade_component_adapter.ts +++ b/packages/upgrade/src/common/downgrade_component_adapter.ts @@ -11,7 +11,7 @@ import {ChangeDetectorRef, ComponentFactory, ComponentRef, EventEmitter, Injecto import * as angular from './angular1'; import {PropertyBinding} from './component_info'; import {$SCOPE} from './constants'; -import {getAttributesAsArray, getComponentName, hookupNgModel} from './util'; +import {getAttributesAsArray, getComponentName, hookupNgModel, strictEquals} from './util'; const INITIAL_VALUE = { __UNINITIALIZED__: true @@ -75,16 +75,28 @@ export class DowngradeComponentAdapter { const observeFn = (prop => { let prevValue = INITIAL_VALUE; return (currValue: any) => { - if (prevValue === INITIAL_VALUE) { + // Initially, both `$observe()` and `$watch()` will call this function. + if (!strictEquals(prevValue, currValue)) { + if (prevValue === INITIAL_VALUE) { + prevValue = currValue; + } + + this.updateInput(prop, prevValue, currValue); prevValue = currValue; } - - this.updateInput(prop, prevValue, currValue); - prevValue = currValue; }; })(input.prop); attrs.$observe(input.attr, observeFn); + // 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: any = this.componentScope.$watch(() => { + unwatch(); + unwatch = null; + observeFn((attrs as any)[input.attr]); + }); + } else if (attrs.hasOwnProperty(input.bindAttr)) { expr = (attrs as any /** TODO #9100 */)[input.bindAttr]; } else if (attrs.hasOwnProperty(input.bracketAttr)) { diff --git a/packages/upgrade/src/common/util.ts b/packages/upgrade/src/common/util.ts index 7649605cfb..cc1d1a086c 100644 --- a/packages/upgrade/src/common/util.ts +++ b/packages/upgrade/src/common/util.ts @@ -75,3 +75,10 @@ export function hookupNgModel(ngModel: angular.INgModelController, component: an component.registerOnChange(ngModel.$setViewValue.bind(ngModel)); } } + +/** + * Test two values for strict equality, accounting for the fact that `NaN !== NaN`. + */ +export function strictEquals(val1: any, val2: any): boolean { + return val1 === val2 || (val1 !== val1 && val2 !== val2); +} diff --git a/packages/upgrade/src/dynamic/upgrade_ng1_adapter.ts b/packages/upgrade/src/dynamic/upgrade_ng1_adapter.ts index e7bdf21b63..38980799d4 100644 --- a/packages/upgrade/src/dynamic/upgrade_ng1_adapter.ts +++ b/packages/upgrade/src/dynamic/upgrade_ng1_adapter.ts @@ -10,7 +10,7 @@ import {Directive, DoCheck, ElementRef, EventEmitter, Inject, OnChanges, OnInit, import * as angular from '../common/angular1'; import {$COMPILE, $CONTROLLER, $HTTP_BACKEND, $SCOPE, $TEMPLATE_CACHE} from '../common/constants'; -import {controllerKey} from '../common/util'; +import {controllerKey, strictEquals} from '../common/util'; interface IBindingDestination { @@ -309,8 +309,7 @@ class UpgradeNg1ComponentAdapter implements OnInit, OnChanges, DoCheck { checkProperties.forEach((propName, i) => { const value = destinationObj ![propName]; const last = lastValues[i]; - if (value !== last && - (value === value || last === last)) { // ignore NaN values (NaN !== NaN) + if (!strictEquals(last, value)) { const eventEmitter: EventEmitter = (this as any)[propOuts[i]]; eventEmitter.emit(lastValues[i] = value); } diff --git a/packages/upgrade/test/dynamic/upgrade_spec.ts b/packages/upgrade/test/dynamic/upgrade_spec.ts index 29a2ed0d8c..c9cc675375 100644 --- a/packages/upgrade/test/dynamic/upgrade_spec.ts +++ b/packages/upgrade/test/dynamic/upgrade_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {ChangeDetectorRef, Class, Component, EventEmitter, Input, NO_ERRORS_SCHEMA, NgModule, NgZone, SimpleChange, SimpleChanges, Testability, destroyPlatform, forwardRef} from '@angular/core'; +import {ChangeDetectorRef, Class, Component, EventEmitter, Input, NO_ERRORS_SCHEMA, NgModule, NgZone, OnChanges, 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'; @@ -410,6 +410,63 @@ export function main() { })); + it('should initialize inputs in time for `ngOnChanges`', async(() => { + const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module)); + + @Component({ + selector: 'ng2', + template: ` + ngOnChangesCount: {{ ngOnChangesCount }} | + firstChangesCount: {{ firstChangesCount }} | + initialValue: {{ initialValue }}` + }) + class Ng2Component implements OnChanges { + ngOnChangesCount = 0; + firstChangesCount = 0; + initialValue: string; + @Input() foo: string; + + ngOnChanges(changes: SimpleChanges) { + this.ngOnChangesCount++; + + if (this.ngOnChangesCount === 1) { + this.initialValue = this.foo; + } + + if (changes['foo'] && changes['foo'].isFirstChange()) { + this.firstChangesCount++; + } + } + } + + @NgModule({imports: [BrowserModule], declarations: [Ng2Component]}) + class Ng2Module { + } + + const ng1Module = angular.module('ng1', []).directive( + 'ng2', adapter.downgradeNg2Component(Ng2Component)); + + const element = html(` + + + + + `); + + adapter.bootstrap(element, ['ng1']).ready(ref => { + const nodes = element.querySelectorAll('ng2'); + const expectedTextWith = (value: string) => + `ngOnChangesCount: 1 | firstChangesCount: 1 | initialValue: ${value}`; + + expect(multiTrim(nodes[0].textContent)).toBe(expectedTextWith('foo')); + expect(multiTrim(nodes[1].textContent)).toBe(expectedTextWith('bar')); + expect(multiTrim(nodes[2].textContent)).toBe(expectedTextWith('baz')); + expect(multiTrim(nodes[3].textContent)).toBe(expectedTextWith('qux')); + + ref.dispose(); + }); + })); + it('should bind to ng-model', async(() => { const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module)); const ng1Module = angular.module('ng1', []); diff --git a/packages/upgrade/test/static/integration/downgrade_component_spec.ts b/packages/upgrade/test/static/integration/downgrade_component_spec.ts index 4ea452e332..3a9a9c7872 100644 --- a/packages/upgrade/test/static/integration/downgrade_component_spec.ts +++ b/packages/upgrade/test/static/integration/downgrade_component_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Compiler, Component, ComponentFactoryResolver, EventEmitter, Injector, NgModule, NgModuleRef, OnChanges, OnDestroy, SimpleChanges, destroyPlatform} from '@angular/core'; +import {Compiler, Component, ComponentFactoryResolver, EventEmitter, Injector, Input, NgModule, NgModuleRef, OnChanges, OnDestroy, SimpleChanges, destroyPlatform} from '@angular/core'; import {async} from '@angular/core/testing'; import {BrowserModule} from '@angular/platform-browser'; import {platformBrowserDynamic} from '@angular/platform-browser-dynamic'; @@ -148,6 +148,64 @@ export function main() { }); })); + it('should initialize inputs in time for `ngOnChanges`', async(() => { + @Component({ + selector: 'ng2', + template: ` + ngOnChangesCount: {{ ngOnChangesCount }} | + firstChangesCount: {{ firstChangesCount }} | + initialValue: {{ initialValue }}` + }) + class Ng2Component implements OnChanges { + ngOnChangesCount = 0; + firstChangesCount = 0; + initialValue: string; + @Input() foo: string; + + ngOnChanges(changes: SimpleChanges) { + this.ngOnChangesCount++; + + if (this.ngOnChangesCount === 1) { + this.initialValue = this.foo; + } + + if (changes['foo'] && changes['foo'].isFirstChange()) { + this.firstChangesCount++; + } + } + } + + @NgModule({ + imports: [BrowserModule, UpgradeModule], + declarations: [Ng2Component], + entryComponents: [Ng2Component] + }) + class Ng2Module { + ngDoBootstrap() {} + } + + const ng1Module = angular.module('ng1', []).directive( + 'ng2', downgradeComponent({component: Ng2Component})); + + const element = html(` + + + + + `); + + bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then(upgrade => { + const nodes = element.querySelectorAll('ng2'); + const expectedTextWith = (value: string) => + `ngOnChangesCount: 1 | firstChangesCount: 1 | initialValue: ${value}`; + + expect(multiTrim(nodes[0].textContent)).toBe(expectedTextWith('foo')); + expect(multiTrim(nodes[1].textContent)).toBe(expectedTextWith('bar')); + expect(multiTrim(nodes[2].textContent)).toBe(expectedTextWith('baz')); + expect(multiTrim(nodes[3].textContent)).toBe(expectedTextWith('qux')); + }); + })); + it('should bind to ng-model', async(() => { const ng1Module = angular.module('ng1', []).run( ($rootScope: angular.IScope) => { $rootScope['modelA'] = 'A'; });