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

Alpine.clone results in click handler being called twice #756

Closed
dvic opened this issue Sep 9, 2020 · 28 comments
Closed

Alpine.clone results in click handler being called twice #756

dvic opened this issue Sep 9, 2020 · 28 comments

Comments

@dvic
Copy link

dvic commented Sep 9, 2020

Reported in phoenixframework/phoenix_live_view#1140 with an example repo.

Short summary: I have a list view that gets three items added after 1 second. Each list item has a @click handler. The weird thing is: it seems that Alpine.clone results in the click handler of the list item being called twice. If I remove that line in app.js, the click handlers still work and they are just called once. Also putting x-data on the list item affects this bug, with it, it never gets called twice.

I tried to isolate the issue with a JavaScript only example, but I was not able to (because in that example I'm not calling / don't need to call Alpine.clone). I'm not sure what a "proper" javascript-only example would be that tests whether Alpine.clone is doing the right thing. Any suggestions?

update: the problem is not specific to Phoenix, the problem also exists with Livewire: https://github.com/dvic/hello-livewire (click on the button on the home page, then on the list item, console.log will be called twice)

@dvic
Copy link
Author

dvic commented Sep 9, 2020

update I managed to reproduce the issue, not sure if it makes sense though (with the Alpine.clone call):

https://jsbin.com/zucomuguza/1/edit?html,js,console,output

image

@HugoDF
Copy link
Contributor

HugoDF commented Sep 9, 2020

From debugging the source it seems that the li listener (technically initializeElements) is getting initialised twice, once by the mutation observer and once by the existing ul component re-initialising itself on post-DOM-insertion

@dvic
Copy link
Author

dvic commented Sep 10, 2020

@HugoDF Thanks for looking into it. Is the mutation observer attached to each scope? Because if I remove the x-data on the outer div, the problem disappears.

@KevinBatdorf
Copy link
Contributor

Chiming in again after some thought. Updated my last response as although I think related, also not related (still probably some optimization could be added the the mutation observer though)

Doesn't Alpine.clone() require the target component to have the x-data attribute to work as expected?

Looking at the tests, and adding ul.setAttribute("x-data", ""); to your example case before the cloning seems to work as expected.

Does that change anything in your actual use?

@dvic
Copy link
Author

dvic commented Sep 11, 2020

