[lit-html] add custom equality support to live #4763

cristinecula · 2024-09-13T22:16:09Z

Added the ability to provide a custom equality function as the second argument to live, to be able to handle cases where a type conversion happens internally.

changeset-bot · 2024-09-13T22:16:12Z

🦋 Changeset detected

Latest commit: d598cdb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
lit-html	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

google-cla · 2024-09-13T22:16:13Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

justinfagnani · 2024-09-13T23:29:50Z

@cristinecula thanks for the PR

Was there an issue yet for this design? At a glance, this looks like a nice change, but we do ask that we have an issue or RFC to discuss additions before PRs: https://github.com/lit/lit/blob/main/CONTRIBUTING.md#pull-requests

I'll take a look at the change soon!

cristinecula · 2024-09-14T07:49:02Z

Hi, @justinfagnani!

It's an issue I ran into during my work. Sorry, I should have reported it first. The gist of it is this:

I have a number input and I store it's value as a number. I use live to bind the value back to the value property. Because there is a type mismatch, live will update the property all the time. This causes UX issues when users try to edit decimals.

You can test it in this reproduction.

Reproduction steps:

type 1.5 in the number input
attempt to change the value to 1.2, by deleting 5 with backspace and typing 2
observe that the input now says 21 instead of 1.2 (on chromium, on firefox it will be 12)

My addition to live would allow me to customize the equality, such that instead of strict equality, I also do a type conversion to string, thus preventing lit from unnecessarily updating the value property.

While writing the issue description and reproduction gists, I've come to realize that I can bind to valueAsNumber, which simply works out of the box. This fixes my original issue, rendering this PR unnecessary. I guess it could still be useful in some situations. What do you think?

cristinecula · 2024-09-16T08:33:13Z

Here is an example where having a custom live equality function makes a lot of sense:

When you want to store a number as fixed point notation.

Playground

Try writing 123 in both inputs.

justinfagnani · 2024-09-16T16:14:11Z

Thanks for the details @cristinecula. These are the things that would be great in an issue. You can even open an issue and PR at the some time so the issue can be the point of reference if there are changes in how we solve something. Like if we decide this is working fine, people might find that in a closed issue, but not a closed PR.

As for whether we still need this change... tough call. In general I like the feature, but I'm by default slightly hesitant to add more code w/o a clear use case example. Thanks for the second example. It behaves a little funny because of the rounding behavior of toFixed. If you type in 1.23456 it'll get rounded to 1.235 and from there any keystroke on 5 or greater will just increment the number by .001 :)

But I think the important takeaway focus management. live() gives us the ability to overwrite the binding value when Lit doesn't think it's necessary. This change gives us more ability to not do that, which I don't think is usually a big deal - setting a property sometimes shouldn't matter much, but in input cases it can preserve focus when you don't set a new value, which is important.

I think it might be better to actually have more customizable focus management for inputs - like to restore the cursor to a specific index, but that seems more than what live() is responsible for.

So I think this might be useful, and it's not much code and not in core. I'll get to reviewing it now 👍

justinfagnani · 2024-09-16T16:39:15Z

packages/lit-html/src/directives/live.ts

+    const element = part.element as Element;
    const name = part.name;
+    const _equal = equal ?? defaults[part.type];
+
+    if (_equal(value, getValue[part.type](element, name))) return noChange;


I think we can inline the getValue function to avoid some property lookups:

Suggested change

const element = part.element as Element;

const name = part.name;

const _equal = equal ?? defaults[part.type];

if (_equal(value, getValue[part.type](element, name))) return noChange;

const {type, name, element} = part;

const currentValue =

type === PartType.PROPERTY

? element[name as keyof typeof element]

: type === PartType.ATTRIBUTE

? element.getAttribute(name)

: element.hasAttribute(name);

if (equal(value, currentValue)) {

return noChange;

}

justinfagnani · 2024-09-16T16:40:07Z

packages/lit-html/src/directives/live.ts

+    part: AttributePart & {
+      readonly type: ValidPartType;
+    },
+    [value, equal]: DirectiveParameters<this>


Defaults can be set in the destructuring parameter:

Suggested change

[value, equal]: DirectiveParameters<this>

