From f2709ac3f9abb547e243a3b9fb13296a179ec77e Mon Sep 17 00:00:00 2001
From: Kristiyan Kostadinov <crisbeto@abv.bg>
Date: Thu, 25 Apr 2019 07:12:37 +0200
Subject: [PATCH] fix(ivy): correctly reflect undefined values (#30103)

Fixes Ivy reflecting properties with `undefined` values, rather than omitting them. Also fixes that Ivy doesn't clear the reflected values when property value changes from something that is defined to undefined.

This PR resolves FW-1277.

PR Close #30103
---
 .../core/src/render3/instructions/shared.ts   | 21 +++--
 packages/core/test/linker/integration_spec.ts | 93 +++++++++++++++++++
 2 files changed, 106 insertions(+), 8 deletions(-)

diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts
index 23fde60df7..b828caa892 100644
--- a/packages/core/src/render3/instructions/shared.ts
+++ b/packages/core/src/render3/instructions/shared.ts
@@ -905,15 +905,20 @@ export function setNgReflectProperty(
   attrName = normalizeDebugBindingName(attrName);
   const debugValue = normalizeDebugBindingValue(value);
   if (type === TNodeType.Element) {
-    isProceduralRenderer(renderer) ?
-        renderer.setAttribute((element as RElement), attrName, debugValue) :
-        (element as RElement).setAttribute(attrName, debugValue);
-  } else if (value !== undefined) {
-    const value = `bindings=${JSON.stringify({[attrName]: debugValue}, null, 2)}`;
-    if (isProceduralRenderer(renderer)) {
-      renderer.setValue((element as RComment), value);
+    if (value == null) {
+      isProceduralRenderer(renderer) ? renderer.removeAttribute((element as RElement), attrName) :
+                                       (element as RElement).removeAttribute(attrName);
     } else {
-      (element as RComment).textContent = value;
+      isProceduralRenderer(renderer) ?
+          renderer.setAttribute((element as RElement), attrName, debugValue) :
+          (element as RElement).setAttribute(attrName, debugValue);
+    }
+  } else {
+    const textContent = `bindings=${JSON.stringify({[attrName]: debugValue}, null, 2)}`;
+    if (isProceduralRenderer(renderer)) {
+      renderer.setValue((element as RComment), textContent);
+    } else {
+      (element as RComment).textContent = textContent;
     }
   }
 }
diff --git a/packages/core/test/linker/integration_spec.ts b/packages/core/test/linker/integration_spec.ts
index f689a89212..2687a54e6a 100644
--- a/packages/core/test/linker/integration_spec.ts
+++ b/packages/core/test/linker/integration_spec.ts
@@ -1772,6 +1772,99 @@ function declareTests(config?: {useJit: boolean}) {
         fixture.detectChanges();
         expect(getDOM().getInnerHTML(fixture.nativeElement)).toContain('[ERROR]');
       });
+
+      it('should not reflect undefined values', () => {
+        TestBed.configureTestingModule({declarations: [MyComp, MyDir, MyDir2]});
+        TestBed.overrideComponent(
+            MyComp, {set: {template: `<div my-dir [elprop]="ctxProp"></div>`}});
+        const fixture = TestBed.createComponent(MyComp);
+
+        fixture.componentInstance.ctxProp = 'hello';
+        fixture.detectChanges();
+
+        expect(getDOM().getInnerHTML(fixture.nativeElement))
+            .toContain('ng-reflect-dir-prop="hello"');
+
+        fixture.componentInstance.ctxProp = undefined !;
+        fixture.detectChanges();
+
+        expect(getDOM().getInnerHTML(fixture.nativeElement)).not.toContain('ng-reflect-');
+      });
+
+      it('should not reflect null values', () => {
+        TestBed.configureTestingModule({declarations: [MyComp, MyDir, MyDir2]});
+        TestBed.overrideComponent(
+            MyComp, {set: {template: `<div my-dir [elprop]="ctxProp"></div>`}});
+        const fixture = TestBed.createComponent(MyComp);
+
+        fixture.componentInstance.ctxProp = 'hello';
+        fixture.detectChanges();
+
+        expect(getDOM().getInnerHTML(fixture.nativeElement))
+            .toContain('ng-reflect-dir-prop="hello"');
+
+        fixture.componentInstance.ctxProp = null !;
+        fixture.detectChanges();
+
+        expect(getDOM().getInnerHTML(fixture.nativeElement)).not.toContain('ng-reflect-');
+      });
+
+      it('should reflect empty strings', () => {
+        TestBed.configureTestingModule({declarations: [MyComp, MyDir, MyDir2]});
+        TestBed.overrideComponent(
+            MyComp, {set: {template: `<div my-dir [elprop]="ctxProp"></div>`}});
+        const fixture = TestBed.createComponent(MyComp);
+
+        fixture.componentInstance.ctxProp = '';
+        fixture.detectChanges();
+
+        expect(getDOM().getInnerHTML(fixture.nativeElement)).toContain('ng-reflect-dir-prop=""');
+      });
+
+      it('should not reflect in comment nodes when the value changes to undefined', () => {
+        const fixture =
+            TestBed.configureTestingModule({declarations: [MyComp]})
+                .overrideComponent(
+                    MyComp, {set: {template: `<ng-template [ngIf]="ctxBoolProp"></ng-template>`}})
+                .createComponent(MyComp);
+
+        fixture.componentInstance.ctxBoolProp = true;
+        fixture.detectChanges();
+
+        let html = getDOM().getInnerHTML(fixture.nativeElement);
+        expect(html).toContain('bindings={');
+        expect(html).toContain('"ng-reflect-ng-if": "true"');
+
+        fixture.componentInstance.ctxBoolProp = undefined !;
+        fixture.detectChanges();
+
+        html = getDOM().getInnerHTML(fixture.nativeElement);
+        expect(html).toContain('bindings={');
+        expect(html).not.toContain('ng-reflect');
+      });
+
+      it('should reflect in comment nodes when the value changes to null', () => {
+        const fixture =
+            TestBed.configureTestingModule({declarations: [MyComp]})
+                .overrideComponent(
+                    MyComp, {set: {template: `<ng-template [ngIf]="ctxBoolProp"></ng-template>`}})
+                .createComponent(MyComp);
+
+        fixture.componentInstance.ctxBoolProp = true;
+        fixture.detectChanges();
+
+        let html = getDOM().getInnerHTML(fixture.nativeElement);
+        expect(html).toContain('bindings={');
+        expect(html).toContain('"ng-reflect-ng-if": "true"');
+
+        fixture.componentInstance.ctxBoolProp = null !;
+        fixture.detectChanges();
+
+        html = getDOM().getInnerHTML(fixture.nativeElement);
+        expect(html).toContain('bindings={');
+        expect(html).toContain('"ng-reflect-ng-if": null');
+      });
+
     });
 
     describe('property decorators', () => {