[lit-html] Add missing tests to close out TODOs in code (#2074)

* Add tests for nothing/noChange in unsafe*

* Add event-firing test from element-position directive

* Add changeset

* Accept undefined || null in unsafeHTML/SVG. Fixes #2047

* Apply suggestions from code review

Co-authored-by: Steve Orvell <sorvell@google.com>

* Updates based on feedback.

* Update API docs

* Update changeset

* Add noChange tests

Co-authored-by: Steve Orvell <sorvell@google.com>

.changeset/clever-rockets-care.md

@@ -0,0 +1,6 @@
+---
+'lit-html': patch
+---
+
+(Cleanup) Added missing tests to close out TODOs in the code.
+Fixed `unsafeHTML` and `unsafeSVG` to render no content (empty string) for values `undefined`, `null`, and `nothing`.

packages/lit-html/src/directives/unsafe-html.ts

@@ -27,9 +27,8 @@ export class UnsafeHTMLDirective extends Directive {
     }
   }
 
-  render(value: string | typeof nothing | typeof noChange) {
-    // TODO: add tests for nothing and noChange
-    if (value === nothing) {
+  render(value: string | typeof nothing | typeof noChange | undefined | null) {
+    if (value === nothing || value == null) {
       this._templateResult = undefined;
       return (this._value = value);
     }
@@ -67,6 +66,9 @@ export class UnsafeHTMLDirective extends Directive {
 /**
  * Renders the result as HTML, rather than text.
  *
+ * The values `undefined`, `null`, and `nothing`, will all result in no content
+ * (empty string) being rendered.
+ *
  * Note, this is unsafe to use with any user-provided input that hasn't been
  * sanitized or escaped, as it may lead to cross-site-scripting
  * vulnerabilities.

packages/lit-html/src/directives/unsafe-svg.ts

@@ -17,6 +17,9 @@ class UnsafeSVGDirective extends UnsafeHTMLDirective {
 /**
  * Renders the result as SVG, rather than text.
  *
+ * The values `undefined`, `null`, and `nothing`, will all result in no content
+ * (empty string) being rendered.
+ *
  * Note, this is unsafe to use with any user-provided input that hasn't been
  * sanitized or escaped, as it may lead to cross-site-scripting
  * vulnerabilities.

packages/lit-html/src/test/directives/unsafe-html_test.ts

@@ -5,7 +5,7 @@
  */
 
 import {unsafeHTML} from '../../directives/unsafe-html.js';
-import {render, html} from '../../lit-html.js';
+import {render, html, nothing, noChange} from '../../lit-html.js';
 import {stripExpressionMarkers} from '../test-utils/strip-markers.js';
 import {assert} from '@esm-bundle/chai';
 
@@ -27,6 +27,45 @@ suite('unsafeHTML directive', () => {
     );
   });
 
+  test('rendering `nothing` renders empty string to content', () => {
+    render(html`<div>before${unsafeHTML(nothing)}after</div>`, container);
+    assert.equal(
+      stripExpressionMarkers(container.innerHTML),
+      '<div>beforeafter</div>'
+    );
+  });
+
+  test('rendering `noChange` does not change the previous content', () => {
+    const template = (v: string | typeof noChange) =>
+      html`<div>before${unsafeHTML(v)}after</div>`;
+    render(template('<p>Hi</p>'), container);
+    assert.equal(
+      stripExpressionMarkers(container.innerHTML),
+      '<div>before<p>Hi</p>after</div>'
+    );
+    render(template(noChange), container);
+    assert.equal(
+      stripExpressionMarkers(container.innerHTML),
+      '<div>before<p>Hi</p>after</div>'
+    );
+  });
+
+  test('rendering `undefined` renders empty string to content', () => {
+    render(html`<div>before${unsafeHTML(undefined)}after</div>`, container);
+    assert.equal(
+      stripExpressionMarkers(container.innerHTML),
+      '<div>beforeafter</div>'
+    );
+  });
+
+  test('rendering `null` renders empty string to content', () => {
+    render(html`<div>before${unsafeHTML(null)}after</div>`, container);
+    assert.equal(
+      stripExpressionMarkers(container.innerHTML),
+      '<div>beforeafter</div>'
+    );
+  });
+
   test('dirty checks primitive values', () => {
     const value = 'aaa';
     const t = () => html`<div>${unsafeHTML(value)}</div>`;

packages/lit-html/src/test/directives/unsafe-svg_test.ts

@@ -5,7 +5,7 @@
  */
 
 import {unsafeSVG} from '../../directives/unsafe-svg.js';
-import {render, html} from '../../lit-html.js';
+import {render, html, nothing, noChange} from '../../lit-html.js';
 import {stripExpressionMarkers} from '../test-utils/strip-markers.js';
 import {assert} from '@esm-bundle/chai';
 
@@ -33,6 +33,45 @@ suite('unsafeSVG', () => {
     assert.equal(lineElement.namespaceURI, 'http://www.w3.org/2000/svg');
   });
 
+  test('rendering `nothing` renders empty string to content', () => {
+    render(html`<svg>before${unsafeSVG(nothing)}after</svg>`, container);
+    assert.equal(
+      stripExpressionMarkers(container.innerHTML),
+      '<svg>beforeafter</svg>'
+    );
+  });
+
+  test('rendering `noChange` does not change the previous content', () => {
+    const template = (v: string | typeof noChange) =>
+      html`<svg>before${unsafeSVG(v)}after</svg>`;
+    render(template('<g>Hi</g>'), container);
+    assert.equal(
+      stripExpressionMarkers(container.innerHTML),
+      '<svg>before<g>Hi</g>after</svg>'
+    );
+    render(template(noChange), container);
+    assert.equal(
+      stripExpressionMarkers(container.innerHTML),
+      '<svg>before<g>Hi</g>after</svg>'
+    );
+  });
+
+  test('rendering `undefined` renders empty string to content', () => {
+    render(html`<svg>before${unsafeSVG(undefined)}after</svg>`, container);
+    assert.equal(
+      stripExpressionMarkers(container.innerHTML),
+      '<svg>beforeafter</svg>'
+    );
+  });
+
+  test('rendering `null` renders empty string to content', () => {
+    render(html`<svg>before${unsafeSVG(null)}after</svg>`, container);
+    assert.equal(
+      stripExpressionMarkers(container.innerHTML),
+      '<svg>beforeafter</svg>'
+    );
+  });
+
   test('dirty checks primitive values', () => {
     const value = 'aaa';
     const t = () => html`<svg>${unsafeSVG(value)}</svg>`;

packages/lit-html/src/test/lit-html_test.ts

@@ -47,6 +47,21 @@ const DEV_MODE = render.setSanitizer != null;
  */
 const INTERNAL = litHtmlLib.INTERNAL === true;
 
+class FireEventDirective extends Directive {
+  render() {
+    return nothing;
+  }
+  override update(part: AttributePart) {
+    part.element.dispatchEvent(
+      new CustomEvent('test-event', {
+        bubbles: true,
+      })
+    );
+    return nothing;
+  }
+}
+const fireEvent = directive(FireEventDirective);
+
 suite('lit-html', () => {
   let container: HTMLDivElement;
 
@@ -1849,21 +1864,6 @@ suite('lit-html', () => {
     });
 
     test('event listeners can see events fired in attribute directives', () => {
-      class FireEventDirective extends Directive {
-        render() {
-          return nothing;
-        }
-        // TODO (justinfagnani): make this work on SpreadPart
-        override update(part: AttributePart) {
-          part.element.dispatchEvent(
-            new CustomEvent('test-event', {
-              bubbles: true,
-            })
-          );
-          return nothing;
-        }
-      }
-      const fireEvent = directive(FireEventDirective);
       let event = undefined;
       const listener = (e: Event) => {
         event = e;
@@ -1875,6 +1875,18 @@ suite('lit-html', () => {
       assert.isOk(event);
     });
 
+    test('event listeners can see events fired in element directives', () => {
+      let event = undefined;
+      const listener = (e: Event) => {
+        event = e;
+      };
+      render(
+        html`<div @test-event=${listener} ${fireEvent()}></div>`,
+        container
+      );
+      assert.isOk(event);
+    });
+
     test('renders directives on ElementParts', () => {
       const log: string[] = [];
       assertRender(html`<div ${count('x', log)}></div>`, `<div></div>`);