[labs/virtualizer] Fix #3481 and #3518 (#3519)

* add test for #3518

* add fix for #3518

* add test for #3481

* add fix for #3481

.changeset/five-pandas-report.md

@@ -0,0 +1,5 @@
+---
+'@lit-labs/virtualizer': patch
+---
+
+Fix [#3481: Error when immediately re-rendering](https://github.com/lit/lit/issues/3481)

.changeset/neat-bananas-cough.md

@@ -0,0 +1,5 @@
+---
+'@lit-labs/virtualizer': patch
+---
+
+Fix [#3518: New layoutComplete promise created instead of using existing one](https://github.com/lit/lit/issues/3518)

packages/labs/virtualizer/src/Virtualizer.ts

@@ -198,6 +198,14 @@ export class Virtualizer {
 
   protected _measureChildOverride: MeasureChildFunction | null = null;
 
+  /**
+   * State for `layoutComplete` promise
+   */
+  private _layoutCompletePromise: Promise<void> | null = null;
+  private _layoutCompleteResolver: Function | null = null;
+  private _layoutCompleteRejecter: Function | null = null;
+  private _pendingLayoutComplete: number | null = null;
+
   constructor(config: VirtualizerConfig) {
     if (!config) {
       throw new Error(
@@ -295,10 +303,10 @@ export class Virtualizer {
     );
     this._scrollEventListeners = [];
     this._clippingAncestors = [];
-    this._scrollerController = this._scrollerController!.detach(this);
-    this._mutationObserver!.disconnect();
-    this._hostElementRO!.disconnect();
-    this._childrenRO!.disconnect();
+    this._scrollerController = this._scrollerController?.detach(this) || null;
+    this._mutationObserver?.disconnect();
+    this._hostElementRO?.disconnect();
+    this._childrenRO?.disconnect();
     this._rejectLayoutCompletePromise('disconnected');
   }
 
@@ -768,42 +776,44 @@ export class Virtualizer {
     );
   }
 
-  private _layoutCompleteResolver: Function | null = null;
-  private _layoutCompleteRejecter: Function | null = null;
-  private _pendingLayoutComplete: number | null = null;
   public get layoutComplete(): Promise<void> {
-    return new Promise((resolve, reject) => {
-      this._layoutCompleteResolver = resolve;
-      this._layoutCompleteRejecter = reject;
-    });
+    // Lazily create promise
+    if (!this._layoutCompletePromise) {
+      this._layoutCompletePromise = new Promise((resolve, reject) => {
+        this._layoutCompleteResolver = resolve;
+        this._layoutCompleteRejecter = reject;
+      });
+    }
+    return this._layoutCompletePromise!;
   }
+
   private _rejectLayoutCompletePromise(reason: string) {
     if (this._layoutCompleteRejecter !== null) {
       this._layoutCompleteRejecter!(reason);
     }
     this._resetLayoutCompleteState();
   }
+
   private _scheduleLayoutComplete() {
-    if (this._pendingLayoutComplete !== null) {
-      cancelAnimationFrame(this._pendingLayoutComplete);
+    // Don't do anything unless we have a pending promise
+    // And only request a frame if we haven't already done so
+    if (this._layoutCompletePromise && this._pendingLayoutComplete === null) {
+      // Wait one additional frame to be sure the layout is stable
+      this._pendingLayoutComplete = requestAnimationFrame(() =>
+        requestAnimationFrame(() => this._resolveLayoutCompletePromise())
+      );
     }
-    // Seems to require waiting one additional frame to
-    // be sure the layout is stable
-    this._pendingLayoutComplete = requestAnimationFrame(() =>
-      requestAnimationFrame(() => this._layoutComplete())
-    );
-  }
-  private _layoutComplete() {
-    this._resolveLayoutCompletePromise();
-    this._pendingLayoutComplete = null;
   }
+
   private _resolveLayoutCompletePromise() {
     if (this._layoutCompleteResolver !== null) {
       this._layoutCompleteResolver();
     }
     this._resetLayoutCompleteState();
   }
+
   private _resetLayoutCompleteState() {
+    this._layoutCompletePromise = null;
     this._layoutCompleteResolver = null;
     this._layoutCompleteRejecter = null;
     this._pendingLayoutComplete = null;

packages/labs/virtualizer/src/test/scenarios/immediate-rerender.test.ts

@@ -0,0 +1,72 @@
+/**
+ * @license
+ * Copyright 2022 Google LLC
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+import {ignoreBenignErrors} from '../helpers.js';
+import {html, LitElement} from 'lit';
+import {customElement} from 'lit/decorators.js';
+import {LitVirtualizer} from '../../lit-virtualizer.js';
+import '../../lit-virtualizer.js';
+import {expect, html as testingHtml, fixture} from '@open-wc/testing';
+
+interface ExampleItem {
+  text: string;
+}
+
+@customElement('my-example')
+export class MyExample extends LitElement {
+  items: ExampleItem[] = new Array(100)
+    .fill('')
+    .map((_, i) => ({text: `Item ${i}`}));
+
+  render() {
+    return html`
+      <slot @slotchange=${this.#onSlotChange} style="display: none;"></slot>
+      <lit-virtualizer
+        .items=${this.items}
+        .renderItem=${(item: ExampleItem) => html`<p>${item.text}</p>`}
+      ></lit-virtualizer>
+    `;
+  }
+
+  #onSlotChange() {
+    this.requestUpdate();
+  }
+}
+
+// This is one minimal repro case for https://github.com/lit/lit/issues/3481.
+// This issue occurs when something triggers an immediate re-render of virtualizer
+// and an attempt to tear down some internal state before the async initialization
+// code for that state had run. In this repro, a slotchange event triggers
+// the re-render, but other minimal repro cases are probably possible. The
+// fix we've identified is some straightforward guard code to ensure that we
+// don't try to tear down state that hasn't yet been set up. It's possible
+// we'll end up wanting to short-circuit immediate re-renders earlier (before
+// they reach this guard code), but for now this fix seems sufficient.
+describe("Don't fail when rerendering before initialization is complete", () => {
+  ignoreBenignErrors(beforeEach, afterEach);
+
+  let errorOccurred = false;
+
+  function recordError(err: PromiseRejectionEvent) {
+    if (err.reason.stack.indexOf('Virtualizer') > -1) {
+      errorOccurred = true;
+    }
+  }
+
+  addEventListener('unhandledrejection', recordError);
+
+  it('should render without throwing an error', async () => {
+    const example = await fixture(
+      testingHtml`<my-example>Slot me!</my-example>`
+    );
+    await customElements.whenDefined('lit-virtualizer');
+    const virtualizer = example.shadowRoot!.querySelector('lit-virtualizer')!;
+    expect(virtualizer).to.be.instanceOf(LitVirtualizer);
+    await virtualizer.layoutComplete;
+    removeEventListener('unhandledrejection', recordError);
+    expect(errorOccurred).to.equal(false);
+  });
+});

packages/labs/virtualizer/src/test/scenarios/layout-complete.test.ts

@@ -0,0 +1,69 @@
+/**
+ * @license
+ * Copyright 2022 Google LLC
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+import {ignoreBenignErrors} from '../helpers.js';
+import {html, LitElement} from 'lit';
+import {customElement} from 'lit/decorators.js';
+import {LitVirtualizer} from '../../lit-virtualizer.js';
+import '../../lit-virtualizer.js';
+import {expect, html as testingHtml, fixture} from '@open-wc/testing';
+
+interface ExampleItem {
+  text: string;
+}
+
+@customElement('my-example')
+export class MyExample extends LitElement {
+  items: ExampleItem[] = new Array(100)
+    .fill('')
+    .map((_, i) => ({text: `Item ${i}`}));
+
+  render() {
+    return html`
+      <lit-virtualizer
+        .items=${this.items}
+        .renderItem=${(item: ExampleItem) => html`<p>${item.text}</p>`}
+      ></lit-virtualizer>
+    `;
+  }
+}
+
+describe('Basic layoutComplete functionality works', () => {
+  ignoreBenignErrors(beforeEach, afterEach);
+
+  it('resolves as soon as virtualizer has rendered, not before', async () => {
+    const example = await fixture(testingHtml`<my-example></my-example>`);
+    await customElements.whenDefined('lit-virtualizer');
+    const virtualizer = example.shadowRoot!.querySelector('lit-virtualizer')!;
+    expect(virtualizer).to.be.instanceOf(LitVirtualizer);
+    // Before layoutComplete, we shouldn't have rendered
+    expect(virtualizer.children.length).to.equal(0);
+    await virtualizer.layoutComplete;
+    // After layoutComplete, we should have rendered children
+    expect(virtualizer.children.length).to.be.greaterThan(0);
+    // And our children should have been explicitly positioned.
+    // This test depends on an implementation detail; will need to be
+    // updated if we ever change the way we position children in the DOM.
+    expect(
+      (virtualizer.children[0] as HTMLElement).style.transform.indexOf(
+        'translate'
+      )
+    ).to.be.greaterThan(-1);
+  });
+
+  it('returns the same promise when layoutComplete is accessed repeatedly during a single cycle', async () => {
+    const example = await fixture(testingHtml`<my-example></my-example>`);
+    await customElements.whenDefined('lit-virtualizer');
+    const virtualizer = example.shadowRoot!.querySelector('lit-virtualizer')!;
+    const lc1 = virtualizer.layoutComplete;
+    const lc2 = virtualizer.layoutComplete;
+    await Promise.resolve();
+    const lc3 = virtualizer.layoutComplete;
+    await new Promise((resolve) => setTimeout(resolve, 0));
+    const lc4 = virtualizer.layoutComplete;
+    expect(lc1 === lc2 && lc2 === lc3 && lc3 === lc4).to.equal(true);
+  });
+});