Add when directive #439

LarsDenBakker · 2018-08-19T14:07:35Z

This PR adds a when directive, which can be used to efficiently switch between templates.

Based on the condition, either the 'if' or the 'else' value is rendered. When switching between conditions, the previously rendered DOM is cached and re-used instead of recreated.

Caching is done by reparenting the the rendered content into a detached dom node when the condition switches. I didn't change any code in the actual library itself, but I think might be useful to create some extra helper functions for moving around part contents. Repeat could also make use of this.

I think this concept could be extended further by creating a 'switch' directive which switches between dom content and caches nodes.

ruphin · 2018-08-19T14:24:41Z

This implementation evaluates both possible outcomes on each resolution. Have you considered a lazy-evaluated implementation? It makes for a less clean API since you'll have to wrap each outcome in an anonymous function, but I think the behaviour is more "safe" and in line with expectation.

I think this implementation is a footgun, and it will invariably lead to people accidentally evaluating code they don't intend to.

LarsDenBakker · 2018-08-19T15:08:36Z

Yeah, I agree. Lazy evaluation is something we will need in other places, it'd be nice to handle this in the core somehow. Currently functions are rendered as a string, maybe lit could just unpack the function when passed as a value. Would that introduce much overhead for regular rendering?

ruphin · 2018-08-19T17:43:09Z

I think lazy evaluation is only necessary in a context where multiple results are possible.

If lit is to assume that all functions return results, should it always just call the function and render the results instead? What if the function returns another function? There's a lot of corner cases to handle here, and I don't think it should be the responsibility of core to do this.

The issue can easily be solved in the relevant directives, in this case:

if (condition) {
  value is ifValue();
} else {
  parts.reverse();
  value = elseValue();
}

Which changes the API to:

html`${when(condition, () => html`<div></div>`, () => html`<span></span>`)}`

It's just a question of what API you want the directive to have. As I mentioned above, I think any directive with multiple options should always be lazy-evaluated by default, because it is easy for developers to make mistakes otherwise.

ruphin · 2018-08-19T14:31:13Z

src/lib/when.ts

