symfony / ux

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

[React] Add `permanent` option to `react_component` function #2283

Closed smnandre closed 1 month ago

smnandre commented 1 month ago
Q A
Bug fix? yes
New feature? yes
Issues Fix #...
License MIT

(Issue reported on a work project)

Using Turbo data-turbo-permanent creates race conditions that lead to the controller code failing to keep or reload the component.

In the current implementation, the Stimulus controller unmounts the component then tried to create it again.. which cannot be done:

Once you call root.unmount you cannot call root.render again on the same root. Attempting to call root.render on an unmounted root will throw a “Cannot update an unmounted root” error. However, you can create a new root for the same DOM node after the previous root for that node has been unmounted.

https://react.dev/reference/react-dom/client/createRoot#root-unmount


~The proposed implementation uses a short delay before calling unmount. It is "morphing-library-agnostic", and will work with any Turbo-like framework, with no impact on the existing projects using UX React (except a bug fixed 😅 ).~

New implementation:

A new option permanent prevent the "unmount" of the component when set to true

{# This component will not be unmounted on "disconnect" #}

{{ react_component('MyComponent', props, {permanent: true}) }}

This allows to avoid the bug when a component is disconnected and immediately reconnected (ex with Turbo)

codisart commented 1 month ago

Hello @smnandre, I work on a website for a french media company which include a audio player which is implemented using react. We tried to use symfony/ux react component with turbo and a data-turbo-permanent attribute to allow the non stop music playing during navigation.

We discovered that it was not possible with the current code like you said. But it was easily fix moving the connect function to the initialize function and removing the disconnect entierly.

Should we use your implementation instead ? Would we see bugs with our solution ? It seems to me that using only initialize lifecycle is more coherent for a permanent component.

smnandre commented 1 month ago

Hi @codisart !

I changed the implementation to avoid playing with timeouts/delays.

(I updated the first message)

kbond commented 1 month ago

Thanks Simon.