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

X-html in x-for #2766

Merged
merged 4 commits into from
Mar 24, 2022
Merged

X-html in x-for #2766

merged 4 commits into from
Mar 24, 2022

Conversation

SimoTod
Copy link
Collaborator

@SimoTod SimoTod commented Mar 17, 2022

Currently, for newly created elements, the initTree callback is inside a mutate dom call which means that directives injected by x-html are not picked up by the mutation observer. It shouldn't be a big issue to move initTree outside of mutateDOm as we do for refreshScope when dealing with elements that are shuffled around.

@leobm
Copy link

leobm commented Mar 21, 2022

ok , cool, the fix looks really simple. What are the implications of the fix? Why was the init for cloned nodes previously in mutateDom?

@SimoTod
Copy link
Collaborator Author

SimoTod commented Mar 21, 2022

From my tests, it doesn't seem to have any undesired side effects. I don't know if there is a reason why it was built that way but Caleb will know when it gets time to review it.

@calebporzio
Copy link
Collaborator

Thanks @SimoTod!

Yeah I don't remember why I have it in there. I'm sure there was a reason. Hopefully the reason was just to make x-for faster because it's DOM Manipulations wouldn't hit the mutation observer. But I don't actually recall.

Because this could have other implications, or at least slow it down a little bit, is it a better strategy to instead call "mutateDom" inside x-html? and init the tree directly from there?

@SimoTod
Copy link
Collaborator Author

SimoTod commented Mar 21, 2022

Thanks for the feedback, sounds good to me. I'll update the PR.

@calebporzio
Copy link
Collaborator

Thanks @SimoTod! I'm much more comfortable merging this. Thanks again

@calebporzio calebporzio merged commit 3fe2086 into alpinejs:main Mar 24, 2022
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.

3 participants