Controllers can be added/removed during lifecycle. by sorvell · Pull Request #4388 · lit/lit

sorvell · 2023-11-10T21:03:14Z

Fixes #4266. Controllers are maintained via a Set instead of an Arry, allowing them to be iterated stably while being modified.

Fixes lit#4266. Controllers are maintained via a Set instead of an Arry, allowing them to be iterated stably while being modified.

changeset-bot · 2023-11-10T21:03:18Z

🦋 Changeset detected

Latest commit: d0773b7

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

This PR includes changesets to release 3 packages

Name	Type
@lit/reactive-element	Patch
lit	Patch
lit-element	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 · 2023-11-10T21:03:31Z

📊 Tachometer Benchmark Results

Summary

nop-update

this-change, tip-of-tree, previous-release: unsure 🔍 -7% - +5% (-1.18ms - +0.86ms)
^{this-change vs tip-of-tree}

render

this-change: 65.47ms - 68.42ms
this-change, tip-of-tree, previous-release: unsure 🔍 -7% - +3% (-1.81ms - +0.90ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -4% - +1% (-1.97ms - +0.51ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +3% (-0.99ms - +1.22ms)
^{this-change vs tip-of-tree}

update

this-change: 712.08ms - 723.99ms
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +6% (-1.04ms - +3.46ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +1% (-2.30ms - +1.36ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-4.58ms - +7.48ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 694.58ms - 705.94ms
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +0% (-12.41ms - +2.77ms)
^{this-change vs tip-of-tree}

Results

this-change

Browser: chrome-headless 118.0.5993.117
Sample size: 50
Built by: Benchmarks #5859
Commit: d0773b7

render

Version	Avg time	vs
	65.47ms - 68.42ms	-

update

Version	Avg time	vs
	712.08ms - 723.99ms	-

update-reflect

Version	Avg time	vs
	694.58ms - 705.94ms	-

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

Browser: chrome-headless 118.0.5993.117
Sample size: 30
Built by: Benchmarks #5859
Commit: d0773b7

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	25.55ms - 27.45ms	-	unsure 🔍 -7% - +3% -1.81ms - +0.90ms	unsure 🔍 -6% - +4% -1.63ms - +0.97ms
tip-of-tree tip-of-tree	25.98ms - 27.92ms	unsure 🔍 -3% - +7% -0.90ms - +1.81ms	-	unsure 🔍 -4% - +5% -1.19ms - +1.43ms
previous-release previous-release	25.94ms - 27.72ms	unsure 🔍 -4% - +6% -0.97ms - +1.63ms	unsure 🔍 -5% - +4% -1.43ms - +1.19ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	56.86ms - 60.71ms	-	unsure 🔍 -2% - +6% -1.04ms - +3.46ms	unsure 🔍 -2% - +6% -1.26ms - +3.30ms
tip-of-tree tip-of-tree	56.41ms - 58.74ms	unsure 🔍 -6% - +2% -3.46ms - +1.04ms	-	unsure 🔍 -3% - +3% -1.88ms - +1.50ms
previous-release previous-release	56.54ms - 59.00ms	unsure 🔍 -6% - +2% -3.30ms - +1.26ms	unsure 🔍 -3% - +3% -1.50ms - +1.88ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	16.31ms - 17.35ms	-	unsure 🔍 -7% - +5% -1.18ms - +0.86ms	unsure 🔍 -4% - +5% -0.61ms - +0.85ms
tip-of-tree tip-of-tree	16.11ms - 17.86ms	unsure 🔍 -5% - +7% -0.86ms - +1.18ms	-	unsure 🔍 -4% - +8% -0.74ms - +1.30ms
previous-release previous-release	16.19ms - 17.22ms	unsure 🔍 -5% - +4% -0.85ms - +0.61ms	unsure 🔍 -8% - +4% -1.30ms - +0.74ms	-

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

Browser: chrome-headless 118.0.5993.117
Sample size: 50
Built by: Benchmarks #5859
Commit: d0773b7

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	47.30ms - 48.59ms	-	unsure 🔍 -4% - +1% -1.97ms - +0.51ms	unsure 🔍 -2% - +2% -1.11ms - +0.76ms
tip-of-tree tip-of-tree	47.61ms - 49.74ms	unsure 🔍 -1% - +4% -0.51ms - +1.97ms	-	unsure 🔍 -1% - +4% -0.70ms - +1.82ms
previous-release previous-release	47.44ms - 48.79ms	unsure 🔍 -2% - +2% -0.76ms - +1.11ms	unsure 🔍 -4% - +1% -1.82ms - +0.70ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	102.28ms - 104.65ms	-	unsure 🔍 -2% - +1% -2.30ms - +1.36ms	unsure 🔍 -3% - +0% -3.34ms - +0.44ms
tip-of-tree tip-of-tree	102.54ms - 105.33ms	unsure 🔍 -1% - +2% -1.36ms - +2.30ms	-	unsure 🔍 -3% - +1% -3.01ms - +1.04ms
previous-release previous-release	103.45ms - 106.39ms	unsure 🔍 -0% - +3% -0.44ms - +3.34ms	unsure 🔍 -1% - +3% -1.04ms - +3.01ms	-

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

Browser: chrome-headless 118.0.5993.117
Sample size: 50
Built by: Benchmarks #5859
Commit: d0773b7

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	44.80ms - 46.52ms	-	unsure 🔍 -2% - +3% -0.99ms - +1.22ms	unsure 🔍 -1% - +4% -0.25ms - +1.92ms
tip-of-tree tip-of-tree	44.85ms - 46.24ms	unsure 🔍 -3% - +2% -1.22ms - +0.99ms	-	unsure 🔍 -1% - +4% -0.24ms - +1.68ms
previous-release previous-release	44.16ms - 45.49ms	unsure 🔍 -4% - +1% -1.92ms - +0.25ms	unsure 🔍 -4% - +1% -1.68ms - +0.24ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	718.29ms - 726.91ms	-	unsure 🔍 -1% - +1% -4.58ms - +7.48ms	unsure 🔍 -1% - +1% -7.56ms - +5.30ms
tip-of-tree tip-of-tree	716.93ms - 725.36ms	unsure 🔍 -1% - +1% -7.48ms - +4.58ms	-	unsure 🔍 -1% - +1% -8.95ms - +3.78ms
previous-release previous-release	718.96ms - 728.50ms	unsure 🔍 -1% - +1% -5.30ms - +7.56ms	unsure 🔍 -1% - +1% -3.78ms - +8.95ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	712.91ms - 721.48ms	-	unsure 🔍 -2% - +0% -12.41ms - +2.77ms	unsure 🔍 -2% - +0% -11.70ms - +1.74ms
tip-of-tree tip-of-tree	715.75ms - 728.28ms	unsure 🔍 -0% - +2% -2.77ms - +12.41ms	-	unsure 🔍 -1% - +1% -8.28ms - +7.97ms
previous-release previous-release	717.00ms - 727.34ms	unsure 🔍 -0% - +2% -1.74ms - +11.70ms	unsure 🔍 -1% - +1% -7.97ms - +8.28ms	-

_{tachometer-reporter-action v2 for Benchmarks}

github-actions · 2023-11-12T16:40:53Z

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

AndrewJakubowicz

This is an excellent bug fix. I checked the current behavior for myself with a small minimal repro , and the current behavior definitely feels like a bug and is non intuitive.

And this improvement comes with tests and a size win!

.changeset/healthy-elephants-itch.md

scripts/check-size.js

.changeset/strong-ants-think.md

rictic · 2023-11-13T18:21:01Z

packages/reactive-element/src/reactive-element.ts

-    // Note, if the indexOf is -1, the >>> will flip the sign which makes the
-    // splice do nothing.
-    this.__controllers?.splice(this.__controllers.indexOf(controller) >>> 0, 1);
+    this.__controllers?.delete(controller);


Oh nice, looks like Sets are very well behaved when mutated during iteration: https://lit.dev/playground/#gist=f424f3aab96d7c839d43c2c5815c8a10

…-remove

augustjk

Thank you! I went ahead and merged main to resolve the size check conflict after the other PR got merged, and removed the extra changeset file.

.changeset/strong-ants-think.md

Steve Orvell added 2 commits November 10, 2023 12:58

Controllers can be added/removed during lifecycle.

821040f

Fixes lit#4266. Controllers are maintained via a Set instead of an Arry, allowing them to be iterated stably while being modified.

Add changeset.

e2abae6

sorvell requested a review from kevinpschaaf as a code owner November 10, 2023 21:03

Update changeset and check-size

a60a3a9

AndrewJakubowicz approved these changes Nov 12, 2023

View reviewed changes

.changeset/healthy-elephants-itch.md Outdated Show resolved Hide resolved

scripts/check-size.js Outdated Show resolved Hide resolved

.changeset/strong-ants-think.md Outdated Show resolved Hide resolved

AndrewJakubowicz requested review from augustjk, justinfagnani and rictic November 12, 2023 17:12

rictic approved these changes Nov 13, 2023

View reviewed changes

augustjk added 2 commits November 14, 2023 05:34

Merge branch 'main' of https://github.com/lit/lit into controller-add…

6611a2a

…-remove

Remove duplicate changeset

e8c0884

augustjk approved these changes Nov 13, 2023

View reviewed changes

.changeset/strong-ants-think.md Outdated Show resolved Hide resolved

Update .changeset/strong-ants-think.md

d0773b7

AndrewJakubowicz merged commit 839ca0f into lit:main Nov 13, 2023

This was referenced Nov 14, 2023

Version Packages #4397

Closed

Version Packages #4399

Merged

Conversation

sorvell commented Nov 10, 2023

Uh oh!

changeset-bot bot commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 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

Uh oh!

github-actions bot commented Nov 12, 2023

Uh oh!

AndrewJakubowicz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rictic Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

augustjk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

changeset-bot bot commented Nov 10, 2023 •

edited

Loading

github-actions bot commented Nov 10, 2023 •

edited

Loading