fix(ivy): determine value of SimpleChange.firstChange per property (#24152)

PR Close #24152

packages/core/src/render3/definition.ts

@@ -253,32 +253,33 @@ export function NgOnChangesFeature(inputPropertyNames?: {[key: string]: string})
   return function(definition: DirectiveDef<any>): 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);
         }
       });
     }

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<string|SimpleChange> = [];
           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<MyDirective>).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<string|SimpleChange> = [];
+          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<MyDirective>).factory() as MyDirective;
+        myDir.valA = 'first';
+        myDir.valB = 'second';
+        (MyDirective.ngDirectiveDef as DirectiveDef<MyDirective>).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<MyDirective>).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<string|SimpleChange> = [];
+
+          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<MyDirective>).factory() as MyDirective;
+        myDir.onlySetter = 'someValue';
+        expect(myDir.onlySetter).toBeUndefined();
+        (MyDirective.ngDirectiveDef as DirectiveDef<MyDirective>).doCheck !.call(myDir);
+        const changeSetter = new SimpleChange(undefined, 'someValue', true);
+        expect(myDir.log).toEqual(['someValue', 'ngOnChanges', 'onlySetter', changeSetter]);
       });
     });
   });