-
-
Notifications
You must be signed in to change notification settings - Fork 396
[Autocomplete] clear tomselect instance to avoid double initialization #3155
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
base: 2.x
Are you sure you want to change the base?
Conversation
📊 Packages dist files size differenceThanks for the PR! Here is the difference in size of the packages dist files between the base branch and the PR.
|
||||||||||||
4cac269 to
10ac03b
Compare
5e5a833 to
1a899ea
Compare
|
@Kocal I took the chance to add playwright tests. I'll submit another PR with some changes to the documentation to enhance the DX. |
Kocal
left a comment
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.
Thanks Marcus, that's much appreciated!
I only have some comments about the E2E test, and of course any documentation improvement is welcomed :)
1a899ea to
2b36965
Compare
|
|
Ok, this is weird. I have force-pushed my changes this morning and the PR is still not updated. My changes are still being processed. 🤨 |
893194e to
025ed56
Compare
|
Rebased against 2.x and force-pushed again. Works now. |
|
@Kocal After several attempts trying to reproduce the behaviour locally, I was unable to provoce it. How shall we move forward? |
smnandre
left a comment
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.
A couple of comments passing by, thank you for the PR!
| @@ -0,0 +1,44 @@ | |||
| <?php | |||
|
|
|||
| declare(strict_types=1); | |||
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.
| declare(strict_types=1); |
|
|
||
| class AutocompleteSelectType extends AbstractType | ||
| { | ||
| public function buildForm(FormBuilderInterface $builder, array $options) |
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.
| public function buildForm(FormBuilderInterface $builder, array $options) | |
| public function buildForm(FormBuilderInterface $builder, array $options): void |
| // clear the text input after selecting a value | ||
| onItemAdd: () => { | ||
| this.tomSelect.setTextboxValue(''); | ||
| this.tomSelect?.setTextboxValue(''); |
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 not supposed to happen, is it ?
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.
We can make it more defensive when something like this happens.
| if (!this.tomSelect) { | ||
| return; | ||
| } |
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.
same thing
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.
See above.
| optgroupField: 'group_by', | ||
| // avoid extra filtering after results are returned | ||
| score: (search: string) => (item: any) => 1, | ||
| score: (_search: string) => (_item: any) => 1, |
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.
why this change ? genuine question here
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.
The underscore is used to indicate an unused variable while making the linter happy.
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.
Was he unhappy ? never observed that .. but ok :)
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.
🤷🏻♂️ It's not easy being a linter I guess.
| if (!this.tomSelect) { | ||
| return; | ||
| } | ||
|
|
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.
Same ting, not supposed to happen here
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.
Will be reverted.
|
@smnandre The defensive checks can remain IHMO. We tell other developers that all code paths are safe. Yes, it should not happen but we make it explicit that it will not happen. Especially in the callbacks, e.g. |
025ed56 to
f4bd90b
Compare
f4bd90b to
60f3a9d
Compare
Problem
When Autocomplete elements are removed and re-added to the DOM (e.g., with Turbo navigation), a race condition causes
"Tom Select already initialized on this element" error.
Root Cause
disconnect()destroys TomSelect but leavesthis.tomSelectpointing to the destroyed instance. IfurlValueChanged()fires before
connect()(during re-attachment), it callsresetTomSelect()on the stale reference, triggeringdouble-initialization.
Impact