[react] "Unset" missing properties #4534

augustjk · 2024-02-01T00:22:32Z

Fixes #4497
Also related #4227

What changed

We now collect previous element props in a map and actually check off ones that we update. We then go through any remaining and set them all as undefined.

Open questions

1. Is patch bump right for this?

It could be considered a bugfix, or it could even be considered breaking. Or a new "feature" of handling unset properties?

2. Is `undefined` the correct behavior?

undefined feels most correct in terms of semantics of "unsetting", but I wanted to see what React's future custom element support would do and it seems react-dom@experimental will actually set the property as null if it's missing.
Playground: https://lit.dev/playground/#gist=001005b1cfb39fd03ccb37e841d256e5
(The current value of the property is logged in the console)

Resolution

We'll include this in a patch release as the existing behavior is considered unexpected, therefore this is a bugfix.

Filed an issue with React regarding they're behavior of setting null. Setting to undefined seems like the semantically correct thing to do.

changeset-bot · 2024-02-01T00:22:36Z

🦋 Changeset detected

Latest commit: a56691e

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

This PR includes changesets to release 2 packages

Name	Type
@lit/react	Patch
@lit-internal/test-elements-react	Patch

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-02-01T00:22:45Z

📊 Tachometer Benchmark Results

Summary

nop-update

this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +3% (-0.58ms - +0.38ms)
^{this-change vs tip-of-tree}

render

