feat: Template class object binding #3950

pmdartus · 2024-01-18T07:22:23Z

Details

This PR implements the following RFC: Template class object binding.
This proposal aims to improve the Developer Experience (DX) around managing components with complex styles by enabling developers to describe the classes applied to an element using JavaScript objects.

export default class CustomButton extends LightningElement {
  variant = null;
  position = null;

  fullWidth = false;
  stretch = false;

  get computedClassNames() {
    return [
      "button__icon",
      this.variant && `button_${this.variant}`,
      this.position && `button_${this.position}`,
      {
        "button_full-width": this.fullWidth,
        "button_stretch": this.stretch,
      },
    ];
  }
}

<template>
  <button class={computedClassNames}>
    <slot></slot>
  </button>
</template>

This new feature will only be enabled for components running on API version 61 and above.

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

This change is guarded behind API versioning. Refer to the RFC for more details.

Does this pull request introduce an observable change?

⚠️ Yes, it does include an observable change.

LWC components that are always running on the latest LWC version (OSS and Salesforce internal components) will see a difference in behavior if they are already binding the class property with a non-string value. This change of behavior is discussed in the RFC.

GUS work item

No work item.

pmdartus · 2024-01-18T08:33:28Z

...es/@lwc/integration-karma/test-hydration/mismatches/type-coercion-to-string/x/main/main.html

@@ -1,4 +1,4 @@
 <template>
    <!-- style attr is validated at compile time -->
-    <p class={num} data-attr={num}>{num}</p>


Remove the class from the hydration test mismatch as before this change the class={123} was serialized to class="123" but after this change, the same expression gets serialized to class="".

As far as I can tell, we are only running the hydration suite for the latest API version. Because of this, there is no way to check the before and after behavior. That said, I don't think this change reduces the coverage of this test as it already checks attribute coercion with the data-attr.

Why do we want class={123} to become nothing, rather than a string? 123 is not an "empty" value, so my baseline expectation is that it would be shown, rather than dropped. Or, if we definitely don't want to allow non-string values, we should throw a TypeError.

@wjhsf If possible, I would prefer to avoid making assumptions about what the component author intended.

The real question here is whether or not the engine should surface an error when the class receives a value that is neither a string, an array nor a JavaScript plain object. I would use "least astonishment principle" here and follow the semantics as defined by other popular UI frameworks and libraries:

Vue ignores all non-string, array, and objects in both devs and production.

classnames coerce number to string (commit), but ignore other objects in dev and prod.

If we take a step back, I don't expect developers to define class names as numbers. Developers are asking for trouble if they are doing this. For this reason, my recommendation would be to stick to Vue's semantics regarding number handling.

All of the frameworks and libraries silently ignore all the "invalid" class name values in both dev and prod today and I haven't seen any pushback from the community. It should be safe for LWC to do the same for now. That said, if we see developers struggling with this, we can always add warnings in dev mode and turn this warning into an error in the next major version.

This behavior is surprising to me. In the current implementation, numbers are coerced to strings for both class and data-foo:

<h1 data-foo={num} class={num}>Hello world!</h1>

Renders:

<h1 class="123" data-foo="123">Hello world!</h1>

Rendering to an empty string definitely seems wrong to me, and it seems wronger to have a hydration mismatch. We have plenty of confusion already with hydration mismatches; IMO we shouldn't be adding on to them (as well as breaking existing functionality unnecessarily). /cc @divmain

