New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Controllers can be added/removed during lifecycle. #4388
Conversation
Fixes lit#4266. Controllers are maintained via a Set instead of an Arry, allowing them to be iterated stably while being modified.
🦋 Changeset detectedLatest commit: d0773b7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultsthis-change
render
update
update-reflect
this-change, tip-of-tree, previous-release
render
update
nop-update
this-change, tip-of-tree, previous-release
render
update
this-change, tip-of-tree, previous-release
render
update
update-reflect
|
The size of lit-html.js and lit-core.min.js are as expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice, looks like Sets are very well behaved when mutated during iteration: https://lit.dev/playground/#gist=f424f3aab96d7c839d43c2c5815c8a10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Fixes #4266. Controllers are maintained via a Set instead of an Arry, allowing them to be iterated stably while being modified.