this-change: 47.78ms - 50.41ms
this-change, tip-of-tree, previous-release: slower ❌ 0% - 6% (0.01ms - 1.12ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +4% (-0.52ms - +1.26ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-0.58ms - +0.64ms)
^{this-change vs tip-of-tree}

update

this-change: 551.15ms - 565.83ms
this-change, tip-of-tree, previous-release: unsure 🔍 -8% - +4% (-3.28ms - +1.47ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-1.67ms - +1.45ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-10.84ms - +8.20ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 549.46ms - 556.78ms
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +1% (-12.35ms - +5.71ms)
^{this-change vs tip-of-tree}

Results

this-change

Browser: chrome-headless 121.0.6167.85
Sample size: 50
Built by: Benchmarks #6096
Commit: a56691e

render

Version	Avg time	vs
	47.78ms - 50.41ms	-

update

Version	Avg time	vs
	551.15ms - 565.83ms	-

update-reflect

Version	Avg time	vs
	549.46ms - 556.78ms	-

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

Browser: chrome-headless 121.0.6167.85
Sample size: 30
Built by: Benchmarks #6096
Commit: a56691e

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	18.65ms - 19.48ms	-	slower ❌ 0% - 6% 0.01ms - 1.12ms	unsure 🔍 -2% - +4% -0.40ms - +0.81ms
tip-of-tree tip-of-tree	18.13ms - 18.87ms	faster ✔ 0% - 6% 0.01ms - 1.12ms	-	unsure 🔍 -5% - +1% -0.93ms - +0.21ms
previous-release previous-release	18.43ms - 19.29ms	unsure 🔍 -4% - +2% -0.81ms - +0.40ms	unsure 🔍 -1% - +5% -0.21ms - +0.93ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	38.13ms - 41.38ms	-	unsure 🔍 -8% - +4% -3.28ms - +1.47ms	unsure 🔍 -6% - +6% -2.31ms - +2.49ms
tip-of-tree tip-of-tree	38.93ms - 42.39ms	unsure 🔍 -4% - +8% -1.47ms - +3.28ms	-	unsure 🔍 -4% - +9% -1.48ms - +3.47ms
previous-release previous-release	37.90ms - 41.43ms	unsure 🔍 -6% - +6% -2.49ms - +2.31ms	unsure 🔍 -8% - +4% -3.47ms - +1.48ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	10.65ms - 11.29ms	-	unsure 🔍 -5% - +3% -0.58ms - +0.38ms	unsure 🔍 -5% - +5% -0.56ms - +0.51ms
tip-of-tree tip-of-tree	10.72ms - 11.43ms	unsure 🔍 -3% - +5% -0.38ms - +0.58ms	-	unsure 🔍 -4% - +6% -0.47ms - +0.63ms
previous-release previous-release	10.57ms - 11.42ms	unsure 🔍 -5% - +5% -0.51ms - +0.56ms	unsure 🔍 -6% - +4% -0.63ms - +0.47ms	-

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

Browser: chrome-headless 121.0.6167.85
Sample size: 50
Built by: Benchmarks #6096
Commit: a56691e

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.16ms - 34.75ms	-	unsure 🔍 -2% - +4% -0.52ms - +1.26ms	unsure 🔍 -2% - +3% -0.68ms - +1.15ms
tip-of-tree tip-of-tree	33.19ms - 34.00ms	unsure 🔍 -4% - +2% -1.26ms - +0.52ms	-	unsure 🔍 -2% - +1% -0.74ms - +0.47ms
previous-release previous-release	33.27ms - 34.17ms	unsure 🔍 -3% - +2% -1.15ms - +0.68ms	unsure 🔍 -1% - +2% -0.47ms - +0.74ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	68.50ms - 70.52ms	-	unsure 🔍 -2% - +2% -1.67ms - +1.45ms	unsure 🔍 -2% - +2% -1.50ms - +1.31ms
tip-of-tree tip-of-tree	68.42ms - 70.81ms	unsure 🔍 -2% - +2% -1.45ms - +1.67ms	-	unsure 🔍 -2% - +2% -1.53ms - +1.55ms
previous-release previous-release	68.63ms - 70.58ms	unsure 🔍 -2% - +2% -1.31ms - +1.50ms	unsure 🔍 -2% - +2% -1.55ms - +1.53ms	-

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

Browser: chrome-headless 121.0.6167.85
Sample size: 50
Built by: Benchmarks #6096
Commit: a56691e

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	31.78ms - 32.64ms	-	unsure 🔍 -2% - +2% -0.58ms - +0.64ms	unsure 🔍 -1% - +3% -0.41ms - +0.85ms
tip-of-tree tip-of-tree	31.75ms - 32.61ms	unsure 🔍 -2% - +2% -0.64ms - +0.58ms	-	unsure 🔍 -1% - +3% -0.44ms - +0.82ms
previous-release previous-release	31.53ms - 32.45ms	unsure 🔍 -3% - +1% -0.85ms - +0.41ms	unsure 🔍 -3% - +1% -0.82ms - +0.44ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	528.59ms - 541.72ms	-	unsure 🔍 -2% - +2% -10.84ms - +8.20ms	unsure 🔍 -2% - +2% -10.45ms - +8.13ms
tip-of-tree tip-of-tree	529.58ms - 543.37ms	unsure 🔍 -2% - +2% -8.20ms - +10.84ms	-	unsure 🔍 -2% - +2% -9.37ms - +9.68ms
previous-release previous-release	529.74ms - 542.89ms	unsure 🔍 -2% - +2% -8.13ms - +10.45ms	unsure 🔍 -2% - +2% -9.68ms - +9.37ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	530.69ms - 542.67ms	-	unsure 🔍 -2% - +1% -12.35ms - +5.71ms	unsure 🔍 -2% - +1% -9.47ms - +7.83ms
tip-of-tree tip-of-tree	533.23ms - 546.76ms	unsure 🔍 -1% - +2% -5.71ms - +12.35ms	-	unsure 🔍 -1% - +2% -6.70ms - +11.70ms
previous-release previous-release	531.25ms - 543.74ms	unsure 🔍 -1% - +2% -7.83ms - +9.47ms	unsure 🔍 -2% - +1% -11.70ms - +6.70ms	-

_{tachometer-reporter-action v2 for Benchmarks}

github-actions · 2024-02-01T00:24:26Z

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

augustjk · 2024-02-01T03:47:18Z

package.json

@@ -171,7 +171,6 @@
        "./packages/labs/gen-wrapper-vue:test",
        "./packages/labs/nextjs:test",
        "./packages/labs/preact-signals:test",
-        "./packages/labs/react:test",


This feels like a band-aid. I had to remove it because there's a copy of tests in packages/labs/react that are now stale and fail. I don't think it makes sense to update those files, and I don't think there's reason to keep things like tests and rollup builds in the labs directories for graduated packages.

Strongly agree. Are there files to delete?

There are. I think they can be done as a separate chore task. Nothing super pressing.

AndrewJakubowicz · 2024-02-01T22:38:59Z

packages/react/src/test/create-component_test.tsx

@@ -490,17 +487,15 @@ suite('createComponent', () => {
    el.fire('foo');


The line above this can be update to either no have the explicit undefined, or the comment above it can mention that both explicit undefined or omitting property will clear listener.

AndrewJakubowicz

Awesome!

Would be cool to link to the React issue (where they are currently setting to null).
Discussed options IRL.

augustjk · 2024-02-01T23:05:43Z

Regarding React's current behavior of setting null, filed facebook/react#28203

- add link to react issue - remove unnecessary type arg

augustjk requested review from justinfagnani and AndrewJakubowicz February 1, 2024 00:22

augustjk requested a review from kevinpschaaf as a code owner February 1, 2024 00:22

augustjk added 4 commits January 31, 2024 19:04

Handle props missing from previous render

b11222f

Fix tests

e17a7a9

Add changeset

61e2304

Don't run packages/labs/react test

3dd21f0

augustjk force-pushed the react-unset-prop branch from 64155cc to 3dd21f0 Compare February 1, 2024 03:42

augustjk commented Feb 1, 2024

View reviewed changes

AndrewJakubowicz reviewed Feb 1, 2024

View reviewed changes

AndrewJakubowicz approved these changes Feb 1, 2024

View reviewed changes

augustjk added 2 commits February 1, 2024 15:10

Address feedback

0bb075c

- add link to react issue - remove unnecessary type arg

More comprehensive comment for event test

a56691e

augustjk merged commit d68f5c7 into main Feb 2, 2024
9 checks passed

augustjk deleted the react-unset-prop branch February 2, 2024 01:02

lit-robot mentioned this pull request Mar 11, 2024

Version Packages #4579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[react] "Unset" missing properties #4534

[react] "Unset" missing properties #4534

augustjk commented Feb 1, 2024 •

edited

Loading

changeset-bot bot commented Feb 1, 2024 •

edited

Loading

github-actions bot commented Feb 1, 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 Feb 1, 2024

augustjk Feb 1, 2024 •

edited

Loading

AndrewJakubowicz Feb 1, 2024 •

edited

Loading

augustjk Feb 2, 2024

AndrewJakubowicz Feb 1, 2024

AndrewJakubowicz left a comment

augustjk commented Feb 1, 2024

		@@ -490,17 +487,15 @@ suite('createComponent', () => {
		el.fire('foo');

[react] "Unset" missing properties #4534

[react] "Unset" missing properties #4534

Conversation

augustjk commented Feb 1, 2024 • edited Loading

What changed

Open questions

1. Is patch bump right for this?

2. Is undefined the correct behavior?

Resolution

changeset-bot bot commented Feb 1, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Feb 1, 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 Feb 1, 2024

augustjk Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

AndrewJakubowicz Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

augustjk Feb 2, 2024

Choose a reason for hiding this comment

AndrewJakubowicz Feb 1, 2024

Choose a reason for hiding this comment

AndrewJakubowicz left a comment

Choose a reason for hiding this comment

augustjk commented Feb 1, 2024

augustjk commented Feb 1, 2024 •

edited

Loading

2. Is `undefined` the correct behavior?

changeset-bot bot commented Feb 1, 2024 •

edited

Loading

github-actions bot commented Feb 1, 2024 •

edited

Loading

augustjk Feb 1, 2024 •

edited

Loading

AndrewJakubowicz Feb 1, 2024 •

edited

Loading