+  const parts: PartCache =  [
+    new NodePart(parentPart.templateFactory),
+    new NodePart(parentPart.templateFactory),
+    document.createElement('div'),


Have you considered using a DocumentFragment instead of a div? It allows for easier relocating of nodes, and also makes more sense semantically.

I've never worked with DocumentFragment before, I will look into that.

I'm pretty sure you are familiar with it :)
For example, HTMLTemplateElement uses DocumentFragment to store the template contents:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLTemplateElement/content

Yeah, I've seen it before but just never used myself or looked much into it. Definitely is the better option here.

ruphin · 2018-08-19T19:44:45Z

src/lib/when.ts

+
+import {directive, reparentNodes, DirectiveFn, NodePart} from '../core.js';
+
+type PartCache = [NodePart, NodePart, DocumentFragment];


Is there a reason to have this an Array instead of an Object with named keys?

{ ifPart: NodePart, elsePart: NodePart, container: DocumentFragment }

Agreed, but a small naming nit: since this isn't exactly an if statement, I'd name the properties semantically as truePart and falsePart

I see what you mean. I started out with just two parts, so I made it a tuple to keep it simple and because I did the reverse() later on. Later on I added the container element.

justinfagnani · 2018-08-19T23:28:02Z

src/lib/when.ts

+export const when = (condition: boolean, ifValue: () => any, elseValue: () => any): DirectiveFn<NodePart> =>
+    directive((part: NodePart) => {
+      // Create cache if this was the first render
+      if (!partCaches.has(part)) {


Using Map#has() causes there to be two map lookups in the hit case and three in the miss case. Use Map#get() instead and check for undefined:

let partCache = partCaches.get(part); if (partCache === undefined) { partCache = { truePart: new NodePart(parentPart.templateFactory), falsePart: new NodePart(parentPart.templateFactory), container: document.createDocumentFragment(), }; partCaches.set(part, partCache); }

This is one lookup for a hit, two for a miss.

justinfagnani · 2018-08-19T23:28:44Z

src/lib/when.ts

+
+import {directive, reparentNodes, DirectiveFn, NodePart} from '../core.js';
+
+type PartCache = [NodePart, NodePart, DocumentFragment];


Agreed, but a small naming nit: since this isn't exactly an if statement, I'd name the properties semantically as truePart and falsePart

justinfagnani · 2018-08-19T23:30:41Z

src/lib/when.ts

+      if (condition) {
+        value = ifValue();
+      } else {
+        parts.reverse();


this reverse() is a bit clever and hard to follow, IMO. Rather than a parts array, can you create two named variables?

justinfagnani · 2018-08-19T23:31:45Z

src/lib/when.ts

+      const parts = [partCache[0], partCache[1]];
+      let value;
+
+      if (condition) {


Before even checking the condition, I think we want to check that it's different from last time. If it's the same we can just propagate the current value.

The value to set depends on the condition, even if it didn't change. So we still need to do some work. You're right though currently we do a lot of dom manipulation when not necessary.

I've separated the work for moving dom nodes when switching conditions.

justinfagnani · 2018-08-19T23:31:58Z

src/lib/when.ts

+
+      // Set the new part's value
+      newPart.setValue(value);
+      newPart.commit();


Shouldn't need a commit here.

The commit is on the 'child part' of true or false condition, not on the 'parent part' of the directive. Without a commit, the value doesn't update.

justinfagnani · 2018-08-19T23:34:38Z

src/test/lib/when_test.ts

+    });
+
+    test('caches if/then templates', () => {
+      let ifEl;


these can be const

ruphin · 2018-08-20T07:19:33Z

src/lib/when.ts

+      const nextValue = condition ? trueValue() : falseValue();
+
+      // Swap nodes if condition changed from previous
+      if (condition !== prevCondition) {


You are checking for equality against the prevCondition, but what you really want is to check if they are both truthy or both falsy. Does TypeScript automatically add a boolean conversion to condition because it is typed as a boolean? If not, it might be worth to manually cast condition to a boolean value with condition = !!condition to make sure this check returns what you expect.

Typescript forces consumers to use a boolean value, which I like personally. But for JS I guess the expectation that it checks truthiness instead.

If we want to support that, then it needs some extra checks as now I depend on boolean equality so that the initial value of undefined for the cached condition does not cause it to match the false condition.

Don't assume that consumer will be using TypeScript. You can rely on the type as much as you want to assert that the docs have to be followed exactly. In this case, JS users are so used to "truthiness" that we should support it.

condition = !!condition is a fine way to do this, and you could even type condition as unknown. Make sure you test initial render in both the true and false cases.

justinfagnani · 2018-08-20T16:23:42Z

src/lib/when.ts

+      const nextValue = condition ? trueValue() : falseValue();
+
+      // Swap nodes if condition changed from previous
+      if (condition !== prevCondition) {


Don't assume that consumer will be using TypeScript. You can rely on the type as much as you want to assert that the docs have to be followed exactly. In this case, JS users are so used to "truthiness" that we should support it.

condition = !!condition is a fine way to do this, and you could even type condition as unknown. Make sure you test initial render in both the true and false cases.

justinfagnani · 2018-08-20T16:24:57Z

src/lib/when.ts

+  let cache = partCaches.get(parentPart);
+
+  // Create a new cache if this is the first render
+  if (!cache) {


Use an explicit check instead of relying on boolean conversion: if (cache === undefined)

justinfagnani · 2018-08-20T16:26:13Z

src/lib/when.ts

+ * @param parentPart the parent part
+ * @returns the cache for the given parent. creates a new cache if none exists
+ */
+function getPartCache(parentPart: NodePart) {


Since this function is only called once I'd just linine it. Especially because it has side-effects on parentPart

justinfagnani · 2018-08-20T16:30:30Z

src/lib/when.ts

+      const partCache = getPartCache(part);
+      const { truePart, falsePart, cacheContainer, prevCondition } = partCache;
+
+      const nextPart = condition ? truePart : falsePart;


Add some comments on what next and previous mean here.

aadamsx · 2018-08-20T18:42:01Z

Don't assume that consumer will be using TypeScript. You can rely on the type as much as you want to assert that the docs have to be followed exactly. In this case, JS users are so used to "truthiness" that we should support it.

I for one will be using this lib when it his 1.0 and WILL NOT be using Typescript to save my life. :)

justinfagnani · 2018-08-21T04:04:34Z

@LarsDenBakker #436 is submitted, so please rebase to master now.

LarsDenBakker · 2018-08-21T19:16:51Z

I rebased onto master

LarsDenBakker requested a review from justinfagnani as a code owner August 19, 2018 14:07

ruphin reviewed Aug 19, 2018

View reviewed changes

justinfagnani requested changes Aug 19, 2018

View reviewed changes

ruphin reviewed Aug 20, 2018

View reviewed changes

justinfagnani reviewed Aug 20, 2018

View reviewed changes

Lars den Bakker added 5 commits August 21, 2018 21:11

Added when directive

6e704a0

Use DocumentFragment for caching nodes in when directive

2671943

Make when parameters functions

db38e29

Address review comments

accaeab

Inline cache creation in when directive

bc0137b

Handle truthiness/falsiness in when directive

559adcc

justinfagnani approved these changes Aug 22, 2018

View reviewed changes

justinfagnani merged commit 47ae12e into lit:master Aug 22, 2018

neuronetio pushed a commit to neuronetio/lit-html that referenced this pull request Dec 2, 2019

Add when directive (lit#439)

cb73e8e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add when directive #439

Add when directive #439

LarsDenBakker commented Aug 19, 2018 •

edited

Loading

ruphin commented Aug 19, 2018

LarsDenBakker commented Aug 19, 2018

ruphin commented Aug 19, 2018

ruphin Aug 19, 2018

LarsDenBakker Aug 19, 2018

ruphin Aug 19, 2018

LarsDenBakker Aug 19, 2018

ruphin Aug 19, 2018

justinfagnani Aug 19, 2018

LarsDenBakker Aug 20, 2018

justinfagnani Aug 19, 2018

justinfagnani Aug 19, 2018

justinfagnani Aug 19, 2018

justinfagnani Aug 19, 2018

LarsDenBakker Aug 20, 2018

justinfagnani Aug 19, 2018

LarsDenBakker Aug 20, 2018

justinfagnani Aug 19, 2018

ruphin Aug 20, 2018

LarsDenBakker Aug 20, 2018 •

edited

Loading

justinfagnani Aug 20, 2018

justinfagnani Aug 20, 2018

justinfagnani Aug 20, 2018

justinfagnani Aug 20, 2018

justinfagnani Aug 20, 2018

aadamsx commented Aug 20, 2018 •

edited

Loading

justinfagnani commented Aug 21, 2018

LarsDenBakker commented Aug 21, 2018


		import {directive, reparentNodes, DirectiveFn, NodePart} from '../core.js';

		type PartCache = [NodePart, NodePart, DocumentFragment];

Add when directive #439

Add when directive #439

Conversation

LarsDenBakker commented Aug 19, 2018 • edited Loading

ruphin commented Aug 19, 2018

LarsDenBakker commented Aug 19, 2018

ruphin commented Aug 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LarsDenBakker Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aadamsx commented Aug 20, 2018 • edited Loading

justinfagnani commented Aug 21, 2018

LarsDenBakker commented Aug 21, 2018

LarsDenBakker commented Aug 19, 2018 •

edited

Loading

LarsDenBakker Aug 20, 2018 •

edited

Loading

aadamsx commented Aug 20, 2018 •

edited

Loading