Skip to content

Commit

Permalink
Fix components instantiated twice because of complex mutations (#2376)
Browse files Browse the repository at this point in the history
* Add failing test

* Fix component instantiated twice because of complex mutations

* Update comment
  • Loading branch information
SimoTod committed Nov 19, 2021
1 parent aa7167b commit 282aacf
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 1 deletion.
22 changes: 21 additions & 1 deletion packages/alpinejs/src/mutation.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,13 +160,33 @@ function onMutate(mutations) {
onAttributeAddeds.forEach(i => i(el, attrs))
})

// Mutations are undled together by the browser but sometimes
// for complex cases, there may be javascript code adding a wrapper
// and then an alpine component as a child of that wrapper in the same
// function and the mutation observer will receive 2 different mutations.

This comment has been minimized.

Copy link
@inaniyants

inaniyants Feb 11, 2022

I have met the same behavior for replacing elements ( or moving element in DOM ): I'm using liveview 0.17.7 and when it processes live_redirect it replaces main element in DOM with new one, copies some old (sticky) elements into new main element, applies some diffs to copied elements.

So when there are a lot of other mutations applied, replaced element tree becomes destroyed, cause one of removeNode mutation comes non bundled with addedNode and a check if (addedNodes.includes(node)) is not working.

So for complex mutations, it’s not safe to rely on mutation bundling.
Probably it somehow related with isCollecting=true state, when some mutations are bundled by alpine and some are processed directly from MutationObserver when isCollecting becomes false. In any case, it would be nice to not affect native bundling of MutationObserver.

I'm not sure how to reproduce this, so I'm just leaving my observations, and probably they could help you understand a problem better.

By the way, why does alpine need destroyTree for removedNodes ? Probably removedNode could be just marked as destroyed and real destroy could be deferred. When deferred destroying would be called it will double-check that node is not existing in DOM ...

This comment has been minimized.

Copy link
@SimoTod

SimoTod via email Feb 11, 2022

Author Collaborator
// when it comes to run them, the dom contains both changes so the child
// element would be processed twice as Alpine calls initTree on
// both mutations. We mark all node as _x_ignored and only remove the flag
// when processing the node to avoid those duplicates.
addedNodes.forEach((node) => {
node._x_ignoreSelf = true
node._x_ignore = true
})
for (let node of addedNodes) {
// If an element gets moved on a page, it's registered
// If an element gets moved on a page, it's registered
// as both an "add" and "remove", so we want to skip those.
if (removedNodes.includes(node)) continue

delete node._x_ignoreSelf
delete node._x_ignore
onElAddeds.forEach(i => i(node))
node._x_ignore = true
node._x_ignoreSelf = true
}
addedNodes.forEach((node) => {
delete node._x_ignoreSelf
delete node._x_ignore
})

for (let node of removedNodes) {
// If an element gets moved on a page, it's registered
Expand Down
28 changes: 28 additions & 0 deletions tests/cypress/integration/mutation.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,3 +110,31 @@ test('can pause and queue mutations for later resuming/flushing',
get('h1').should(haveText('3'))
}
)

test('does not initialise components twice when contained in multiple mutations',
html`
<div x-data="{
foo: 0,
bar: 0,
test() {
container = document.createElement('div')
this.$root.appendChild(container)
alpineElement = document.createElement('span')
alpineElement.setAttribute('x-data', '{init() {this.bar++}}')
alpineElement.setAttribute('x-init', 'foo++')
container.appendChild(alpineElement)
}
}">
<span id="one" x-text="foo"></span>
<span id="two" x-text="bar"></span>
<button @click="test">Test</button>
</div>
`,
({ get }) => {
get('span#one').should(haveText('0'))
get('span#two').should(haveText('0'))
get('button').click()
get('span#one').should(haveText('1'))
get('span#two').should(haveText('1'))
}
)

0 comments on commit 282aacf

Please sign in to comment.