symfony / ux

Symfony UX initiative: a JavaScript ecosystem for Symfony
https://ux.symfony.com/
MIT License
858 stars 315 forks source link

[LiveComponent] Refactor elementBelongsToThisComponent #2399

Closed smnandre closed 1 day ago

smnandre commented 5 days ago
Q A
Bug fix? yes
New feature? no
Issues Fix #2388
License MIT

This method is called from multiple places, sometimes before children component are instanciated and stored in the component map.

So let's back to the quickest selector we have: the browser DOM selector :)

We'll know tomorrow, but it could solve #2388

github-actions[bot] commented 5 days ago

📊 Packages dist files size difference

Thanks for the PR! Here is the difference in size of the packages dist files between the base branch and the PR. Please review the changes and make sure they are expected.

FileBefore (Size / Gzip)After (Size / Gzip)
LiveComponent
live_controller.js 120.36 kB / 23.38 kB 120.15 kB0% / 23.38 kB0%
Pechynho commented 3 days ago

I can confirm that this change fixes the issue I reported in #2388. It might be helpful to add a comment about the potential race condition that could occur, so other developers are aware of it when building new plugins. For now, however, this solution seems sufficient to me.

In the future, it might be necessary to address this issue more specifically and deal with the timing problem, but for now, I would say this solution is acceptable.

smnandre commented 2 days ago

Thank you @Pechynho for your time and for showing me where the bug was happening, how it was happening, and for having again time to check if the solution did work.

You cannot imagine how it eases this kind of situation and how it makes a bug almost.. enjoyable :)

I 100% note your comment about the whole "optimistic timing" issue and we will keep it in mind for the incoming changes.