The comments here are confusing since this is AsyncAppend.

I made AsyncAppend subclass AsyncReplace since it's mostly identical code. As you can see in AsyncAppend, the only differences are:

• validatePartType: AsyncReplace can be used anywhere but AsyncAppend can only go in ChildPart position
• update: AsyncAppend needs to save the __childPart since it inserts new parts into it
• commitValue: AsyncAppend inserts new parts for each value, whereas AsyncReplace just calls setValue and overwrites the previous

Oic, I had the comments backward. Fixed.

Why is there no check here?

Ah, I had originally done AsyncReplace extends AsyncAppend which needed to loosen the part validation, but flipping it to AsyncAppend extends AsyncReplace lets AsyncAppend just do the stricter part validation in its constructor.

Removed.

Remove?

Removed

Done

Done

Done

Done

What if the same iterable and thus promise is used in multiple directives? Does this need to be (can it be?) on the directive instance?

Man, that's a good point (more for until, since I think that iterables are more likely to return fresh promises for each next() call). I can see if the WeakMaps can live on the directive instance if we pull it off first and only close over the WeakMap. Lemme try that.

To address this, went ahead and refactored it into a DisconnectableAwaiter that wraps up the logic around indirecting the this ref to the directive, storing a value that resolves while disconnected, and flushing it back to the thenFn if resolved while disconnected.

Added tests for same promise/iterable passed to multiple directive instances as well.

Here directive is always the same as this isn't it? Does this mean we don't need to actually store the directive value and only need to know if this promise is mapped?

The whole thing is not closing over this in the thenFn. Going through the WeakMap with the promise as the key is what breaks the dependency.

This is a lot of boilerplate that each of the directives are having to implement. Ideally, we'd just be using WeakRefs to weakly hold onto the directive this, and everything would be handled automatically. But not every browser will support that.

The good news is that directives support disconnected and reconnected, so we can implement a pseudo WeakRef with manual tracking very easily (the only difficult thing about implementing a WeakRef is making it automatic).

const backing = new WeakMap<WeakRefWrapper, unknown>();
class WeakRefWrapper {
  _weakref = null;
  
  constructor(obj: unknown) {
    if (typeof WeakRef === 'function') {
      this._weakref = new WeakRef(obj);
    } else {
      backing.set(this, obj);
    }
  }
  
  deref() {
    if (this._weakref) return this._weakref.deref();
    return backing.get(this);
  }
  
  // reconnected would call `weakthis.aquire(this)` to reestablish the weakref.
  acquire(obj) {
    if (!this._weakref) {
      // This should really be the same object as original, but we can't
      // verify that without strongly holding onto the original...
      backing.set(this, obj);
    }
  }
  
  // disconnected would call `weakthis.release()` to break the weakref.
  release() {
    if (!this._weakref) {
      backing.delete(this);
    }
  }
}

Then, we don't need this manual promise->directive tracking anymore. Most of the code can remain unchanged. The only difference is that we now refer to a weakthis instead of this. Combined with the asyncForEach helper, that means the majority of the code can stay the same while still preventing the memory leak.

I have a really basic example in https://output.jsbin.com/qumamir/quiet

This is never called with the argument so let's remove it.

Done

This is pretty tricky. It'd be really nice to have a DisconnectablePromise directive-helper to make this more generally available.

With a helper, you can actually continue using for await. The issue is that we're inside the same method where we just referenced this. If we defer to something like an asyncForEach, we've broken the inherent memory leak. We'll still need to indirect to get the directve value, but that's much simpler than writing manual iteration:

async function asyncForEach(iterable, cb) {
  for await (const v of iterable) {
    if (await cb(v) === false) break;
  }
}

Adding in === false allows us to break the iteration, if needed for any reason. And the await cb(v) would allow us to await a pause action, much like the current code:

class AsyncPause {
  _pause = null;
  _resolve = null;

  get() {
    return this._pause;
  }

  pause() {
    this._pause ||= new Promise(resolve => {
      this._resolve = resolve;
    });
  }
  
  resume() {
    this._resolve?.();
    this._pause = null;
    this._resolve = null;
  }
}

class AsyncDirective {
  _pauser = new AsyncPause();

