[labs/forms] WIP: Initial implementation #4773

justinfagnani · 2024-09-21T21:40:10Z

This a rough sketch of a no-public-API FormAssociated mixin.

changeset-bot · 2024-09-21T21:40:14Z

🦋 Changeset detected

Latest commit: b2b88a5

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

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

github-actions · 2024-09-21T21:40:24Z

📊 Tachometer Benchmark Results

Summary

nop-update

this-change, tip-of-tree, previous-release: unsure 🔍 -19% - +6% (-2.77ms - +0.93ms)
^{this-change vs tip-of-tree}

render

this-change: 48.77ms - 61.59ms
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +9% (-0.49ms - +1.70ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-0.60ms - +0.60ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -7% - +56% (-1.95ms - +26.48ms)
^{this-change vs tip-of-tree}

update

this-change: 506.03ms - 513.88ms
this-change, tip-of-tree, previous-release: unsure 🔍 -11% - +6% (-5.18ms - +3.01ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +3% (-2.31ms - +2.53ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +1% (-9.47ms - +3.27ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 510.39ms - 519.98ms
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-4.89ms - +5.10ms)
^{this-change vs tip-of-tree}

Results

this-change

Browser: chrome-headless 128.0.0.0
Sample size: 50
Built by: Benchmarks #6420
Commit: b2b88a5

render

Version	Avg time	vs
	48.77ms - 61.59ms	-

update

Version	Avg time	vs
	506.03ms - 513.88ms	-

update-reflect

Version	Avg time	vs
	510.39ms - 519.98ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 128.0.0.0
Sample size: 30
Built by: Benchmarks #6420
Commit: b2b88a5

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	19.79ms - 21.41ms	-	unsure 🔍 -3% - +9% -0.49ms - +1.70ms	unsure 🔍 -2% - +8% -0.33ms - +1.59ms
tip-of-tree tip-of-tree	19.26ms - 20.73ms	unsure 🔍 -8% - +2% -1.70ms - +0.49ms	-	unsure 🔍 -4% - +5% -0.88ms - +0.92ms
previous-release previous-release	19.46ms - 20.49ms	unsure 🔍 -8% - +2% -1.59ms - +0.33ms	unsure 🔍 -5% - +4% -0.92ms - +0.88ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	42.14ms - 47.02ms	-	unsure 🔍 -11% - +6% -5.18ms - +3.01ms	unsure 🔍 -7% - +11% -3.22ms - +4.64ms
tip-of-tree tip-of-tree	42.38ms - 48.95ms	unsure 🔍 -7% - +12% -3.01ms - +5.18ms	-	unsure 🔍 -6% - +15% -2.71ms - +6.30ms
previous-release previous-release	40.79ms - 46.95ms	unsure 🔍 -10% - +7% -4.64ms - +3.22ms	unsure 🔍 -14% - +6% -6.30ms - +2.71ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	12.30ms - 13.82ms	-	unsure 🔍 -19% - +6% -2.77ms - +0.93ms	unsure 🔍 -7% - +7% -0.97ms - +0.97ms
tip-of-tree tip-of-tree	12.29ms - 15.67ms	unsure 🔍 -7% - +21% -0.93ms - +2.77ms	-	unsure 🔍 -7% - +21% -0.87ms - +2.72ms
previous-release previous-release	12.45ms - 13.66ms	unsure 🔍 -7% - +7% -0.97ms - +0.97ms	unsure 🔍 -19% - +5% -2.72ms - +0.87ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 128.0.0.0
Sample size: 50
Built by: Benchmarks #6420
Commit: b2b88a5

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	36.18ms - 37.04ms	-	unsure 🔍 -2% - +2% -0.60ms - +0.60ms	unsure 🔍 -3% - +1% -1.19ms - +0.23ms
tip-of-tree tip-of-tree	36.19ms - 37.03ms	unsure 🔍 -2% - +2% -0.60ms - +0.60ms	-	unsure 🔍 -3% - +1% -1.19ms - +0.23ms
previous-release previous-release	36.52ms - 37.66ms	unsure 🔍 -1% - +3% -0.23ms - +1.19ms	unsure 🔍 -1% - +3% -0.23ms - +1.19ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	79.97ms - 83.51ms	-	unsure 🔍 -3% - +3% -2.31ms - +2.53ms	unsure 🔍 -2% - +3% -2.00ms - +2.72ms
tip-of-tree tip-of-tree	79.99ms - 83.28ms	unsure 🔍 -3% - +3% -2.53ms - +2.31ms	-	unsure 🔍 -2% - +3% -2.02ms - +2.52ms
previous-release previous-release	79.82ms - 82.95ms	unsure 🔍 -3% - +2% -2.72ms - +2.00ms	unsure 🔍 -3% - +2% -2.52ms - +2.02ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 128.0.0.0
Sample size: 50
Built by: Benchmarks #6420
Commit: b2b88a5

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	51.02ms - 72.75ms	-	unsure 🔍 -7% - +56% -1.95ms - +26.48ms	unsure 🔍 -28% - +18% -18.89ms - +12.25ms
tip-of-tree tip-of-tree	40.46ms - 58.78ms	unsure 🔍 -40% - +1% -26.48ms - +1.95ms	-	faster ✔ 5% - 43% 1.15ms - 30.02ms
previous-release previous-release	54.05ms - 76.36ms	unsure 🔍 -20% - +31% -12.25ms - +18.89ms	unsure 🔍 -2% - +64% +1.15ms - +30.02ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	513.47ms - 522.90ms	-	unsure 🔍 -2% - +1% -9.47ms - +3.27ms	unsure 🔍 -1% - +2% -6.50ms - +10.29ms
tip-of-tree tip-of-tree	517.00ms - 525.57ms	unsure 🔍 -1% - +2% -3.27ms - +9.47ms	-	unsure 🔍 -1% - +3% -3.17ms - +13.16ms
previous-release previous-release	509.34ms - 523.24ms	unsure 🔍 -2% - +1% -10.29ms - +6.50ms	unsure 🔍 -3% - +1% -13.16ms - +3.17ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	552.02ms - 559.44ms	-	unsure 🔍 -1% - +1% -4.89ms - +5.10ms	unsure 🔍 -1% - +1% -4.59ms - +6.50ms
tip-of-tree tip-of-tree	552.28ms - 558.97ms	unsure 🔍 -1% - +1% -5.10ms - +4.89ms	-	unsure 🔍 -1% - +1% -4.46ms - +6.16ms
previous-release previous-release	550.66ms - 558.90ms	unsure 🔍 -1% - +1% -6.50ms - +4.59ms	unsure 🔍 -1% - +1% -6.16ms - +4.46ms	-

_{tachometer-reporter-action v2 for Benchmarks}

github-actions · 2024-09-21T21:42:09Z

The size of lit-html.js and lit-core.min.js are as expected.

packages/labs/forms/src/form-associated.ts

sorvell · 2024-09-22T14:41:19Z

packages/labs/forms/src/form-associated.ts

+        target.set.call(this, value);
+        getInternals(this)!.setFormValue(value);
+      },
+      init(value: V) {


So this requires using standard decorators. Think that's ok?

Just sketching things out for now and making sure they compile. I think we'll need to support experimental decorators though, and figure out how to handle the default value. Maybe we try out the pattern we're discussion for @property() here first.

If we need user-configurable default values, then we'll have to have a separate decorator for defaults and won't need to rely on the initializer anymore.

packages/labs/forms/src/form-associated.ts

43081j · 2024-10-06T03:13:18Z

packages/labs/forms/README.md

+
+To enable this two decorators are provided to read and write form state:
+`@formStateGetter()` and `@formStateSetter()`. They are applied to a getter and
+setter to mark them as beind use to readn adn write state. When present,


Suggested change

setter to mark them as beind use to readn adn write state. When present,

setter to mark them as being used to read and write state. When present,

I'm guessing this is what was meant

kyubisation

Nice work! I generally like this approach. 👍

kyubisation · 2024-10-10T00:13:26Z

packages/labs/forms/README.md

+  internals. `getInternals()` requires the element class as a parameter and is
+  guarded against multiple calls similar to `attachInternals()`.
+
+- `isDisabled()`: An element can be disabled because ti's fieldset is disabled,


Suggested change

- `isDisabled()`: An element can be disabled because ti's fieldset is disabled,

- `isDisabled()`: An element can be disabled because its fieldset is disabled,

kyubisation · 2024-10-10T13:20:32Z

packages/labs/forms/src/form-associated.ts

+  /**
+   * Implement this method to validate the element. Return a `ValidityResult`.
+   */
+  _getValidity?(): ValidityResult;


Code style question; Has this pattern (public properties/methods starting with underscore) been used previously? If not, would protected with a declared class be an option?

Another option is using symbols, which has a few benefits in terms of "hiding" the method from outsiders and making it explicit for implementers:

export const getValidity = Symbol("getValidity"); export interface FormAssociated extends ReactiveElement { [getValidity]?(): ValidityResult; }

kyubisation · 2024-10-10T13:28:41Z

packages/labs/forms/src/form-associated.ts

+ * Returns true if the element is disabled.
+ */
+export const isDisabled = (element: FormAssociated) =>
+  element.matches(':disabled');


This would probably break in SSR. I assume that is acceptable?

kyubisation · 2024-10-10T13:39:13Z

packages/labs/forms/src/form-associated.ts

+interface ValidityResult {
+  flags: Partial<Omit<ValidityState, 'valid'>>;
+  message?: string;
+  anchor?: HTMLElement;


I believe this breaks in webkit, if anchor is omitted (or is the element itself, from what I remember).
whatwg/html#10155
https://bugs.webkit.org/show_bug.cgi?id=269832

This is apparently only a problem, if setValidity is called during constructor phase.

theacodes · 2024-10-28T15:35:44Z

packages/labs/forms/README.md

+- `validationMessage`: a readonly property that returns the error message that
+  would be shown to the user if the element was to be checked for validity.
+- `willValidate`: a readonly property that returns true if internals's target
+  element will be validated when the form is submitted. For example, disbabled


Suggested change

element will be validated when the form is submitted. For example, disbabled

element will be validated when the form is submitted. For example, disabled

[labs/forms] WIP: Initial implementation

b995e19

Add some comments

56317de

sorvell reviewed Sep 22, 2024

View reviewed changes

justinfagnani added 11 commits September 22, 2024 11:50

Add some FormAssociated tests

7395d93

Add tests. Protect getInternals() against bad calls.

5d838e4

Add FormControl mixin

ee2a0ea

Add _getValidity() and _validate() methods

da0a53f

Update ignore files

5aa562b

Add @formValue() options and form value converter

a51a3c1

Revert WTR config

a52fbe4

Organize source into foldable regions

5d19e93

Update, test, and document formState decorators

b430022

Add @formDefaultValue() and @formDefaultState()

f06c783

Add disabled test for FormControl

b2b88a5

43081j reviewed Oct 6, 2024

View reviewed changes

43081j approved these changes Oct 6, 2024

View reviewed changes

kyubisation reviewed Oct 10, 2024

View reviewed changes

theacodes reviewed Oct 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[labs/forms] WIP: Initial implementation #4773

[labs/forms] WIP: Initial implementation #4773

justinfagnani commented Sep 21, 2024

changeset-bot bot commented Sep 21, 2024 •

edited

Loading

github-actions bot commented Sep 21, 2024 •

edited

Loading

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

update-reflect

github-actions bot commented Sep 21, 2024

sorvell Sep 22, 2024

justinfagnani Sep 22, 2024

justinfagnani Sep 23, 2024

43081j Oct 6, 2024

kyubisation left a comment

kyubisation Oct 10, 2024

kyubisation Oct 10, 2024

theacodes Oct 28, 2024

kyubisation Oct 10, 2024

kyubisation Oct 10, 2024

kyubisation Oct 18, 2024

theacodes Oct 28, 2024

	setter to mark them as beind use to readn adn write state. When present,
	setter to mark them as being used to read and write state. When present,

	- `isDisabled()`: An element can be disabled because ti's fieldset is disabled,
	- `isDisabled()`: An element can be disabled because its fieldset is disabled,

	element will be validated when the form is submitted. For example, disbabled
	element will be validated when the form is submitted. For example, disabled

[labs/forms] WIP: Initial implementation #4773

Are you sure you want to change the base?

[labs/forms] WIP: Initial implementation #4773

Conversation

justinfagnani commented Sep 21, 2024

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

🦋 Changeset detected

github-actions bot commented Sep 21, 2024 • edited Loading

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

update-reflect

github-actions bot commented Sep 21, 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

kyubisation 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

changeset-bot bot commented Sep 21, 2024 •

edited

Loading

github-actions bot commented Sep 21, 2024 •

edited

Loading