Skip to content
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

Add failing test for x-model on checkboxes inside conditional #5454

Closed
wants to merge 5 commits into from

Conversation

austenc
Copy link
Collaborator

@austenc austenc commented Dec 20, 2022

The Problem:
In some cases, the change listener for x-model is duplicated when x-model is within a blade @if. This causes incorrect values to be bound when checkboxes are bound to an array on the Alpine side of things.

I would expect the checkbox here to bind 1 to the checks array only once, but it actually is bound twice!
CleanShot 2022-12-20 at 15 36 58

Discovered this while using Livewire v2 and Alpine v3. Didn't make a discussion... the failing test seemed more useful to post as a PR instead. Tried to figure out what was happening on the Alpine side, but haven't been successful thus far. It maxes out at two x-model listeners. Something to do with cloning the Alpine component and the original x-model event listener not being removed.

See this line:
https://github.com/livewire/livewire/pull/5454/files#diff-c9834c12ea72083bd51b7800e43124ccc188229415d4c7a3e30770d4e66a6d65R21

If the checkbox is within the @if statement, the problem appears. When Livewire "Alpinifies" the element, it seems to clone and never remove the original event listener.

@austenc
Copy link
Collaborator Author

austenc commented Dec 20, 2022

If you add the line below before this block of code the problem seems fixed. Not sure what it does, though!

el._x_removeModelListeners && el._x_removeModelListeners['default'] && el._x_removeModelListeners['default']()

CleanShot 2022-12-20 at 16 30 21

The question is, why is the model directive's method called twice when Livewire injects the content on the page?

@joshhanley
Copy link
Member

@austenc I was thinking maybe it was adding the second listener as part of the checking the checkbox request, but looking at it, you're right, it's adding the event listener twice when the checkbox is added to the page.

Yeah I think trying to find the underlying cause as to why the listener is being added twice would be the better solution.

@austenc
Copy link
Collaborator Author

austenc commented Dec 21, 2022

Okay, thanks @joshhanley, when alpinejs/alpine#3351 is merged and tagged (🤞) we can update the Alpine version used in the testing app.blade.php layout and this test will pass. Then I think this is good to merge and tag!

Will wait to see what the consensus is over on the Alpine repo and update this after that. Thanks!

@austenc
Copy link
Collaborator Author

austenc commented Jan 31, 2023

We'll probably want to bump the version of Alpine used in the tests for this (once the next release is tagged there).

Please feel free to close if this test doesn't belong in Livewire. It's fixed in Alpine, so perhaps this test isn't needed now?

@austenc austenc marked this pull request as ready for review January 31, 2023 14:41
@joshhanley
Copy link
Member

@austenc yup, it will need to wait for the Alpine release. Not sure if test is still needed but can't hurt either 🤷‍♂️

@nuernbergerA
Copy link
Contributor

I would keep it. Maybe some stuff will break again in alpine then we have a test here, therefore we should include the latest version of alpine in test.

@nuernbergerA
Copy link
Contributor

@vercel
Copy link

vercel bot commented Apr 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
livewire ❌ Failed (Inspect) Apr 13, 2023 11:59pm

@austenc
Copy link
Collaborator Author

austenc commented Apr 14, 2023

@austenc you can add https://github.com/alpinejs/alpine/releases/tag/v3.12.0

Thanks for the reminder, @nuernbergerA! Updated.

@calebporzio
Copy link
Collaborator

Thank you for your contributing to Livewire. Your efforts are much appreciated.

We are declaring "Livewire PR bankruptcy:" closing all outstanding pull requests to start fresh with a clean slate.

If you think this is too heavy-handed and your PR is important, please re-open it against the v2 branch.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants