(restructure) - controlled components #3251

JoviDeCroock · 2021-08-23T16:30:26Z

This is a bit of a new feature a big nuisance in Preact X was that when people had components like this:

import { useState } from 'preact/hooks';

const Input = () => {
  const [value, setValue} = useState('');
  const onInput = e => {
    if (e.target.value.length > 3) return;
    setValue(e.target.value);
  }
  return <input value={value} onInput={onInput} />
}

This would transition to an uncontrolled component as the underlying dom-element would update to the new character but there was no trigger to the render cycle for this meaning that the control expressed in this event-handler wasn't respected.

github-actions · 2021-08-23T16:30:42Z

📊 Tachometer Benchmark Results

Summary

duration

02_replace1k: unsure 🔍 -2% - +3% (-2.71ms - +5.25ms)
^{preact-local vs preact-master}
03_update10th1k_x16: unsure 🔍 -1% - +3% (-0.77ms - +2.61ms)
^{preact-local vs preact-master}
07_create10k: unsure 🔍 -3% - +1% (-41.10ms - +8.81ms)
^{preact-local vs preact-master}
filter_list: unsure 🔍 -1% - +3% (-2.12ms - +5.89ms)
^{preact-local vs preact-master}
hydrate1k: unsure 🔍 -2% - +2% (-1.84ms - +1.84ms)
^{preact-local vs preact-master}
many_updates: unsure 🔍 -2% - +2% (-2.02ms - +2.41ms)
^{preact-local vs preact-master}
text_update: unsure 🔍 -4% - +4% (-2.54ms - +2.68ms)
^{preact-local vs preact-master}
todo: unsure 🔍 -2% - +2% (-1.39ms - +1.10ms)
^{preact-local vs preact-master}

usedJSHeapSize

02_replace1k: unsure 🔍 -0% - +0% (-0.00ms - +0.01ms)
^{preact-local vs preact-master}
03_update10th1k_x16: unsure 🔍 -0% - +0% (-0.01ms - +0.00ms)
^{preact-local vs preact-master}
07_create10k: unsure 🔍 +0% - +0% (+0.00ms - +0.00ms)
^{preact-local vs preact-master}
filter_list: unsure 🔍 -1% - +1% (-0.02ms - +0.02ms)
^{preact-local vs preact-master}
hydrate1k: unsure 🔍 -0% - +0% (-0.01ms - +0.00ms)
^{preact-local vs preact-master}
many_updates: unsure 🔍 -0% - +0% (-0.01ms - +0.00ms)
^{preact-local vs preact-master}
text_update: unsure 🔍 -2% - +3% (-0.02ms - +0.03ms)
^{preact-local vs preact-master}
todo: unsure 🔍 -1% - +4% (-0.01ms - +0.04ms)
^{preact-local vs preact-master}

Results

02_replace1k

Browser: chrome-headless 92.0.4515.131
Sample size: 80
Built by: Benchmarks #448
Commit: 4720d97

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	154.81ms - 160.37ms	-	unsure 🔍 -3% - +2% -5.25ms - +2.71ms
preact-local	156.02ms - 161.70ms	unsure 🔍 -2% - +3% -2.71ms - +5.25ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	3.75ms - 3.76ms	-	unsure 🔍 -0% - +0% -0.01ms - +0.00ms
preact-local	3.76ms - 3.77ms	unsure 🔍 -0% - +0% -0.00ms - +0.01ms	-

run-warmup-0

Version	Avg time	vs preact-master	vs preact-local
preact-master	72.39ms - 75.82ms	-	unsure 🔍 -3% - +3% -2.31ms - +2.48ms
preact-local	72.35ms - 75.69ms	unsure 🔍 -3% - +3% -2.48ms - +2.31ms	-

run-warmup-1

Version	Avg time	vs preact-master	vs preact-local
preact-master	97.98ms - 104.38ms	-	unsure 🔍 -5% - +4% -5.52ms - +3.78ms
preact-local	98.68ms - 105.43ms	unsure 🔍 -4% - +5% -3.78ms - +5.52ms	-

run-warmup-2

Version	Avg time	vs preact-master	vs preact-local
preact-master	83.58ms - 87.25ms	-	unsure 🔍 -3% - +3% -2.32ms - +2.58ms
preact-local	83.65ms - 86.91ms	unsure 🔍 -3% - +3% -2.58ms - +2.32ms	-

run-warmup-3

Version	Avg time	vs preact-master	vs preact-local
preact-master	67.76ms - 72.03ms	-	unsure 🔍 -5% - +4% -3.83ms - +2.53ms
preact-local	68.19ms - 72.91ms	unsure 🔍 -4% - +5% -2.53ms - +3.83ms	-

run-warmup-4

Version	Avg time	vs preact-master	vs preact-local
preact-master	84.14ms - 91.45ms	-	unsure 🔍 -7% - +4% -5.90ms - +3.56ms
preact-local	85.96ms - 91.97ms	unsure 🔍 -4% - +7% -3.56ms - +5.90ms	-

