_commitPromise is inefficient

[ruphin]

There's a problem with _commitPromise; because it changes this.value, it will cause all forms of "caching" for other commits to break. Because of this things like the current until directive implementation are really inefficient.

The until directive does this:

part.setValue(defaultContent);
part.commit();
part.setValue(promise);

Imagine that defaultContent is a Node. Right now, the node is removed from DOM and re-inserted every time the directive is processed, because part.setValue(promise) changes part.value which causes the next part.setValue(defaultContent) to fail to detect that it already rendered this node.

This happens regardless if the Promise is resolved or not.

Essentially, _commitPromise breaks the this.value API by setting it to a value that does not represent the currently rendered value, which then breaks the other commit functions that rely on that API.

It can be solved by using a this.promise-like variable and use that instead of this.value for promises.

I can make a PR for this.

[ruphin]

The solution is actually quite awkward, because you need to somehow know if something has been rendered after rendering the promise, and the only reliable way to do that is by checking if this.value has changed.

One solution is to call clear() when rendering a Promise so this.value = promise makes sense since the old value is removed, and updating the relevant directives like until to account for this. It does change the behaviour of rendering Promises to clearing any previously existing content (which may be reasonable?)

If the _commitPromise behaviour is to stay the same, all other commit functions will have to set this.promise to undefined to make sure the promise is inert when it resolves, which adds some overhead.

[ruphin]

Looking into it more, even the most basic usecase like this is affected:

const promise = Promise.resolve(document.createElement('span'));
render(html`<div>${promise}</div>`, document.body)

That will remove and re-attach the span on every render, because the promise and the promise result keep alternating this.value so nothing is ever cached.

[ruphin]

I'm working on a PR for this

[ruphin]

I made a PR that fixes the issue with Promises. After looking at issue #272 (Promises for AttributeParts) I realise that it's effectively possible to implement a Directive that wraps Promises, and solves the issue entirely.

So effectively, there is an alternative solution. We can change the render / setValue API to no longer accept Promises, and support Promise rendering through a Directive instead. Something like this:

import {promise} from 'lit-html/directives/promise.js';

const p = Promise.resolve('value');

render(html`<div>${promise(p)}</div>`, document.body);

Downside: less flexibility in rendering, if you throw in a Promise it will render [object Promise].
Upsides: Easier implementation. Fixes the issue for all Part types.

[justinfagnani]

Closed by #555