preact Ignore undefined children in createElement 3rd arg

Ignore undefined children in createElement 3rd arg

Open developit opened this issue 3 years ago • 6 comments

I believe #3091 may have introduced a bug where h('div', { children: 'foo' }, undefined) would produce { children: undefined }. This fixes that bug, if it is a bug.

I've left this as a draft because I actually don't know which is the more desirable behavior here. From an implementation standpoint, I actually think the arguments.length > 2 approach is best, because it allows us to seamlessly transition to rest parameters since the semantics are then the same:

function createElement(type, props, ...children) {
  if (children.length !== 0) props.children = children;
}

Apr 15 '21 20:04 developit

📊 Tachometer Benchmark Results

Summary

duration

02_replace1k: unsure 🔍 -1% - +2% (-1.33ms - +2.53ms)
^{preact-local vs preact-master}
03_update10th1k_x16: unsure 🔍 -5% - +5% (-1.69ms - +1.58ms)
^{preact-local vs preact-master}
07_create10k: unsure 🔍 -1% - +0% (-9.18ms - +7.82ms)
^{preact-local vs preact-master}
filter_list: unsure 🔍 -5% - +1% (-1.56ms - +0.49ms)
^{preact-local vs preact-master}
hydrate1k: unsure 🔍 -2% - +1% (-2.62ms - +0.90ms)
^{preact-local vs preact-master}
many_updates: unsure 🔍 -5% - +7% (-1.72ms - +2.64ms)
^{preact-local vs preact-master}
text_update: unsure 🔍 -8% - +3% (-0.29ms - +0.10ms)
^{preact-local vs preact-master}

usedJSHeapSize

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

Results

02_replace1k

Browser: chrome-headless 89.0.4389.114
Sample size: 80
Built by: Benchmarks #215
Commit: 5b2dc48

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	157.57ms - 159.81ms	-	unsure 🔍 -2% - +1% -2.53ms - +1.33ms
preact-local	157.72ms - 160.86ms	unsure 🔍 -1% - +2% -1.33ms - +2.53ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	3.58ms - 3.59ms	-	unsure 🔍 -0% - +0% -0.00ms - +0.01ms
preact-local	3.57ms - 3.58ms	unsure 🔍 -0% - +0% -0.01ms - +0.00ms	-

run-warmup-0

Version	Avg time	vs preact-master	vs preact-local
preact-master	57.80ms - 58.46ms	-	unsure 🔍 -1% - +1% -0.44ms - +0.59ms
preact-local	57.66ms - 58.45ms	unsure 🔍 -1% - +1% -0.59ms - +0.44ms	-

run-warmup-1

Version	Avg time	vs preact-master	vs preact-local
preact-master	90.79ms - 91.56ms	-	unsure 🔍 -1% - +1% -0.82ms - +0.48ms
preact-local	90.82ms - 91.87ms	unsure 🔍 -1% - +1% -0.48ms - +0.82ms	-

run-warmup-2

Version	Avg time	vs preact-master	vs preact-local
preact-master	98.03ms - 103.83ms	-	unsure 🔍 -3% - +6% -2.69ms - +6.14ms
preact-local	95.87ms - 102.54ms	unsure 🔍 -6% - +3% -6.14ms - +2.69ms	-

run-warmup-3

Version	Avg time	vs preact-master	vs preact-local
preact-master	59.43ms - 63.57ms	-	unsure 🔍 -7% - +3% -4.35ms - +2.01ms
preact-local	60.26ms - 65.09ms	unsure 🔍 -3% - +7% -2.01ms - +4.35ms	-

run-warmup-4

Version	Avg time	vs preact-master	vs preact-local
preact-master	94.16ms - 99.55ms	-	unsure 🔍 -4% - +3% -4.35ms - +3.11ms
preact-local	94.90ms - 100.06ms	unsure 🔍 -3% - +5% -3.11ms - +4.35ms	-

run-final

Version	Avg time	vs preact-master	vs preact-local
preact-master	58.20ms - 59.90ms	-	unsure 🔍 -3% - +2% -1.71ms - +0.91ms
preact-local	58.46ms - 60.44ms	unsure 🔍 -2% - +3% -0.91ms - +1.71ms	-

03_update10th1k_x16

Browser: chrome-headless 89.0.4389.114
Sample size: 60
Built by: Benchmarks #215
Commit: 5b2dc48

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	33.06ms - 35.62ms	-	unsure 🔍 -5% - +5% -1.58ms - +1.69ms
preact-local	33.28ms - 35.30ms	unsure 🔍 -5% - +5% -1.69ms - +1.58ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	3.63ms - 3.65ms	-	unsure 🔍 -0% - +0% -0.01ms - +0.02ms
preact-local	3.63ms - 3.65ms	unsure 🔍 -0% - +0% -0.02ms - +0.01ms	-

07_create10k

Browser: chrome-headless 89.0.4389.114
Sample size: 50
Built by: Benchmarks #215
Commit: 5b2dc48

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	1647.48ms - 1659.56ms	-	unsure 🔍 -0% - +1% -7.82ms - +9.18ms
preact-local	1646.87ms - 1658.82ms	unsure 🔍 -1% - +0% -9.18ms - +7.82ms	-