run-final

Version	Avg time	vs preact-master	vs preact-local
preact-master	154.84ms - 160.40ms	-	unsure 🔍 -3% - +2% -5.25ms - +2.70ms
preact-local	156.05ms - 161.74ms	unsure 🔍 -2% - +3% -2.70ms - +5.25ms	-

03_update10th1k_x16

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #448
Commit: 4720d97

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	87.92ms - 90.24ms	-	unsure 🔍 -3% - +1% -2.61ms - +0.77ms
preact-local	88.77ms - 91.23ms	unsure 🔍 -1% - +3% -0.77ms - +2.61ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	3.74ms - 3.74ms	-	unsure 🔍 -0% - +0% -0.00ms - +0.01ms
preact-local	3.73ms - 3.74ms	unsure 🔍 -0% - +0% -0.01ms - +0.00ms	-

07_create10k

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #448
Commit: 4720d97

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	1606.24ms - 1639.36ms	-	unsure 🔍 -1% - +3% -8.81ms - +41.10ms
preact-local	1588.00ms - 1625.32ms	unsure 🔍 -3% - +1% -41.10ms - +8.81ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	27.30ms - 27.30ms	-	unsure 🔍 -0% - -0% -0.00ms - -0.00ms
preact-local	27.30ms - 27.30ms	unsure 🔍 +0% - +0% +0.00ms - +0.00ms	-

filter_list

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #448
Commit: 4720d97

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	221.32ms - 227.14ms	-	unsure 🔍 -3% - +1% -5.89ms - +2.12ms
preact-local	223.37ms - 228.87ms	unsure 🔍 -1% - +3% -2.12ms - +5.89ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	1.77ms - 1.80ms	-	unsure 🔍 -1% - +1% -0.02ms - +0.02ms
preact-local	1.77ms - 1.80ms	unsure 🔍 -1% - +1% -0.02ms - +0.02ms	-

hydrate1k

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #448
Commit: 4720d97

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	73.38ms - 76.08ms	-	unsure 🔍 -2% - +2% -1.84ms - +1.84ms
preact-local	73.48ms - 75.98ms	unsure 🔍 -2% - +2% -1.84ms - +1.84ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	6.32ms - 6.33ms	-	unsure 🔍 -0% - +0% -0.00ms - +0.01ms
preact-local	6.32ms - 6.33ms	unsure 🔍 -0% - +0% -0.01ms - +0.00ms	-

many_updates

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #448
Commit: 4720d97

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	127.87ms - 131.46ms	-	unsure 🔍 -2% - +2% -2.41ms - +2.02ms
preact-local	128.56ms - 131.15ms	unsure 🔍 -2% - +2% -2.02ms - +2.41ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	5.37ms - 5.37ms	-	unsure 🔍 -0% - +0% -0.00ms - +0.01ms
preact-local	5.36ms - 5.38ms	unsure 🔍 -0% - +0% -0.01ms - +0.00ms	-

text_update

Browser: chrome-headless 92.0.4515.131
Sample size: 60
Built by: Benchmarks #448
Commit: 4720d97

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	66.53ms - 70.57ms	-	unsure 🔍 -4% - +4% -2.68ms - +2.54ms
preact-local	66.98ms - 70.26ms	unsure 🔍 -4% - +4% -2.54ms - +2.68ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	0.95ms - 0.98ms	-	unsure 🔍 -3% - +2% -0.03ms - +0.02ms
preact-local	0.95ms - 0.98ms	unsure 🔍 -2% - +3% -0.02ms - +0.03ms	-

todo

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #448
Commit: 4720d97

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	64.84ms - 66.55ms	-	unsure 🔍 -2% - +2% -1.10ms - +1.39ms
preact-local	64.65ms - 66.46ms	unsure 🔍 -2% - +2% -1.39ms - +1.10ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	1.02ms - 1.06ms	-	unsure 🔍 -4% - +1% -0.04ms - +0.01ms
preact-local	1.04ms - 1.08ms	unsure 🔍 -1% - +4% -0.01ms - +0.04ms	-

_{tachometer-reporter-action v2 for Benchmarks}

coveralls · 2021-08-23T16:32:24Z

Coverage remained the same at 95.956% when pulling 4720d97 on restructure-controlled-components-2 into 5e8a3f9 on restructure.

github-actions · 2021-08-23T16:32:33Z

Size Change: +398 B (1%)

Total Size: 32.1 kB

Filename	Size	Change
`dist/preact.js`	4.59 kB	+134 B (2%)
`dist/preact.min.js`	4.63 kB	+134 B (2%)
`dist/preact.umd.js`	4.68 kB	+130 B (2%)

ℹ️ View Unchanged

