fix: dispose of Lit parts after changing a renderer (#2325) (#2328)

Author: tomivirkki

packages/vaadin-combo-box/package.json

@@ -44,6 +44,7 @@
     "@vaadin/testing-helpers": "^0.2.1",
     "@vaadin/vaadin-dialog": "^21.0.0-beta2",
     "@vaadin/vaadin-template-renderer": "^21.0.0-beta2",
+    "lit": "^2.0.0-rc.1",
     "sinon": "^9.2.0"
   },
   "publishConfig": {

packages/vaadin-combo-box/src/vaadin-combo-box-item.js

@@ -145,6 +145,10 @@ class ComboBoxItemElement extends ThemableMixin(DirMixin(PolymerElement)) {
 
     if (this._oldRenderer !== renderer) {
       this.$.content.innerHTML = '';
+      // Whenever a Lit-based renderer is used, it assigns a Lit part to the node it was rendered into.
+      // When clearing the rendered content, this part needs to be manually disposed of.
+      // Otherwise, using a Lit-based renderer on the same node will throw an exception or render nothing afterward.
+      delete this.$.content._$litPart$;
     }
 
     if (renderer) {

packages/vaadin-combo-box/test/item-renderer.test.js

@@ -123,4 +123,10 @@ describe('item renderer', () => {
 
     expect(getFirstItem().$.content.textContent).to.equal('foo');
   });
+
+  it('should clear the old content after assigning a new renderer', () => {
+    comboBox.opened = true;
+    comboBox.renderer = () => {};
+    expect(getFirstItem().$.content.textContent).to.equal('');
+  });
 });

packages/vaadin-combo-box/test/lit.test.js

@@ -0,0 +1,41 @@
+import { expect } from '@esm-bundle/chai';
+import { fixtureSync } from '@vaadin/testing-helpers';
+import { html, render } from 'lit';
+import '../vaadin-combo-box.js';
+
+describe('lit', () => {
+  describe('renderer', () => {
+    let comboBox;
+
+    function getFirstItem() {
+      return comboBox.$.overlay._selector.querySelector('vaadin-combo-box-item');
+    }
+
+    beforeEach(() => {
+      comboBox = fixtureSync(`<vaadin-combo-box></vaadin-combo-box>`);
+
+      const size = 100;
+
+      comboBox.items = new Array(size).fill().map((e, i) => {
+        return { value: `value-${i}` };
+      });
+
+      comboBox.renderer = (root, _, { index }) => {
+        render(html`value-${index}`, root);
+      };
+    });
+
+    it('should render the content', () => {
+      comboBox.opened = true;
+      expect(getFirstItem().$.content.textContent).to.equal('value-0');
+    });
+
+    it('should render new content after assigning a new renderer', () => {
+      comboBox.opened = true;
+      comboBox.renderer = (root, _, { index }) => {
+        render(html`new-${index}`, root);
+      };
+      expect(getFirstItem().$.content.textContent).to.equal('new-0');
+    });
+  });
+});

packages/vaadin-virtual-list/package.json

@@ -34,6 +34,7 @@
   "devDependencies": {
     "@esm-bundle/chai": "^4.1.5",
     "@vaadin/testing-helpers": "^0.2.1",
+    "lit": "^2.0.0-rc.1",
     "sinon": "^9.2.4"
   },
   "publishConfig": {

packages/vaadin-virtual-list/src/vaadin-virtual-list.js

@@ -127,14 +127,36 @@ class VirtualListElement extends ElementMixin(ThemableMixin(PolymerElement)) {
 
   /** @private */
   __updateElement(el, index) {
+    if (el.__renderer !== this.renderer) {
+      el.__renderer = this.renderer;
+      this.__clearRenderTargetContent(el);
+    }
+
     if (this.renderer) {
       this.renderer(el, this, { item: this.items[index], index });
     }
   }
 
+  /**
+   * Clears the content of a render target.
+   * @private
+   */
+  __clearRenderTargetContent(element) {
+    element.innerHTML = '';
+    // Whenever a Lit-based renderer is used, it assigns a Lit part to the node it was rendered into.
+    // When clearing the rendered content, this part needs to be manually disposed of.
+    // Otherwise, using a Lit-based renderer on the same node will throw an exception or render nothing afterward.
+    delete element._$litPart$;
+  }
+
   /** @private */
   __itemsOrRendererChanged(items = [], renderer, virtualizer) {
-    if (renderer && virtualizer) {
+    // If the renderer is removed but there are elements created by
+    // a previous renderer, we need to request an update from the virtualizer
+    // to get the already existing elements properly cleared.
+    const hasRenderedItems = this.childElementCount > 0;
+
+    if ((renderer || hasRenderedItems) && virtualizer) {
       if (items.length === virtualizer.size) {
         virtualizer.update();
       } else {

packages/vaadin-virtual-list/test/lit.test.js

@@ -0,0 +1,36 @@
+import { expect } from '@esm-bundle/chai';
+import { fixtureSync } from '@vaadin/testing-helpers';
+import { html, render } from 'lit';
+import '../vaadin-virtual-list.js';
+
+describe('lit', () => {
+  describe('renderer', () => {
+    let list;
+
+    beforeEach(() => {
+      list = fixtureSync(`<vaadin-virtual-list></vaadin-virtual-list>`);
+
+      const size = 100;
+
+      list.items = new Array(size).fill().map((e, i) => {
+        return { value: `value-${i}` };
+      });
+
+      list.renderer = (root, _, { index }) => {
+        render(html`value-${index}`, root);
+      };
+    });
+
+    it('should render the content', () => {
+      expect(list.children[0].textContent.trim()).to.equal('value-0');
+    });
+
+    it('should render new content after assigning a new renderer', () => {
+      list.renderer = (root, _, { index }) => {
+        render(html`new-${index}`, root);
+      };
+
+      expect(list.children[0].textContent.trim()).to.equal('new-0');
+    });
+  });
+});

packages/vaadin-virtual-list/test/virtual-list.test.js

@@ -100,5 +100,15 @@ describe('virtual-list', () => {
       const itemRect = item.getBoundingClientRect();
       expect(list.getBoundingClientRect().bottom).to.be.within(itemRect.top, itemRect.bottom);
     });
+
+    it('should clear the old content after assigning a new renderer', () => {
+      list.renderer = () => {};
+      expect(list.children[0].textContent.trim()).to.equal('');
+    });
+
+    it('should clear the old content after removing the renderer', () => {
+      list.renderer = null;
+      expect(list.children[0].textContent.trim()).to.equal('');
+    });
   });
 });