Refactor AttributePart to only have a single value #400

justinfagnani · 2018-07-14T00:50:16Z

This PR removes the SinglePart/MultiPart distinction. AttributeParts now only have a single value, but are associated with an AttributeCommitter that commits multiple values to an attribute.

This PR is based on #398.

The attribute handling code is a bit simpler in this factoring, though the "fast" path for attribute with a single value isn't implemented here, but it is implemented for PropertyCommitter. Not sure if this matters much.

bgotink · 2018-07-14T22:51:41Z

src/core.ts

      }
-      throw new Error(`Unknown part type ${templatePart.type}`);
+      throw [new Error(`Unknown part type ${templatePart.type}`)];


Is this intentional?

Yeah. I'll add some more comments, but now there's not a 1-1 correspondence between "template parts" and Parts. Maybe there's some further refinement there, but we need something with knowledge of a whole attribute.

It just seems weird to me to be throwing an Array<Error>, especially because there's no special handling of this array anywhere in lit-html itself.

oops, I didn't even see that it was an array of Error, I read this as a line returning multiple parts! Thanks!

bgotink · 2018-07-14T22:56:41Z

src/core.ts

-        (part as MultiPart).setValue(values, valueIndex);
-        valueIndex += part.size;
+      if (part !== undefined) {
+        part.commit();


Won't the current setup lead to multiple commits for some attributes?

e.g.

const template = (one, two) => html`<my-element some-attr="${one} ${two}"></my-element>`; render(template('lorem', 'ipsum'), container); // later on render(template('dolor', 'sit'), container);

Won't that change the some-attribute to "dolor sit" twice? This loop commits every part, and both AttributeParts in the attribute commit their AttributeCommitter because they're both dirty.

A possible solution would be to mark all parts of an AttributeCommitter as not dirty when running AttributeCommitter#commit()

This was a mistake, I was switching patterns and forgot to make committing only run once. I'll fix asap :)

Fixed and added a test

justinfagnani · 2018-07-18T19:19:15Z

@sorvell this is ready for review now. I like the shape of the API now, with the small exception of PartCallback returning an array of Parts. I didn't yet explore returning multiple TemplateParts for attributes. I can look into that or not... I don't think there will be that many PartCallback implementations our there.

core.ts gets a tiny bit smaller with this factoring, by about ~40 bytes.

sorvell

Just a couple of nits and questions ...

sorvell · 2018-07-27T18:23:09Z

src/core.ts

+      if (part !== undefined) {
+        const v = part._value;
+        if (v != null &&
+            (Array.isArray(v) || typeof v !== 'string' && v[Symbol.iterator])) {


The TODO went away and it's not obvious why.

What happens if one of the values is a Promise or a TemplateResult or a Node? Is it worth handling any of those weird cases more gracefully?

I think this TODO was mostly incorrect, and fully incorrect now that this code is in AttributeCommitter. The values here are after AttributePart has already handled them, passing them to getValue. Directive handling is the biggest thing we get out of calling getValue, and arrays of directive don't make sense, since there's no individual part to pass to the directive. TemplateResults don't make sense for attribute parts - using them here would be essentially toString'ing a DOM tree into an attribute.

sorvell · 2018-07-27T18:39:12Z

src/core.ts

+    let i = 0;
+    for (const part of this._parts) {
+      if (part !== undefined) {
+        part.setValue(values[i]);


Since AttributePart's actually apply to DOM during commit, but NodeParts do so in setValue, this could be an observable change to, say, a custom element. Does that concern you?

Yes, right now all attribute changes will apply before children changes. That shouldn't affect any well behaved elements, and won't affect anything that uses batching. I guess we can change this so it goes in DOM order.

Actually, it looks a bit tricky to defer the work in _setValue()

Still concerns me a bit. For example, events happen in commit but nodes are done in setValue so an event listener installed on a parent wouldn't see an event fired by a child custom element at connect time.

Note, this has been addressed since NodePart now applies changes to DOM in commit.

sorvell · 2018-07-27T21:16:29Z

src/lit-html.ts

+                  node as Element,
+                  templatePart.name!.slice(1),
+                  templatePart.strings!);
+              return comitter.parts;


It seems somewhat weird that you make AttributeParts for properties, but I'm not sure this is worth addressing, wdyt?

I actually do think we should add a trivial PropertyPart subclass of AttributePart just in case there's a need for a difference in behavior in the future and we want that change to be less likely to break anyone.

I'll add this.

This is done btw

justinfagnani · 2018-07-27T23:20:10Z

@sorvell added PropertyPart. PTAL

sorvell · 2018-07-28T00:18:06Z

src/core.ts

+    let i = 0;
+    for (const part of this._parts) {
+      if (part !== undefined) {
+        part.setValue(values[i]);


Still concerns me a bit. For example, events happen in commit but nodes are done in setValue so an event listener installed on a parent wouldn't see an event fired by a child custom element at connect time.

justinfagnani · 2018-07-28T00:24:10Z

Still concerns me a bit. For example, events happen in commit but nodes are done in setValue so an event listener installed on a parent wouldn't see an event fired by a child custom element at connect time.

Good example. I'll write a test for this case.

…eComitter.

TODO: lit-extended tests, fix AttributeCommitter to write only once per attribute.

keanulee · 2018-08-13T17:49:32Z

src/lit-html.ts

-    } else {
-      // Interpolation, so interpolate
-      value = this._interpolate(values, startIndex);
+    this._value = !!value;


This line should be after the dirty check.

justinfagnani · 2018-08-14T23:22:43Z

@sorvell PTAL

justinfagnani · 2018-08-15T22:57:25Z

src/lit-html.ts

+                  node as Element,
+                  templatePart.name!.slice(1),
+                  templatePart.strings!);
+              return comitter.parts;


This is done btw

justinfagnani · 2018-08-15T22:57:34Z

src/lit-html.ts

+
+  setValue(value: any): void {
+    this._pendingValue = value;
+    // value = !!value;


will remove

sorvell

Also should add documentation about the changes to directives, and are there any examples to update?

sorvell · 2018-08-15T22:47:32Z

src/core.ts

-  }
-  return value === null ? undefined : value;
-};
+// export const getValue = (part: Part, value: any) => {


remove cruft

sorvell · 2018-08-15T22:48:28Z

src/core.ts

@@ -462,126 +462,153 @@ export const noChange = {};
 */
 export {noChange as directiveValue};

-export const _isPrimitiveValue = (value: any) => value === null ||
-    !(typeof value === 'object' || typeof value === 'function');
+export const _isPrimitiveValue = (value: any) =>


exporting a _ prefixed function is weird since that implies private.

Renamed to isPrimitive

sorvell · 2018-08-15T22:52:13Z

src/core.ts

+    value = value === null ? undefined : value;
+    if (value !== noChange || !_isPrimitiveValue(value) || value !== this._value) {
+      this._value = value;
+      if (!isDirective(value)) {


Please add a comment explaining why this is important.

sorvell · 2018-08-15T22:53:26Z

src/core.ts

+  }
+
+  setValue(value: any): void {
+    value = value === null ? undefined : value;


Why reset null to undefined?

This was inlined from getValue(), but I think it's only necessary on NodePart. Testing without it.

sorvell · 2018-08-15T22:53:46Z

src/core.ts

-    } else {
-      value = this._interpolate(values, startIndex);
+  commit() {
+    // console.log('commit', this._value);


remove comment

sorvell · 2018-08-15T22:57:48Z

src/core.ts

+      this._pendingValue = noChange;
+      directive(this);
+    }
+    const value = this._pendingValue === null ? undefined : this._pendingValue;


Again, why reset null to undefined?

textContent = null would render "null", so this conversion was done in getValue() previous. Moved into _setText()

sorvell · 2018-08-15T22:59:42Z

src/core.ts

-      if (value === this._previousValue) {
-        return;
+      if (value !== this._value) {
+        this._setText(value);


Should these functions be called commit* since they occur in the commit phase? This seems easier to understand since setValue is a different phase.

sorvell · 2018-08-15T23:02:04Z

src/core.ts

+    let i = 0;
+    for (const part of this._parts) {
+      if (part !== undefined) {
+        part.setValue(values[i]);


Note, this has been addressed since NodePart now applies changes to DOM in commit.

sorvell · 2018-08-15T23:04:05Z

src/core.ts

+
+export class AttributePart implements Part {
+  committer: AttributeCommitter;
+  _value: any = undefined;


Seems wrong that this is _value since directives can and do access it. (Same for NodePart).

Renamed to value

sorvell · 2018-08-15T23:06:41Z

src/lit-html.ts

+
+  setValue(value: any): void {
+    this._pendingValue = value;
+    // value = !!value;


remove cruft

justinfagnani

@sorvell PTAL

justinfagnani · 2018-08-17T01:10:37Z

src/core.ts

+
+export class AttributePart implements Part {
+  committer: AttributeCommitter;
+  _value: any = undefined;


Renamed to value

justinfagnani · 2018-08-17T01:11:50Z

src/core.ts

+  }
+
+  setValue(value: any): void {
+    value = value === null ? undefined : value;


This was inlined from getValue(), but I think it's only necessary on NodePart. Testing without it.

justinfagnani · 2018-08-17T01:12:05Z

src/core.ts

+    value = value === null ? undefined : value;
+    if (value !== noChange || !_isPrimitiveValue(value) || value !== this._value) {
+      this._value = value;
+      if (!isDirective(value)) {


justinfagnani · 2018-08-17T01:12:18Z

src/core.ts

-      value = this._interpolate(values, startIndex);
+  commit() {
+    // console.log('commit', this._value);
+    while (isDirective(this._value)) {


justinfagnani · 2018-08-17T01:14:21Z

src/core.ts

+      this._pendingValue = noChange;
+      directive(this);
+    }
+    const value = this._pendingValue === null ? undefined : this._pendingValue;


textContent = null would render "null", so this conversion was done in getValue() previous. Moved into _setText()

justinfagnani · 2018-08-17T01:14:30Z

src/core.ts

-      if (value === this._previousValue) {
-        return;
+      if (value !== this._value) {
+        this._setText(value);


justinfagnani · 2018-08-17T01:15:50Z

src/lit-html.ts

+
+  setValue(value: any): void {
+    this._pendingValue = value;
+    // value = !!value;


justinfagnani · 2018-08-17T01:18:54Z

src/core.ts

+  }
+
+  commit() {
+    while (isDirective(this._pendingValue)) {


Possibly a little, maybe on the base class. I'd like to do another refactoring attempt later as well.

sorvell

LGTM

bgotink reviewed Jul 14, 2018

View reviewed changes

bgotink mentioned this pull request Jul 16, 2018

Duplicate property on element causes subsequent interpolation to not render #403

Closed

justinfagnani requested a review from sorvell July 18, 2018 19:14

justinfagnani force-pushed the new-syntax branch from e715f3c to ad7b5ca Compare July 19, 2018 17:59

justinfagnani force-pushed the parts-api branch from 104069f to 09678d8 Compare July 19, 2018 19:14

justinfagnani changed the base branch from new-syntax to master July 19, 2018 19:14

justinfagnani force-pushed the parts-api branch from 09678d8 to 994bb7a Compare July 19, 2018 19:16

sorvell requested changes Jul 27, 2018

View reviewed changes

sorvell requested changes Jul 28, 2018

View reviewed changes

justinfagnani added 11 commits July 28, 2018 11:45

WIP: Refactor AttributePart to only have a single value. Add Attribut…

aa6da5a

…eComitter.

WIP: lit-html tests passing.

5ef6251

TODO: lit-extended tests, fix AttributeCommitter to write only once per attribute.

All tests passing

d75bf0a

Ensure attributes & properties are only set once for multiple bindings

3f1f015

Ensure PropertyCommitter sets properties

3f0a8ec

Format

123f5a1

Remove unused imports

1f956ad

Fix EventPart

115be14

Re-enable shady tests

3ec481f

Add PropertyPart

0f08697

Require NodePart#commit to be called before writing values to DOM

89fe937

justinfagnani force-pushed the parts-api branch from eb28d07 to 89fe937 Compare July 28, 2018 18:45

Checkpoint: inline getValue into NodePart#setValue

a625262

justinfagnani force-pushed the parts-api branch from b4c6c29 to 39af054 Compare July 30, 2018 23:48

justinfagnani added 3 commits July 30, 2018 17:20

All tests passing

befbbc6

Don’t use custom elements in lit-html_test.ts

9f11020

Checkpoint

fbc3eae

WIP (remove getValue…)

fdb607e

keanulee reviewed Aug 13, 2018

View reviewed changes

justinfagnani added 2 commits August 14, 2018 14:32

Fix BooleanAttributePart?

9f1be79

Revert adding the keyValue argument to setValue

d01c7cd

justinfagnani force-pushed the parts-api branch from 39af054 to d01c7cd Compare August 14, 2018 23:21

justinfagnani added 2 commits August 14, 2018 16:39

Add web components polyfills for IE11

d50c9c7

Update webcomponents polyfill

438b052

justinfagnani force-pushed the parts-api branch from 45c435c to 438b052 Compare August 15, 2018 00:46

justinfagnani added 2 commits August 14, 2018 19:27

¯\_(ツ)_/¯

52f178c

Don’t use new Set(iterable) in tests

d255783

justinfagnani commented Aug 15, 2018

View reviewed changes

Remove commented out code

4219f6d

sorvell requested changes Aug 15, 2018

View reviewed changes

Address review comments

797af69

justinfagnani commented Aug 17, 2018

View reviewed changes

justinfagnani added 2 commits August 17, 2018 10:18

Update web-component-tester to version compatible with Node 10

a70d615

Format

a627939

sorvell approved these changes Aug 17, 2018

View reviewed changes

justinfagnani merged commit decdb52 into master Aug 17, 2018

justinfagnani deleted the parts-api branch August 17, 2018 23:21

This was referenced Aug 21, 2018

Align NodePart and AttributePart interfaces #360

Closed

Inconsistencies with "previousValue" property on parts #198

Closed

Refactor AttributePart to only have a single value #400

Refactor AttributePart to only have a single value #400

Conversation

justinfagnani commented Jul 14, 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

Choose a reason for hiding this comment

justinfagnani commented Jul 18, 2018

sorvell left a 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

sorvell Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinfagnani Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinfagnani commented Jul 27, 2018

sorvell Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

justinfagnani commented Jul 28, 2018

Choose a reason for hiding this comment

justinfagnani commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorvell left a 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

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

justinfagnani left a 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

sorvell left a comment

Choose a reason for hiding this comment

justinfagnani commented Jul 14, 2018 •

edited

Loading

sorvell Jul 28, 2018 •

edited

Loading

justinfagnani Jul 27, 2018 •

edited

Loading

sorvell Jul 28, 2018 •

edited

Loading