diff --git a/packages/core/src/render3/definition.ts b/packages/core/src/render3/definition.ts index dbe6f53663..d17049868d 100644 --- a/packages/core/src/render3/definition.ts +++ b/packages/core/src/render3/definition.ts @@ -253,32 +253,33 @@ export function NgOnChangesFeature(inputPropertyNames?: {[key: string]: string}) return function(definition: DirectiveDef): void { const inputs = definition.inputs; const proto = definition.type.prototype; - // Place where we will store SimpleChanges if there is a change - Object.defineProperty(proto, PRIVATE_PREFIX, {value: undefined, writable: true}); for (let pubKey in inputs) { const minKey = inputs[pubKey]; const propertyName = inputPropertyNames && inputPropertyNames[minKey] || pubKey; const privateMinKey = PRIVATE_PREFIX + minKey; - // Create a place where the actual value will be stored and make it non-enumerable - Object.defineProperty(proto, privateMinKey, {value: undefined, writable: true}); - - const existingDesc = Object.getOwnPropertyDescriptor(proto, minKey); - + const originalProperty = Object.getOwnPropertyDescriptor(proto, minKey); + const getter = originalProperty && originalProperty.get; + const setter = originalProperty && originalProperty.set; // create a getter and setter for property Object.defineProperty(proto, minKey, { - get: function(this: OnChangesExpando) { - return (existingDesc && existingDesc.get) ? existingDesc.get.call(this) : - this[privateMinKey]; - }, + get: getter || + (setter ? undefined : function(this: OnChangesExpando) { return this[privateMinKey]; }), set: function(this: OnChangesExpando, value: any) { let simpleChanges = this[PRIVATE_PREFIX]; - let isFirstChange = simpleChanges === undefined; - if (simpleChanges == null) { - simpleChanges = this[PRIVATE_PREFIX] = {}; + if (!simpleChanges) { + // Place where we will store SimpleChanges if there is a change + Object.defineProperty( + this, PRIVATE_PREFIX, {value: simpleChanges = {}, writable: true}); } + const isFirstChange = !this.hasOwnProperty(privateMinKey); simpleChanges[propertyName] = new SimpleChange(this[privateMinKey], value, isFirstChange); - (existingDesc && existingDesc.set) ? existingDesc.set.call(this, value) : - this[privateMinKey] = value; + if (isFirstChange) { + // Create a place where the actual value will be stored and make it non-enumerable + Object.defineProperty(this, privateMinKey, {value, writable: true}); + } else { + this[privateMinKey] = value; + } + setter && setter.call(this, value); } }); } diff --git a/packages/core/test/render3/define_spec.ts b/packages/core/test/render3/define_spec.ts index 7e03c7460d..2253dba899 100644 --- a/packages/core/test/render3/define_spec.ts +++ b/packages/core/test/render3/define_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {DoCheck, OnChanges, SimpleChanges} from '../../src/core'; +import {DoCheck, OnChanges, SimpleChange, SimpleChanges} from '../../src/core'; import {DirectiveDef, NgOnChangesFeature, defineDirective} from '../../src/render3/index'; describe('define', () => { @@ -14,7 +14,7 @@ describe('define', () => { describe('NgOnChangesFeature', () => { it('should patch class', () => { class MyDirective implements OnChanges, DoCheck { - public log: string[] = []; + public log: Array = []; public valA: string = 'initValue'; public set valB(value: string) { this.log.push(value); } @@ -23,8 +23,8 @@ describe('define', () => { ngDoCheck(): void { this.log.push('ngDoCheck'); } ngOnChanges(changes: SimpleChanges): void { this.log.push('ngOnChanges'); - this.log.push('valA', changes['valA'].previousValue, changes['valA'].currentValue); - this.log.push('valB', changes['valB'].previousValue, changes['valB'].currentValue); + this.log.push('valA', changes['valA']); + this.log.push('valB', changes['valB']); } static ngDirectiveDef = defineDirective({ @@ -45,9 +45,74 @@ describe('define', () => { expect(myDir.valB).toEqual('works'); myDir.log.length = 0; (MyDirective.ngDirectiveDef as DirectiveDef).doCheck !.call(myDir); - expect(myDir.log).toEqual([ - 'ngOnChanges', 'valA', 'initValue', 'first', 'valB', undefined, 'second', 'ngDoCheck' - ]); + const changeA = new SimpleChange('initValue', 'first', false); + const changeB = new SimpleChange(undefined, 'second', true); + expect(myDir.log).toEqual(['ngOnChanges', 'valA', changeA, 'valB', changeB, 'ngDoCheck']); + }); + + it('correctly computes firstChange', () => { + class MyDirective implements OnChanges { + public log: Array = []; + public valA: string = 'initValue'; + public valB: string; + + ngOnChanges(changes: SimpleChanges): void { + this.log.push('valA', changes['valA']); + this.log.push('valB', changes['valB']); + } + + static ngDirectiveDef = defineDirective({ + type: MyDirective, + selectors: [['', 'myDir', '']], + factory: () => new MyDirective(), + features: [NgOnChangesFeature()], + inputs: {valA: 'valA', valB: 'valB'} + }); + } + + const myDir = + (MyDirective.ngDirectiveDef as DirectiveDef).factory() as MyDirective; + myDir.valA = 'first'; + myDir.valB = 'second'; + (MyDirective.ngDirectiveDef as DirectiveDef).doCheck !.call(myDir); + const changeA1 = new SimpleChange('initValue', 'first', false); + const changeB1 = new SimpleChange(undefined, 'second', true); + expect(myDir.log).toEqual(['valA', changeA1, 'valB', changeB1]); + + myDir.log.length = 0; + myDir.valA = 'third'; + (MyDirective.ngDirectiveDef as DirectiveDef).doCheck !.call(myDir); + const changeA2 = new SimpleChange('first', 'third', false); + expect(myDir.log).toEqual(['valA', changeA2, 'valB', undefined]); + }); + + it('should not create a getter when only a setter is originally defined', () => { + class MyDirective implements OnChanges { + public log: Array = []; + + public set onlySetter(value: string) { this.log.push(value); } + + ngOnChanges(changes: SimpleChanges): void { + this.log.push('ngOnChanges'); + this.log.push('onlySetter', changes['onlySetter']); + } + + static ngDirectiveDef = defineDirective({ + type: MyDirective, + selectors: [['', 'myDir', '']], + factory: () => new MyDirective(), + features: [NgOnChangesFeature()], + inputs: {onlySetter: 'onlySetter'} + }); + } + + const myDir = + (MyDirective.ngDirectiveDef as DirectiveDef).factory() as MyDirective; + myDir.onlySetter = 'someValue'; + expect(myDir.onlySetter).toBeUndefined(); + (MyDirective.ngDirectiveDef as DirectiveDef).doCheck !.call(myDir); + const changeSetter = new SimpleChange(undefined, 'someValue', true); + expect(myDir.log).toEqual(['someValue', 'ngOnChanges', 'onlySetter', changeSetter]); }); }); });