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

Issues with mutations and third party libs #2399

Conversation

SimoTod
Copy link
Collaborator

@SimoTod SimoTod commented Nov 23, 2021

Many third-party libraries (carousels, data tables) will remove portions of HTML and reinsert them wrapped into additional markup when initializing.
This behaviour triggers 2 mutations on the same tick:

  1. Remove element A
  2. Add element B containing A

Currently, since we always run added nodes first and removed nodes after, 1) will always run last destroying event listeners and effects which means that components are no longer reactive.

This PR proposes to swap the order so we always do removal first and addition after.
Before fix: https://github.com/SimoTod/alpine/runs/4270770739?check_suite_focus=true#step:6:900
After fix: https://github.com/SimoTod/alpine/runs/4210531827?check_suite_focus=true#step:6:900

This would introduce a regression if the mutations happened the other way around (not sure if it's a real use case)
Regression: https://github.com/SimoTod/alpine/runs/4210531827?check_suite_focus=true#step:6:900
but this will be prevented by main...SimoTod:bug/issue-with-mutations-and-third-party-libs#diff-84fd6cab30ac34eaf2c3376fe2e0ac7e1268a30d454cad78d402f6983edd4f4fR19

Another solution would be to respect the mutation order but this gets trickier if we want to skip elements that are removed and readded without additional markup. About this last point, I wonder if this is the desired behaviour though as you could do odd stuff like https://codepen.io/SimoTod/pen/mdMYKBG? (x-for usesmutateDom so it wouldn't be affected)? Happy to take the other approach if we think it's better

Related to #2329, #2332, #1757 and probably many others

@calebporzio
Copy link
Collaborator

Thanks for digging into this @SimoTod - this makes good sense.

About your last codepen, I think that WOULD be desired behavior IMO - even if it may seem unintuitive, you kept the live DOM node and just moved it so you would expect the initialized event listeners, etc.. to remain, and therefore expect the scope to remain.

I could be thinking wrong here, but let's go with it for now. Thanks for being so thorough and attentive - you're the best!

@calebporzio calebporzio merged commit 1f6f1ca into alpinejs:main Nov 27, 2021
@SimoTod
Copy link
Collaborator Author

SimoTod commented Nov 27, 2021

No worries, I think moving the node in that way between different components would be unusual so I don't see it as a real use case. 👍

@mnaderian
Copy link

mnaderian commented Dec 1, 2021

@SimoTod I had a big problem that I was not able to use Alpine.js because of a conflict. Now this problem is solved because of you. Thank so much. God bless you man.

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

3 participants