usedJSHeapSize

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

filter_list

Browser: chrome-headless 89.0.4389.114
Sample size: 70
Built by: Benchmarks #215
Commit: 5b2dc48

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	32.30ms - 34.18ms	-	unsure 🔍 -1% - +5% -0.49ms - +1.56ms
preact-local	32.31ms - 33.11ms	unsure 🔍 -5% - +1% -1.56ms - +0.49ms	-

usedJSHeapSize

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

hydrate1k

Browser: chrome-headless 89.0.4389.114
Sample size: 50
Built by: Benchmarks #215
Commit: 5b2dc48

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	141.81ms - 143.74ms	-	unsure 🔍 -1% - +2% -0.90ms - +2.62ms
preact-local	140.44ms - 143.38ms	unsure 🔍 -2% - +1% -2.62ms - +0.90ms	-

usedJSHeapSize

Version	Avg time	vs preact-master	vs preact-local
preact-master	6.17ms - 6.20ms	-	unsure 🔍 -1% - +0% -0.05ms - +0.01ms
preact-local	6.18ms - 6.23ms	unsure 🔍 -0% - +1% -0.01ms - +0.05ms	-

many_updates

Browser: chrome-headless 89.0.4389.114
Sample size: 80
Built by: Benchmarks #215
Commit: 5b2dc48

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	34.33ms - 37.22ms	-	unsure 🔍 -7% - +5% -2.64ms - +1.72ms
preact-local	34.60ms - 37.87ms	unsure 🔍 -5% - +7% -1.72ms - +2.64ms	-

usedJSHeapSize

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

text_update

Browser: chrome-headless 89.0.4389.114
Sample size: 250
Built by: Benchmarks #215
Commit: 5b2dc48

duration

Version	Avg time	vs preact-master	vs preact-local
preact-master	3.37ms - 3.73ms	-	unsure 🔍 -3% - +8% -0.10ms - +0.29ms
preact-local	3.39ms - 3.52ms	unsure 🔍 -8% - +3% -0.29ms - +0.10ms	-

usedJSHeapSize

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

_{tachometer-reporter-action v2 for Benchmarks}

Apr 15 '21 20:04 github-actions[bot]

Size Change: +2 B (0%)

Total Size: 42.4 kB

Filename	Size	Change
`dist/preact.module.js`	4.01 kB	+1 B
`dist/preact.umd.js`	4.05 kB	+1 B

ℹ️ View Unchanged

Filename	Size	Change
`compat/dist/compat.js`	3.51 kB	0 B
`compat/dist/compat.module.js`	3.52 kB	0 B
`compat/dist/compat.umd.js`	3.56 kB	0 B
`debug/dist/debug.js`	2.99 kB	0 B
`debug/dist/debug.module.js`	2.98 kB	0 B
`debug/dist/debug.umd.js`	3.07 kB	0 B
`devtools/dist/devtools.js`	232 B	0 B
`devtools/dist/devtools.module.js`	241 B	0 B
`devtools/dist/devtools.umd.js`	308 B	0 B
`dist/preact.js`	3.98 kB	0 B
`dist/preact.min.js`	4.01 kB	0 B
`hooks/dist/hooks.js`	1.13 kB	0 B
`hooks/dist/hooks.module.js`	1.15 kB	0 B
`hooks/dist/hooks.umd.js`	1.2 kB	0 B
`jsx-runtime/dist/jsxRuntime.js`	327 B	0 B
`jsx-runtime/dist/jsxRuntime.module.js`	335 B	0 B
`jsx-runtime/dist/jsxRuntime.umd.js`	405 B	0 B
`test-utils/dist/testUtils.js`	437 B	0 B
`test-utils/dist/testUtils.module.js`	439 B	0 B
`test-utils/dist/testUtils.umd.js`	515 B	0 B

_{compressed-size-action}

Apr 15 '21 20:04 github-actions[bot]

Coverage remained the same at 99.446% when pulling 5b2dc4813bc13b6338ef801fa2d23de11779059f on createelement-undefined-children into 4cdcc88afde9000c1b2e1b5d6e6f0a96f8f39ca6 on master.

Apr 15 '21 20:04 coveralls

IMO, we should match the behavior of react and babel-plugin-transform-react-jsx with the new jsx runtime.

Babel with the new jsx runtime (which preact also supports with preact/jsx-runtime) transpiles

<div children="foo">{undefined}</div>

import { jsx as _jsx } from "preact/jsx-runtime";

/*#__PURE__*/
_jsx("div", {
  children: "foo",
  children: undefined
});

Apr 16 '21 02:04 clyfish

That is a bug in Babel and doesn't affect createElement(), it only affects jsxs()

Apr 17 '21 16:04 developit

The bug affects both React.createElement and Babel, if it is a bug. I think Preact needs to be consistent with React and Babel, whether it is a bug or not. https://github.com/facebook/react/blob/v17.0.2/packages/react/src/ReactElement.js#L386-L400

Apr 18 '21 15:04 clyfish

preact preact copied to clipboard

Ignore undefined children in createElement 3rd arg

📊 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

preact
preact copied to clipboard