Inconsistencies with "previousValue" property on parts

[mraerino]

When recently implementing a directive I ran into issues with the previousValue property on the Part interface.

Expected behaviour

The Readme says it should be usable like this:

const render = () => html`
  <div>
    ${directive((part) => part.setValue((part.previousValue + 1) || 0))}
  </div>`;

Which implies that the previousValue property will take the value set through setValue on every class implementing the Part interface.

Actual behaviour

Only a subset of Parts actually track their previous value, but they only do in a private property. So actually the property is not available on the Part interface.

Proposed solutions

1. Make a previousValue property mandatory on the Part interface and implement it in every part type.
2. Make Part an abstract class instead of an interface and use it as a base class for the part types. It then could implement a default get previousValue() accessor which returns null.

Obstacles

This would certainly have to be a breaking change since it would require changes on the user's classes which implement the Part interface

How could this property be useful to directive creators?

• See the example from the Readme
• Implementation of element decoration directives that want to track the decorated element on subsequent calls
• Implementation of the when() directive

[mraerino]

So I just noticed the privateness of the _previousValue property will likely change in #118

The bad thing: The typescript compiler will still prevent you from using this property on the Part interface.

[justinfagnani]

Thanks for the issue @mraerino,

Indeed, the usage of _previousValue differs in different places in a way that we should probably try to fix.

My thinking up to now was that _previousValue was essentially private storage with fairly loose semantics so that whatever code path needed to use it could do so as it saw fit. The first time I set it to something other than the value passed to setValue was with nested templates, whose value is represented by TemplateResult. What we need to preserve in that case was the TemplateInstance created from the TemplateResult, so that we don't create a new one and can recurse into it for updates. repeat() does something similar by storing an array of previous values, rather than the iterable passed to it, which might not be consumable anymore. If we do give _previousValue stricter semantics, we'll have to account for extra state like this. Reusing _previousValue has the benefit of not needing an extra slot for this state and not making the Part instances change shape over time.

I'm implicitly assuming here that if another value type is passed to a part, that the code path that handles that type will overwrite _previousValue. I haven't had in mind any cases where parts or directives would need a guarantee that _previousValue is === the object passed to setValue(). If you have a case like that I'd love to hear more details about it. I think the README section is either outdated, or was a bit of my trying to simplify reality to show how a directive could work. The other cases you mentioned I'm not so sure really need those _previousValue semantics.

At the very least this needs to be documented better, so 👍

[mraerino]

So. I finally got time to put my head into this again.

It seems like _previousValue is not really suitable to be used by the directive api, so maybe we should not touch it, but rather make a concept that helps the Directive API in a good way.

My directive (simplified)

• The directive I wrote is a pure function producing DOM from its parameters.
• It takes a TemplateResult and some trivial parameters
• The result written to the NodePart will be the result of the TemplateResult enhanced with some attributes and event handlers

So basically my use case is to apply DRY by delegating the application of common attributes and handlers to a directive.

Problem: No performance optimization for the template

When I'm not able to keep track of the latter parameters I will not be able to figure out when to re-render the template, so I must do it every time. Meh.

When looking into the directive api I was pretty happy there already was some kind of concept to help with dirty checking: The previousValue property.

In the end, for the reasons outlined above, I did not use the property of the part, but extended the part object with my own property:

export interface DirtyCheckingPart extends NodePart {
    previousValue: any;
}

export const decorateInput = (
    literal: TemplateResult,
    name: string
): DirectiveFn => directive((part: DirtyCheckingPart) => {
    const previous = part.previousValue || {};

    const instance: TemplateInstance = previous.template !== literal.template
        ? new TemplateInstance(literal.template, part.instance._partCallback)
        : previous.instance;

    let element: HTMLElement = previous.element;
    if(previous.instance !== instance) {
        const fragment: DocumentFragment = instance._clone();
        element = fragment.childNodes[0]
    }

    instance.update(literal.values);

    if(previous.element !== element || previous.name !== name) {
        // custom logic messing with the element
    }

    part.previousValue = {
        instance,
        element,
        name,
        template: literal.template
    };

    part.setValue(element);
    return element;
});

I'm setting the value as well as returning it because of recent changes in the Directive API - this way will work with every version.

Proposal

The part passed to the Directive API should expose a method/property to get the previous state of the part. By default this state should be set by the setValue function of the part.
For special cases there should be a method/property to set a state that differs from the part value.

[justinfagnani]

This was mostly fixed in #400 (and some other PRs) which standardized on a public .value property for all Parts, and created a single Part interface.

.value is still not always exactly the same value passed to setValue() because in many cases that value is not the important thing to remember. This is especially true for TemplateResults, where the previous TemplateResult isn't what matters, but it's associated Template.