I'm not sure if the x-data is required. If it is, then I don't know how that would help me (I'm using Phoenix LiveView). In my actual usecase, LiveView calls the onBeforeElUpdated callback with the root div of the component that is being updated (which has x-data in this case). The recommended callback in Phoenix LiveView is:

onBeforeElUpdated(from, to) {
  if (from.__x) {
    window.Alpine.clone(from.__x, to)
  }
},

@KevinBatdorf
Copy link
Contributor

KevinBatdorf commented Sep 11, 2020

Maybe elsewhere too, but I know there is a check in the mutation observer for that x-data attribute, so it's required for some functionality at least.

Can you reproduce the issue on jsbin with the x-data attribute in place?

To solve your main issue though, do you need to use .clone()? Usually there are better ways to do things than rely on the internal API. If you reinsert any Alpine component into the DOM it will re-evaluate itself automatically, for example. Something like $el.replaceWith($el.cloneNode(true)) would be a way to instruct Alpine to reset the component. It depends on your situation though.

@dvic
Copy link
Author

dvic commented Sep 11, 2020

Can you reproduce the issue on jsbin with the x-data attribute in place?

No I can't, I'll see if I can come up with other examples but with this one I can't reproduce it.

To solve your main issue though, do you need to use .clone()? Usually there are better ways to do things than rely on the internal API. If you reinsert any Alpine component into the DOM it will re-evaluate itself automatically, for example. Something like $el.replaceWith($el.cloneNode(true)) would be a way to instruct Alpine to reset the component. It depends on your situation though.

Well I'm starting to ask myself the same thing now, I just followed the advice that came out of the suggestion of @calebporzio (phoenixframework/phoenix_live_view#809 (comment) and phoenixframework/phoenix_live_view#809 (comment)). I'm not sure why and when the clone() call is necessary, because without it, the particular Phoenix LiveView autocomplete that I have now works. But I (and other users) encountered also situations where it was required to call it, so I'm a bit confused as to whether this is the right way to do it.

@SimoTod
Copy link
Collaborator

SimoTod commented Sep 11, 2020

You need clone to preserve the datacontext between partial renderings, for example you have an x-model that you change. Foralpine, that snippet of html is a new component every time and it would reset to the original x-data content. If you don't need to preserve client side data, you can safely skip clone. It's definitely a bug though.

@dvic
Copy link
Author

dvic commented Sep 11, 2020

Clear, and of course with frameworks like LiveView or LiveWire you have one global "onElUpdated" callback so you'd always call clone(), even if you don't need it for a particular element.

@dkln
Copy link

dkln commented Nov 10, 2020

I'm having this problem when using LiveView, Alpine and the clone function. What I've noticed, is that after clone, the click listener is still bound to the "old" element. So I end up with two click dispatches with only one being bound to the new DOM element.

@KevinBatdorf
Copy link
Contributor

@dkln Is the old element still on the page? If not, the browser's garbage collector should be clean that up old event listener soon enough, probably after the first click.

@dkln
Copy link

dkln commented Nov 11, 2020

Using the Chrome inspector with a little help of a simple console.log reveals that one element is one the page in the DOM and the other one is nowhere to be found. I added this console.log in the click event. This will result in the following:

Screenshot from 2020-11-11 09-05-37

If I collapse both elements, I see that the invisible element has different contents that it's visible counterpart:

Screenshot from 2020-11-11 09-11-10

(In the screenshot above, the last log entry is the element that is currently visible on the page)

@dvic
Copy link
Author

dvic commented Nov 11, 2020

I think, if I'm not mistaken, that I mimicked exactly this behavior (of Phoenix LiveView) in the above jsbin (https://jsbin.com/zucomuguza/1/edit?html,js,console,output). The main take away that I got from it is that the problem only occurs if the parent is an Alpine container (i.e., has the x-data attribute). I assume this is also the case for you @dkln ?

@dkln
Copy link

dkln commented Nov 11, 2020

@dvic correct!

@KevinBatdorf
Copy link
Contributor

What happens if you click a second time? Are both logged again?

@KevinBatdorf
Copy link
Contributor

Okay, I figured why the issue occurs and a solution. However, it's not a trivial fix by any means so I don't think it's something that will be merged in without more examples of it breaking, and without more people coming here with similar issues (Just my guess though!).

Here's the fix implemented with the jsfiddle from before: https://jsbin.com/rakovuruwi/edit?html,js,console,output

(Note that it's a rough fix and other Alpine tests are failing. Its because it would require a significant refactor and I just wanted to test my thoughts on the current issue here. In short, don't use the code I shared as it's broken in almost every other way than with a simple click event listener)

@dkln Is it possible to share your code as well?

Best case there is a better way to implement Alpine into LiveView without having to change anything. For example, the fiddle above also works on current Alpine version if you use .cloneNode() to create the new element instead:
https://jsbin.com/bixidewete/edit?html,js,console,output

@dvic
Copy link
Author

dvic commented Nov 11, 2020

Haven't looked at the solution, but nice 👍 The cloneNode 'fix' is interesting but I'm not sure it helps because afaik Phoenix LiveView uses https://github.com/patrick-steele-idem/morphdom to update the DOM, maybe I can cook up a reasonable example using morphdom + alpine.js? (if that helps better determine the importance of this issue?)

@KevinBatdorf
Copy link
Contributor

maybe I can cook up a reasonable example using morphdom + alpine.js?

Livewire is using a modified version of morphdom but if it breaks in both versions then a fix should be added so that Livewire is fully supported. If you can reproduce something in Livewire that would be the most ideal, of course.

@dvic
Copy link
Author

dvic commented Nov 11, 2020

@KevinBatdorf I can confirm the problem also exists in Livewire (using CDN Alpine 2.7.3), here is an example repo: https://github.com/dvic/hello-livewire

From the console:
image

This means that the bug is not specific to Phoenix LiveView or Livewire, but probably to this "specific" usage of Alpine + Morphdom?

@dvic
Copy link
Author

dvic commented Nov 17, 2020

@KevinBatdorf you mentioned the fix for this issue is not really a trivial fix. Now that we know the bug is also present in Livewire, do you believe there is enough justification to further explore your solution and merge that? Or would you go another direction? If there anything I could help out with, then let me know! Thanks!

@KevinBatdorf
Copy link
Contributor

I just need to find the time to set it up. I might have some time tomorrow actually. Can you mention the steps to reproduce the issue? ie. Load the home page, click the button, etc.

Or is it pretty obvious?

@dvic
Copy link
Author

dvic commented Nov 18, 2020

It's the same kind of example, right on the home page of the example app. Good luck!

@dvic
Copy link
Author

dvic commented Feb 23, 2021

@KevinBatdorf I can confirm that v2.8.1 solves this problem

@dvic dvic closed this as completed Feb 23, 2021
@richstandbrook
Copy link

I've updated my development project but I'm still getting this kind of bug, https://laravelplayground.com/#/snippets/e522714f-f15d-4314-b478-7a665a1e89a3 you can see in devtools that the checkbox replacing the deleted item has two events attached

@jinhucheung
Copy link

I have this problem on v3.3.3
test

@SimoTod
Copy link
Collaborator

SimoTod commented Sep 12, 2021

@jinhucheung there is a big warning in your dev console telling you that you forgot the defer attribute on the script tag. Does that fix your issue?

@jinhucheung
Copy link

jinhucheung commented Sep 13, 2021

@SimoTod Thank you! I am playing around with a project that is using turbolinks.

I found your great work.
https://github.com/SimoTod/alpine-turbo-drive-adapter

But it cannot be used in alpinejs@3.x. So I can only downgrade alpinejs to 2.8.1.

I tried to use alpinejs@3.3.3 without turbolinks. It works well.

Thanks for help!

@dvic
Copy link
Author

dvic commented Sep 13, 2021

FYI see also livewire/livewire#3392, it seems that using liveview or turbolinks (both which replace parts of the page) causes problems with 3.x.

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 a pull request may close this issue.

7 participants