(I agree that numbers as classes is rare BTW. But I still don't see a reason to break it.)

OK wait, I see that .123 is not a valid CSS selector. So maybe there is no point in rendering 123 as anything but "", yeah.

After some thought, I think we should just fix the hydration mismatch issue. A developer may want to have an "invalid" value here, e.g. a value that is either "foo" or false (e.g. someCondition && "foo"). The framework should be smart enough to recognize that the server rendered the correct thing if the value is false`.

ekashida · 2024-01-18T22:06:05Z

...es/@lwc/integration-karma/test-hydration/mismatches/type-coercion-to-string/x/main/main.html

@@ -1,4 +1,4 @@
 <template>
    <!-- style attr is validated at compile time -->
-    <p class={num} data-attr={num}>{num}</p>


@wjhsf If possible, I would prefer to avoid making assumptions about what the component author intended.

packages/@lwc/integration-karma/test/template/attribute-class/object-values.spec.js

ravijayaramappa · 2024-01-23T17:25:55Z

packages/@lwc/engine-core/src/framework/api.ts

+        for (let i = 0; i < value.length; i++) {
+            const normalized = ncls(value[i]);
+            if (normalized) {
+                res += normalized + ' ';
+            }
+        }


Suggested change

for (let i = 0; i < value.length; i++) {

const normalized = ncls(value[i]);

if (normalized) {

res += normalized + ' ';

}

}

res += ArrayMap.call(value, ncls).filter(Boolean).join(' ');

I don't have a strong opinion on this @ravijayaramappa. At this point, it is unclear to me whether or not this approach is more performant (new array allocation + extra invocations in the filter). The only thing I can say is that it is less readable than the current implementation.

@nolanlawson Any additional thoughts on this?

I don’t have a strong opinion either. My suggestion was to make the code concise with native Array APIs. I am fine either way 🙂

Add me to the list of people without a strong opinion on this, but I will say that both versions are equally readable for me 😆

Suggested change

for (let i = 0; i < value.length; i++) {

const normalized = ncls(value[i]);

if (normalized) {

res += normalized + ' ';

}

}

res += ArrayReduce.call(value, (acc, item) => {

const normalized = ncls(item);

return normalized ? `${acc} ${normalized}` : acc;

}, '')

Since there are so many strong opinions, here's a third option! Should be more performant than map/filter/join, but I don't know how it compares to the for loop. It's probably the least readable, though.

A for loop is always fastest. In fact we recently refactored an Array.prototype.reduce to a for loop to get some perf gains: #3729

Normally I would not care, but this code is in the hot path, so we should prefer the for loop.

pmdartus · 2024-01-24T08:10:30Z

I am not an "official LWC maintainer" anymore, I don't want to merge it myself.
Please merge this PR and release it at your convenience.

wjhsf · 2024-01-24T16:40:34Z

scripts/bundlesize/bundlesize.config.json

@@ -2,7 +2,7 @@
    "files": [
        {
            "path": "packages/@lwc/engine-dom/dist/index.js",
-            "maxSize": "22KB"
+            "maxSize": "22.10KB"


Not super important, but I wonder if any of the suggested implementations for ncls would avoid the need for this size bump.

@wjhsf This test really exists to catch unintended size increases, so this increase isn't an issue.

That's why I prefaced with "not super important" 😜

packages/@lwc/integration-karma/test/template/attribute-class/object-values.spec.js

nolanlawson · 2024-01-25T23:29:11Z

packages/@lwc/integration-karma/test/template/attribute-class/object-values.spec.js

+    testClassNameValue('false', false, isObjectBindingEnabled ? '' : 'false');
+    testClassNameValue('null', null, isObjectBindingEnabled ? '' : '');
+    testClassNameValue('undefined', undefined, isObjectBindingEnabled ? '' : '');
+    testClassNameValue('number', 1, isObjectBindingEnabled ? '' : '1');


I'd like to see 0 and NaN here as well for completeness, since they are falsy.

nolanlawson

I have some remaining minor nitpicks about tests, but overall this LGTM.

I think we should do this as part of 7.0.0 with API versioning to 1) avoid breaking changes, and 2) to offer a carrot for API versioning upgrades.

…thand

nolanlawson · 2024-04-16T22:54:56Z

Merged with master. There was also a conflict with the recent static content optimization work (#4071), but I resolved those in 8c32821.

…thand

pmdartus added 7 commits January 18, 2024 07:43

feat: add sample code

a4a8cd4

test: add broken reative tests

474e4a7

chore: revert change to engine

cc0a4da

feat: move logic to compiler

68a394d

feat: add API versioning

32c71f3

chore: use repo call convention

76e7119

test: only enable Karma test when API_VERSION >= 61

cde9608

pmdartus requested a review from a team as a code owner January 18, 2024 07:22

pmdartus added 2 commits January 18, 2024 08:26

chore: increase engine bundlesize

81f3c73

test: remove type coersion test from the hydration suite

ebe42a8

pmdartus commented Jan 18, 2024

View reviewed changes

ekashida requested changes Jan 18, 2024

View reviewed changes

ekashida reviewed Jan 18, 2024

View reviewed changes

packages/@lwc/integration-karma/test/template/attribute-class/object-values.spec.js Outdated Show resolved Hide resolved

ravijayaramappa reviewed Jan 23, 2024

View reviewed changes

pmdartus added 2 commits January 24, 2024 06:13

test: type coersion

2de363b

test: reapeated class names

9e1b2e7

pmdartus requested a review from ekashida January 24, 2024 05:19

ravijayaramappa approved these changes Jan 24, 2024

View reviewed changes

ekashida approved these changes Jan 24, 2024

View reviewed changes

wjhsf reviewed Jan 24, 2024

View reviewed changes

nolanlawson reviewed Jan 25, 2024

View reviewed changes

packages/@lwc/integration-karma/test/template/attribute-class/object-values.spec.js Outdated Show resolved Hide resolved

nolanlawson reviewed Jan 25, 2024

View reviewed changes

nolanlawson added the nomerge label Feb 16, 2024

nolanlawson added this to the 7.0.0 milestone Feb 16, 2024

nolanlawson approved these changes Feb 16, 2024

View reviewed changes

nolanlawson added 2 commits April 16, 2024 15:45

Merge remote-tracking branch 'origin/master' into pmdartus/class-shor…

42ab135

…thand

fix: fix static content optimization

8c32821

Merge remote-tracking branch 'origin/master' into pmdartus/class-shor…

5d7395d

…thand

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Template class object binding #3950

feat: Template class object binding #3950

pmdartus commented Jan 18, 2024

pmdartus Jan 18, 2024

wjhsf Jan 18, 2024

ekashida Jan 18, 2024

pmdartus Jan 24, 2024

nolanlawson Jan 25, 2024

nolanlawson Jan 25, 2024

nolanlawson Jan 25, 2024

ekashida Jan 18, 2024

ravijayaramappa Jan 23, 2024

pmdartus Jan 24, 2024

ravijayaramappa Jan 24, 2024

ekashida Jan 24, 2024

wjhsf Jan 24, 2024

nolanlawson Jan 25, 2024

pmdartus commented Jan 24, 2024

wjhsf Jan 24, 2024

ekashida Jan 24, 2024

wjhsf Jan 25, 2024

nolanlawson Jan 25, 2024

nolanlawson left a comment

nolanlawson commented Apr 16, 2024

feat: Template class object binding #3950

Are you sure you want to change the base?

feat: Template class object binding #3950

Conversation

pmdartus commented Jan 18, 2024

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

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

pmdartus commented Jan 24, 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

nolanlawson left a comment

Choose a reason for hiding this comment

nolanlawson commented Apr 16, 2024