Filename	Size	Change
`compat/dist/compat.js`	3.52 kB	0 B
`compat/dist/compat.umd.js`	3.59 kB	0 B
`debug/dist/debug.js`	3.2 kB	0 B
`debug/dist/debug.umd.js`	3.3 kB	0 B
`devtools/dist/devtools.js`	232 B	0 B
`devtools/dist/devtools.umd.js`	316 B	0 B
`hooks/dist/hooks.js`	1.15 kB	0 B
`hooks/dist/hooks.umd.js`	1.23 kB	0 B
`jsx-runtime/dist/jsxRuntime.js`	342 B	0 B
`jsx-runtime/dist/jsxRuntime.umd.js`	425 B	0 B
`test-utils/dist/testUtils.js`	431 B	0 B
`test-utils/dist/testUtils.umd.js`	516 B	0 B

_{compressed-size-action}

developit · 2021-08-23T16:39:48Z

Question as I peek through here: does this require that a controlled input have an onInput handler in order to run patch() on every keystroke/change? Wondering if that gets blocked by shouldComponentUpdate and reverts back to uncontrolled.

JoviDeCroock · 2021-08-23T16:42:09Z

@developit I'm not entirely sure I understand the question, controlled components as it stands require both a value and a notion of a changeHandler here being onInput if it bails out of an update due to return; it restores the previous value as that means a bailout. This never enters the diff so it's restored to the current stateful value.

This in no way meddles with our current uncontrolled components it's only a fix for our bug in controlled components when we bail out of updates due to the above example

developit · 2021-08-23T18:06:23Z

Heh - sorry, I typed that out and didn't realize how vague it ended up being 😅

I was under the impression the following would be considered "controlled", and would not permit any user modification of the input:

function X() {
  return <input value="foo" />
}

You're saying the change handler is actually required in order to flip the input into controlled mode?

src/diff/mount.js

src/diff/patch.js

src/diff/props.js

marvinhagemeister · 2021-08-24T07:07:30Z

@JoviDeCroock This looks great! 🙌

I was under the impression the following would be considered "controlled", and would not permit any user modification of the input:
function X() {
  return <input value="foo" />
}

That's a good case we should cover in our tests 👍

JoviDeCroock · 2021-08-24T08:25:44Z

@marvinhagemeister Yeah the test covers that case (in how far we can do that in JSDom) 😅

marvinhagemeister

LGMT 👍

EmmanuelOga · 2024-07-16T17:01:10Z

Note: Reverted on #4008

control components through the vdom props

419aaf2

JoviDeCroock added the Restructure label Aug 23, 2021

JoviDeCroock requested review from developit, andrewiggins and marvinhagemeister August 23, 2021 16:30

JoviDeCroock commented Aug 23, 2021

View reviewed changes

src/diff/mount.js Outdated Show resolved Hide resolved

JoviDeCroock commented Aug 23, 2021

View reviewed changes

src/diff/patch.js Outdated Show resolved Hide resolved

JoviDeCroock commented Aug 23, 2021

View reviewed changes

src/diff/props.js Outdated Show resolved Hide resolved

JoviDeCroock added 5 commits August 24, 2021 14:50

golf

d613bf1

support textarea and select as well

0420e3f

only allow for controlled components when we have onChange or onInput

a8d7f44

undo lockfile change

6e93831

add a few tests

5505dcc

marvinhagemeister approved these changes Aug 30, 2021

View reviewed changes

Merge branch 'restructure' into restructure-controlled-components-2

4720d97

JoviDeCroock merged commit da2e759 into restructure Aug 30, 2021

JoviDeCroock deleted the restructure-controlled-components-2 branch August 30, 2021 15:33

(restructure) - controlled components #3251

(restructure) - controlled components #3251

Conversation

JoviDeCroock commented Aug 23, 2021 • edited Loading

github-actions bot commented Aug 23, 2021 • edited Loading

📊 Tachometer Benchmark Results

Summary

duration

usedJSHeapSize

Results

duration

usedJSHeapSize

run-warmup-0

run-warmup-1

run-warmup-2

run-warmup-3

run-warmup-4

run-final

duration

usedJSHeapSize

duration

usedJSHeapSize

duration

usedJSHeapSize

duration

usedJSHeapSize

duration

usedJSHeapSize

duration

usedJSHeapSize

duration

usedJSHeapSize

coveralls commented Aug 23, 2021 • edited Loading

github-actions bot commented Aug 23, 2021 • edited Loading

developit commented Aug 23, 2021

JoviDeCroock commented Aug 23, 2021 • edited Loading

developit commented Aug 23, 2021

marvinhagemeister commented Aug 24, 2021

JoviDeCroock commented Aug 24, 2021

marvinhagemeister left a comment

Choose a reason for hiding this comment

EmmanuelOga commented Jul 16, 2024

JoviDeCroock commented Aug 23, 2021 •

edited

Loading

github-actions bot commented Aug 23, 2021 •

edited

Loading

coveralls commented Aug 23, 2021 •

edited

Loading

github-actions bot commented Aug 23, 2021 •

edited

Loading

JoviDeCroock commented Aug 23, 2021 •

edited

Loading