[value, equal = defaults[part.type]]: DirectiveParameters<this>

justinfagnani · 2024-09-16T16:46:36Z

packages/lit-html/src/directives/live.ts

@@ -88,6 +100,14 @@ class LiveDirective extends Directive {
 * `live()` should not be used when the binding will cause a type conversion. If
 * you use `live()` with an attribute binding, make sure that only strings are
 * passed in, or the binding will update every render.
+ *
+ * To customize the equality check, you can pass in a function as the second


I think the previous paragraph needs to be updated a bit to refer to that as default behavior.

Updated. I'm not sure if the wording is clear enough, but I was not sure how to make it better.

justinfagnani · 2024-09-16T16:49:05Z

packages/lit-html/src/directives/live.ts

@@ -14,6 +14,24 @@ import {
 } from '../directive.js';
 import {isSingleExpression, setCommittedValue} from '../directive-helpers.js';

+type ValidPartType = 1 | 3 | 4;
+type Element = HTMLElement & {[key: string]: unknown};


I don't think we should override the global Element type, and this type could remove a lot of type-checking in the whole file. We can cast at the access site.

Resolved. Thank you for the insight. I'm still a novice at typescript.

cristinecula · 2024-09-17T07:49:48Z

Thank you for the thorough analysis and feedback! I've updated the PR with the suggested changes.

That rounding input behavior is indeed funny. You have a keen eye to catch that!
It was actually a bug in the equality function. This updated version does not have that issue.

I'm not sure if I should go and create an issue referencing this PR or not. Please let me know and I will do it ASAP!

[lit-html] add custom equality support to live

f7f716a

Added the ability to provide a custom equality function as the second argument to live, to be able to handle cases where a type conversion happens internally.

cristinecula requested a review from kevinpschaaf as a code owner September 13, 2024 22:16

justinfagnani self-requested a review September 13, 2024 23:29

cristinecula mentioned this pull request Sep 16, 2024

feat(live): improved version of live Neovici/cosmoz-utils#184

Draft

justinfagnani requested changes Sep 16, 2024

View reviewed changes

Integrate review feedback

d598cdb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lit-html] add custom equality support to live #4763

[lit-html] add custom equality support to live #4763

cristinecula commented Sep 13, 2024

changeset-bot bot commented Sep 13, 2024 •

edited

Loading

google-cla bot commented Sep 13, 2024

justinfagnani commented Sep 13, 2024

cristinecula commented Sep 14, 2024

cristinecula commented Sep 16, 2024

justinfagnani commented Sep 16, 2024

justinfagnani Sep 16, 2024

cristinecula Sep 17, 2024

justinfagnani Sep 16, 2024

cristinecula Sep 17, 2024

justinfagnani Sep 16, 2024

cristinecula Sep 17, 2024

justinfagnani Sep 16, 2024

cristinecula Sep 17, 2024

cristinecula commented Sep 17, 2024

-    const element = part.element as Element;
-    const name = part.name;
-    const _equal = equal ?? defaults[part.type];
-    if (_equal(value, getValue[part.type](element, name))) return noChange;
+    const {type, name, element} = part;
+    const currentValue =
+      type === PartType.PROPERTY
+        ? element[name as keyof typeof element]
+        : type === PartType.ATTRIBUTE
+          ? element.getAttribute(name)
+          : element.hasAttribute(name);
+    if (equal(value, currentValue)) {
+      return noChange;
+    }

	[value, equal]: DirectiveParameters<this>
	[value, equal = defaults[part.type]]: DirectiveParameters<this>

[lit-html] add custom equality support to live #4763

Are you sure you want to change the base?

[lit-html] add custom equality support to live #4763

Conversation

cristinecula commented Sep 13, 2024

changeset-bot bot commented Sep 13, 2024 • edited Loading

🦋 Changeset detected

google-cla bot commented Sep 13, 2024

justinfagnani commented Sep 13, 2024

cristinecula commented Sep 14, 2024

cristinecula commented Sep 16, 2024

justinfagnani commented Sep 16, 2024

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

cristinecula commented Sep 17, 2024

changeset-bot bot commented Sep 13, 2024 •

edited

Loading