[Autocomplete] Avoid destroying on disconnect

[weaverryan]

Q	A
Bug fix?	yes
New feature?	no
Tickets	Fix #1204
License	MIT

The tests pass - so I can't see a problem with NOT destroying the element.

[smnandre]

so I can't see a problem with NOT destroying the element.

Is that not best practice in term of memory, Turbo compatibility, etc ?

• https://www.betterstimulus.com/integrating-libraries/lifecycle
• https://stimulus.hotwired.dev/reference/lifecycle-callbacks
• https://thoughtbot.com/blog/taking-the-most-out-of-stimulus

If the controller creates the instance, it should handle the deletion too, no ?

Could Sortable mess with the Dom / create duplicates ?

[smnandre]

The tests pass

Is there any test related to the disconnection ? 👼

[weaverryan]

Indeed, destroying is a best practice. And looking at tom-select now, it DOES remove some event listeners (for me, removal of event listeners is the REAL important part of disconnect().

Sigh, in that case, maybe @rodnaph can create a reproducer so I can dive deeper into the underlying cause of why the value is lost?

[smnandre]

I'm wondering if the cloning could conflict with stimulus... https://sortablejs.github.io/Sortable/#cloning

[smnandre]

So, after many tests, there is indeed something we need to fix in our implementation...

...but it seems your solution could be a good fix in the meanwhile (pro/cons 100% favor).

WDYT to merge it and then open another issue to study why our implementation has hidden side-effets ?

[weaverryan]

This is ready to go. I kept the destroy(). The problem is that TomSelect.destroy() reverts the innerHTML of the <select>. This causes any value the user has selected to be lost. We restore it.