lit · kevinpschaaf · Aug 19, 2021 · Aug 10, 2021 · Aug 10, 2021 · Aug 11, 2021
@@ -0,0 +1,5 @@
+---
+'lit-html': patch
+---
+
+Improves disconnection handling for first-party `AsyncDirective`s (`until`, `asyncAppend`, `asyncReplace`) so that the directive (and any DOM associated with it) can be garbage collected before any promises they are awaiting resolve.
@@ -4,97 +4,47 @@
  * SPDX-License-Identifier: BSD-3-Clause
  */
 
-import {ChildPart, noChange} from '../lit-html.js';
+import {ChildPart} from '../lit-html.js';
 import {
   directive,
   DirectiveParameters,
   PartInfo,
   PartType,
 } from '../directive.js';
-import {AsyncDirective} from '../async-directive.js';
+import {AsyncReplaceDirective} from './async-replace.js';
 import {
   clearPart,
   insertPart,
   setChildPartValue,
 } from '../directive-helpers.js';
 
-type Mapper<T> = (v: T, index?: number) => unknown;
-
-class AsyncAppendDirective extends AsyncDirective {
-  private _value?: AsyncIterable<unknown>;
-  private _reconnectResolver?: () => void;
-  private _reconnectPromise?: Promise<void>;
+class AsyncAppendDirective extends AsyncReplaceDirective {
+  private __childPart!: ChildPart;
 
+  // Override AsyncReplace to narrow the allowed part type to ChildPart only
   constructor(partInfo: PartInfo) {
     super(partInfo);
     if (partInfo.type !== PartType.CHILD) {
       throw new Error('asyncAppend can only be used in child expressions');
     }
   }
 
-  // @ts-expect-error value not used, but we want a nice parameter for docs
-  // eslint-disable-next-line @typescript-eslint/no-unused-vars
-  render<T>(value: AsyncIterable<T>, _mapper?: Mapper<T>) {
-    return noChange;
+  // Override AsyncReplace to save the part since we need to append into it
+  update(part: ChildPart, params: DirectiveParameters<this>) {
+    this.__childPart = part;
+    return super.update(part, params);
   }
 
-  update(part: ChildPart, [value, mapper]: DirectiveParameters<this>) {
-    // If we've already set up this particular iterable, we don't need
-    // to do anything.
-    if (value === this._value) {
-      return;
-    }
-    this._value = value;
-    this.__iterate(part, mapper);
-    return noChange;
-  }
-
-  // Separate function to avoid an iffe in update; update can't be async
-  // because its return value must be `noChange`
-  private async __iterate(part: ChildPart, mapper?: Mapper<unknown>) {
-    let i = 0;
-    const {_value: value} = this;
-    for await (let v of value!) {
-      // Check to make sure that value is the still the current value of
-      // the part, and if not bail because a new value owns this part
-      if (this._value !== value) {
-        break;
-      }
-
-      // If we were disconnected, pause until reconnected
-      if (this._reconnectPromise) {
-        await this._reconnectPromise;
-      }
-
-      // When we get the first value, clear the part. This lets the
-      // previous value display until we can replace it.
-      if (i === 0) {
-        clearPart(part);
-      }
-      // As a convenience, because functional-programming-style
-      // transforms of iterables and async iterables requires a library,
-      // we accept a mapper function. This is especially convenient for
-      // rendering a template for each item.
-      if (mapper !== undefined) {
-        v = mapper(v, i);
-      }
-      const newPart = insertPart(part);
-      setChildPartValue(newPart, v);
-      i++;
+  // Override AsyncReplace to append rather than replace
+  protected commitValue(value: unknown, index: number) {
+    // When we get the first value, clear the part. This lets the
+    // previous value display until we can replace it.
+    if (index === 0) {
+      clearPart(this.__childPart);
     }
-  }
-
-  disconnected() {
-    // Pause iteration while disconnected
-    this._reconnectPromise = new Promise(
-      (resolve) => (this._reconnectResolver = resolve)
-    );
-  }
-
-  reconnected() {
-    // Resume iteration when reconnected
-    this._reconnectPromise = undefined;
-    this._reconnectResolver!();
+    // Create and insert a new part and set its value to the next value
+    const newPart = insertPart(this.__childPart);
+    setChildPartValue(newPart, value);
   }
 }
 

@@ -7,13 +7,14 @@
 import {ChildPart, noChange} from '../lit-html.js';
 import {directive, DirectiveParameters} from '../directive.js';
 import {AsyncDirective} from '../async-directive.js';
+import {Pauser, PseudoWeakRef, forAwaitOf} from './private-async-helpers.js';
 
 type Mapper<T> = (v: T, index?: number) => unknown;
 
-class AsyncReplaceDirective extends AsyncDirective {
-  private _value?: AsyncIterable<unknown>;
-  private _reconnectResolver?: () => void;
-  private _reconnectPromise?: Promise<void>;
+export class AsyncReplaceDirective extends AsyncDirective {
+  private __value?: AsyncIterable<unknown>;
+  private __weakThis = new PseudoWeakRef(this);
+  private __pauser = new Pauser();
 
   // @ts-expect-error value not used, but we want a nice parameter for docs
   // eslint-disable-next-line @typescript-eslint/no-unused-vars
@@ -22,57 +23,64 @@ class AsyncReplaceDirective extends AsyncDirective {
   }
 
   update(_part: ChildPart, [value, mapper]: DirectiveParameters<this>) {
+    // If our initial render occurs while disconnected, ensure that the pauser
+    // and weakThis are in the disconnected state
+    if (!this.isConnected) {
+      this.disconnected();
+    }
     // If we've already set up this particular iterable, we don't need
     // to do anything.
-    if (value === this._value) {
+    if (value === this.__value) {
       return;
     }
-    this._value = value;
-    this.__iterate(mapper);
-    return noChange;
-  }
-
-  // Separate function to avoid an iffe in update; update can't be async
-  // because its return value must be `noChange`
-  private async __iterate(mapper?: Mapper<unknown>) {
+    this.__value = value;
     let i = 0;
-    const {_value: value} = this;
-    for await (let v of value!) {
-      // Check to make sure that value is the still the current value of
-      // the part, and if not bail because a new value owns this part
-      if (this._value !== value) {
-        break;
+    const {__weakThis: weakThis, __pauser: pauser} = this;
+    // Note, the callback avoids closing over `this` so that the directive
+    // can be gc'ed before the promise resolves
+    forAwaitOf(value, async (v: unknown) => {
+      // The while loop here handles the case that the connection state
+      // thrashes, causing the pauser to resume and then get re-paused
+      while (pauser.get()) {
+        await pauser.get();
       }
+      const _this = weakThis.deref();
+      if (_this !== undefined) {
+        // Check to make sure that value is the still the current value of
+        // the part, and if not bail because a new value owns this part
+        if (_this.__value !== value) {
+          return false;
+        }
 
-      // If we were disconnected, pause until reconnected
-      if (this._reconnectPromise) {
-        await this._reconnectPromise;
-      }
+        // As a convenience, because functional-programming-style
+        // transforms of iterables and async iterables requires a library,
+        // we accept a mapper function. This is especially convenient for
+        // rendering a template for each item.
+        if (mapper !== undefined) {
+          v = mapper(v, i);
+        }
 
-      // As a convenience, because functional-programming-style
-      // transforms of iterables and async iterables requires a library,
-      // we accept a mapper function. This is especially convenient for
-      // rendering a template for each item.
-      if (mapper !== undefined) {
-        v = mapper(v, i);
+        _this.commitValue(v, i);
+        i++;
       }
+      return true;
+    });
+    return noChange;
+  }
 
-      this.setValue(v);
-      i++;
-    }
+  // Override point for AsyncAppend to append rather than replace
+  protected commitValue(value: unknown, _index: number) {
+    this.setValue(value);
   }
 
   disconnected() {
-    // Pause iteration while disconnected
-    this._reconnectPromise = new Promise(
-      (resolve) => (this._reconnectResolver = resolve)
-    );
+    this.__weakThis.disconnect();
+    this.__pauser.pause();
   }
 
   reconnected() {
-    // Resume iteration when reconnected
-    this._reconnectPromise = undefined;
-    this._reconnectResolver!();
+    this.__weakThis.reconnect(this);
+    this.__pauser.resume();
   }
 }
 
@@ -95,9 +103,3 @@ class AsyncReplaceDirective extends AsyncDirective {
  *     value. Useful for generating templates for each item in the iterable.
  */
 export const asyncReplace = directive(AsyncReplaceDirective);
-
-/**
- * The type of the class that powers this directive. Necessary for naming the
- * directive's return type.
- */
-export type {AsyncReplaceDirective};
@@ -0,0 +1,88 @@
+/**
+ * @license
+ * Copyright 2021 Google LLC
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+// Note, this module is not included in package exports so that it's private to
+// our first-party directives. If it ends up being useful, we can open it up and
+// export it.
+
+/**
+ * Helper to iterate an AsyncIterable in its own closure.
+ * @param iterable The iterable to iterate
+ * @param callback The callback to call for each value. If the callback returns
+ * `false`, the loop will be broken.
+ */
+export const forAwaitOf = async <T>(
+  iterable: AsyncIterable<T>,
+  callback: (value: T) => Promise<boolean>
+) => {
+  for await (const v of iterable) {
+    if ((await callback(v)) === false) {
+      return;
+    }
+  }
+};
+
+/**
+ * Holds a reference to an instance that can be disconnected and reconnected,
+ * so that a closure over the ref (e.g. in a then function to a promise) does
+ * not strongly hold a ref to the instance. Approximates a WeakRef but must
+ * be manually connected & disconnected to the backing instance.
+ */
+export class PseudoWeakRef<T> {
+  private _ref?: T;
+  constructor(ref: T) {
+    this._ref = ref;
+  }
+  /**
+   * Disassociates the ref with the backing instance.
+   */
+  disconnect() {
+    this._ref = undefined;
+  }
+  /**
+   * Reassociates the ref with the backing instance.
+   */
+  reconnect(ref: T) {
+    this._ref = ref;
+  }
+  /**
+   * Retrieves the backing instance (will be undefined when disconnected)
+   */
+  deref() {
+    return this._ref;
+  }
+}
+
+/**
+ * A helper to pause and resume waiting on a condition in an async function
+ */
+export class Pauser {
+  private _promise?: Promise<void> = undefined;
+  private _resolve?: () => void = undefined;
+  /**
+   * When paused, returns a promise to be awaited; when unpaused, returns
+   * undefined. Note that in the microtask between the pauser being resumed
+   * an an await of this promise resolving, the pauser could be paused again,
+   * hence callers should check the promise in a loop when awaiting.
+   * @returns A promise to be awaited when paused or undefined
+   */
+  get() {
+    return this._promise;
+  }
+  /**
+   * Creates a promise to be awaited
+   */
+  pause() {
+    this._promise ??= new Promise((resolve) => (this._resolve = resolve));
+  }
+  /**
+   * Resolves the promise which may be awaited
+   */
+  resume() {
+    this._resolve?.();
+    this._promise = this._resolve = undefined;
+  }
+}