  update(iterable) {
    const pauser = this._pauser;
    asyncForEach(iterable, async (value) => {
      if (pauser.get()) await pauser.get();

      // indirect to get directive here.

      if (directive._value !== iterable) return false;

      // render here
    });
  }
}

Nit: I actually got this incorrect. SMIs in v8 can only be 30bits, with 1 bit for the SMI tag and 1 bit for the sign. https://v8.dev/blog/react-cliff

Done

This is a lot of boilerplate that each of the directives are having to implement. Ideally, we'd just be using WeakRefs to weakly hold onto the directive this, and everything would be handled automatically. But not every browser will support that.

The good news is that directives support disconnected and reconnected, so we can implement a pseudo WeakRef with manual tracking very easily (the only difficult thing about implementing a WeakRef is making it automatic).

const backing = new WeakMap<WeakRefWrapper, unknown>();
class WeakRefWrapper {
  _weakref = null;
  
  constructor(obj: unknown) {
    if (typeof WeakRef === 'function') {
      this._weakref = new WeakRef(obj);
    } else {
      backing.set(this, obj);
    }
  }
  
  deref() {
    if (this._weakref) return this._weakref.deref();
    return backing.get(this);
  }
  
  // reconnected would call `weakthis.aquire(this)` to reestablish the weakref.
  acquire(obj) {
    if (!this._weakref) {
      // This should really be the same object as original, but we can't
      // verify that without strongly holding onto the original...
      backing.set(this, obj);
    }
  }
  
  // disconnected would call `weakthis.release()` to break the weakref.
  release() {
    if (!this._weakref) {
      backing.delete(this);
    }
  }
}

Then, we don't need this manual promise->directive tracking anymore. Most of the code can remain unchanged. The only difference is that we now refer to a weakthis instead of this. Combined with the asyncForEach helper, that means the majority of the code can stay the same while still preventing the memory leak.

I have a really basic example in https://output.jsbin.com/qumamir/quiet

With a helper, you can actually continue using for await. The issue is that we're inside the same method where we just referenced this. If we defer to something like an asyncForEach, we've broken the inherent memory leak. We'll still need to indirect to get the directve value, but that's much simpler than writing manual iteration:

async function asyncForEach(iterable, cb) {
  for await (const v of iterable) {
    if (await cb(v) === false) break;
  }
}

Adding in === false allows us to break the iteration, if needed for any reason. And the await cb(v) would allow us to await a pause action, much like the current code:

class AsyncPause {
  _pause = null;
  _resolve = null;

  get() {
    return this._pause;
  }

  pause() {
    this._pause ||= new Promise(resolve => {
      this._resolve = resolve;
    });
  }
  
  resume() {
    this._resolve?.();
    this._pause = null;
    this._resolve = null;
  }
}

class AsyncDirective {
  _pauser = new AsyncPause();

  update(iterable) {
    const pauser = this._pauser;
    asyncForEach(iterable, async (value) => {
      if (pauser.get()) await pauser.get();

      // indirect to get directive here.

      if (directive._value !== iterable) return false;

      // render here
    });
  }
}

We have access to previousValues here, so we could preemptively clean up the __pendingValueToAwaiterMap to prevent it from getting too large (map performance degrades with its size).

Really, I would just store the pending awaiters in an array.

Because you're performing disconnect() and reconnect() manually, there shouldn't be a need to indirect through a WeakMap. Just storing this directive as an own property will be considerably faster. If the DisconnectableAwaiter gets collected, then own property will go with it the same as the weak key. And if it doesn't get collected, then both the property and key would stick around.

Doh, right.

Nit: This is a slight change it behavior. for await will consume a sync iterator if the async iterator isn't define.

Reverted to for await

This still seems to be a lot more complex than the alternative. Every loop requires a new object, 2 promises, and 2/3 closures.

The AsyncPause and WeakRefWrapper (could just be PseudoWeakRef without the fallback to real WeakRef) requires just 2 objects and a closure (all of which can be reused for every iteration) and internal promise allocation.

Ok, I think that makes sense. Did another update to go with that approach. @justinfagnani @sorvell also PTAL

Nit: No longer async. If you want to return a promise still, we could return forAwaitOf(…) instead.

Good catch, it can be inlined back into update now.
Done.

Nit: do we want to copy this comment to the AsyncReplaceDirective, too?