glimmerjs / glimmer-vm

MIT License
1.13k stars 191 forks source link

Conflicts with dragula throwing Uncaught DOMException #358

Closed tmeloliveira closed 7 years ago

tmeloliveira commented 7 years ago

Hi guys, I’m having a few issues with glimmer 2 after I tried to upgrade my app to Ember 2.10.0.

I'm using a library for drag and drop called dragula, which is being wrapped through ember-dragula.


My scenario is the following:

I have a model for categories which has_many items. So in my page, I have a bunch of <ul> to represent each category, and a bunch of <li> to represent each item inside the category. I use dragula so that our users can drag an item from one category into the other and when they drop it, I make the proper model updates.

The thing is, this was working just fine with Ember 2.9.1, but when I tried to upgrade to 2.10.0, I keep getting this error messages on console:

Uncaught DOMException: Failed to execute 'insertBefore' on 'Node': The node before which the new node is to be inserted is not a child of this node.

Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

Uncaught Error: Cannot call next() on an empty iterator

It seems like there is a conflict between the changes that dragula makes to the DOM and what Ember expects to see in the DOM. I tried to debug as much as I could, but I couldn’t find a solution.

Console is showing that the errors are coming from these files:

Initially I thought the issue was related to ember-dragula, because the add-on isn't updated often. But after some debugging, it seems like it's more of a conflict than an actual bug.

Any help on that would be much appreciated! 

My team and I are looking forward to see our app running on 2.10!

ghost commented 7 years ago

I am seeing the Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node. within one of our components that integrates with a charting library. The error occurs from https://github.com/tildeio/glimmer/blob/master/packages/glimmer-runtime/lib/bounds.ts#L82-L83. Upon pausing execution, the parent was a valid element attached to the DOM, but next was null. My hunch is that the external library's DOM manipulation causes a mismatch with Glimmer or vice versa. happy to isolate this into a test case too.

krisselden commented 7 years ago

Prior versions of Ember did bounds tracking that would be confused by manual DOM manipulation, however, this bounds checking did not cache parentNode of the boundary nodes, this allowed for more manual DOM manipulation to just work, so long as you didn't mess up the boundary nodes you could move the boundary nodes and the contents and everything would just work (ember-wormhole). The bounds objects in glimmer have cached parentNode, which has added more caveats to manual DOM manipulation mixing with glimmer DOM manipulation.

ghost commented 7 years ago

Thus, the error is expected behavior @krisselden ? e.g. Glimmer2 requires stricter validation of DOM states in order to provide the performance benefits. That makes sense; however, I would like to help documenting a best practice / workaround for everyone out there who has to refactor part of their application to handle this.

krisselden commented 7 years ago

@efx the only workaround at the moment is the same as ember-wormhole you need a static Element in your template that you move, you can't move Nodes that touch the bounds that Glimmer manages in a way that changes the first, last and parent of those bounds. The first and last thing has always been true, the parent is new.

Drag and drop is best "previewed" (ghosted, or changed then reverted before glimmer updates) then you update the source state and let the glimmer update the DOM to reflect the state. Within a single list, if properly keyed, glimmer will move instead of recreate the elements. Between two lists there is no move option that currently works, it is always a remove from one list and rebuild in the other. In order to declaritively handle the latter, we need to be able to pass the same block to more than one each, and scope the key across lists.

tmeloliveira commented 7 years ago

@krisselden thanks so much for the help!

I ended up doing what you suggested... I'm reverting the drop event and then updating the model, so that ember/glimmer can take care of the re-rendering.

If anyone is interested, for dragula there is a way to revert the drop event with drake.cancel(true). More details here: https://github.com/bevacqua/dragula/issues/188#issuecomment-146703638 and here: https://github.com/kalcifer/ember-dragula/issues/25

Appreciate the help guys! I've being fighting this bug for quite some time now! 🐛 🔫 Glimmer 2, here I go!

tmeloliveira commented 7 years ago

Should I close the issue then?

rwjblue commented 7 years ago

Yes, but thank you for following up with your solution!