Skip to content

Prevent removeChild exception #2

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Prevent removeChild exception #2

wants to merge 5 commits into from

Conversation

Swizz
Copy link

@Swizz Swizz commented Jan 7, 2020

This fix the

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

exception occuring when changing route using Sapper.

This occur because of the deletetion of the node in the DOM before of the destroy lifecycle being called.

fix #1

@Swizz
Copy link
Author

Swizz commented Jan 7, 2020

I dont know the need behind the use of destroy.

But I would recommand to use of the following, instead

function fragment(node) {
  node.parentElement.appendChild(document.adoptNode(node).content);
}

That will produce a more clean domtree close to what we wanted at start. sveltejs/svelte#2080

Orginal behavior :
image

Proposed behavior :
image

The node adoption make sense because the template tag will not be reuse in the context. It the one time use. And given the slot use, it should not be use in another context.

I will personnaly strongly stand in favor of adoptNode.

@Swizz
Copy link
Author

Swizz commented Jan 7, 2020

Well. I found out the why of the destroy lifecycle need. But it is also the orginal reason of the bug.
Even the original PR will solve nothing. I will continue to work on this.

I focused my self on the Sapper routing, but conditional rendering and loop are involed too.

edit:
To provide explanation. When a fragment is added as node child its content is spread as children, but it seem we are not able to use the fragment content to remove these childs from the DOM.

From : https://developer.mozilla.org/en-US/docs/Web/API/DocumentFragment#Usage_notes

Doing this moves the fragment's nodes into the DOM, leaving behind an empty DocumentFragment. Because all of the nodes are inserted into the document at once, only one reflow and render is triggered instead of potentially one for each node inserted if they were inserted separately.

That's why we can't remove child from an used fragment.

@Swizz Swizz changed the title Prevent removeChild exception WIP: Prevent removeChild exception Jan 7, 2020
@Swizz Swizz changed the title WIP: Prevent removeChild exception Prevent removeChild exception Jan 7, 2020
@Swizz
Copy link
Author

Swizz commented Jan 7, 2020

Here is the workaround I found.
I let you some comments @qutran, to open the discussion about it.

@Swizz
Copy link
Author

Swizz commented Jan 7, 2020

Following a weird behavior of Sapper. This feature should be needed too.

Because of SSR, I would love to not necessarly use a template element as slot node.
This change mimic the fragment behavior for non template nodes.

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.

Failed to execute 'removeChild' on 'Node'
1 participant