prevent dynamic components being detached twice

[Rich-Harris]

fixes #3113. I suspect this is also the correct fix for #2086

[btakita]

I can confirm that this fixes #2086

tested using: "svelte": "btakita/svelte#gh-3113-hotfix"

[char]

Can also confirm this fixes #2086.

[Conduitry]

When using this branch locally with Sapper and the default sapper-template, navigating between pages is now causing the new page to be appended to the old one.

[btakita]

This PR does not address the detach error when the DOM node is removed by software external to svelte.

#3178

I also added some reasons in #3156 why we should have the if (node.parentNode) conditional in detach.

I've essentially been saying the same thing in several issues. Should we consolidate all discussion on the detach error into a single issue?

Here are some of the other related issues & pull requests that I could find from a cursory search.

#2086
#3187
sveltejs/sapper-template#158
sveltejs/sapper#793
#3113

[Conduitry]

Two other people were unable to reproduce the issue with Sapper I mentioned above, and when I tried again just now I wasn't able to either. I'm withdrawing that concern. It's be nice to know what was happening there for me before, but oh well.

This does also seem to address the issue when navigating away from a page with a <meta> tag in its <svelte:head>.

---

I'm also copying my comment on #3156 over here so it's more visible, as this seems to be the main place where stuff related to this is going to happen.

I guess it makes sense to have the conditional in detach. I mean ideally, we wouldn't need that check, and I also don't know how far we want to go down the road of 'be able to handle it when other code messes with the DOM'. I'm also worried about doing this masking real bugs in Svelte itself.

Perhaps as a compromise, we can log something scary sounding ('if you aren't messing with the DOM in some other way, this shouldn't happen', except scarier) if there's no parent node and we're compiling in dev mode?

@pngwn said:

Svelte is already pretty forgiving when it comes to using third-party libraries that interact with the DOM, i don't know if it makes sense to cater to them directly.

[btakita]

I missed this. In the spirit of consolidating our discussion, I'll repost the response I made:

I'm not asking svelte to spend time catering to any library, though it's a bonus if Svelte is interoperable with other libraries. As somebody who uses Svelte, I've run into this bug before (internal to Svelte & not caused by outside code) & it resulted in a blank screen & I was unable to reproduce the issue in the repl. It's also difficult to reason about, the stack trace does not lead to the source of the problem (so a binary search of commenting out component logic is necessary to locate the offending code), & leaves the programmer with less confidence in using Svelte.

This issue will come up again & rather than wasting everybody's time with the bug report, reproduction, discussion, we should remove the possibility of it appearing again.

This fix even saves code. Rather than generating n if (detach) conditionals in the output, we would have only 1 if (node.parentNode).

@Conduitry I see your point re: this exception (or warning) providing a cue that something occurred that should not have occurred. I just wonder if one of the core purposes of Svelte is to ensure that detach is called only once per DOM node. I don't think this is a problem that a programmer using Svelte cares about. It has negligible performance implications & does not seem to affect any of the component destroy logic. I understand that unnecessary calls to detach may indicate that optimization needs to occur, though there's probably other ways of determining if unnecessary calls are being made.

To your point, the warning could occur if (!node.parentNode). That would at least remove the blank screen due to the exception. It would still cause a bug report & time to reproduce, which I suspect will yield false positives re: the function of Svelte itself; Meaning the only issue will be without any other problem related to the operation of the software:

"You or another library should not manually remove a DOM element"
"Why not?"
"You just shouldn't"

Other than detach being called on a DOM element already removed, what is the actual problem? If detach being called on a DOM element already removed is not considered a problem, would there be any other problem?

[antony]

@Conduitry I can reproduce the error you showed above. I am getting this when using "btakita/svelte#gh-3113-hotfix". It's definitely an issue. First link I clicked failed in fact. This is definitely not ready to merge yet.

[antony]

Here's a video of the issue, caused by the proposed fix, that I've screenshotted above.

https://vimeo.com/346819413

[antony]

In case it's useful - here's the site with the latest Svelte release 3.6.5 - the site is unnavigable - no links on the entire site work, just throw errors.

https://vimeo.com/346819796

[antony]

In case it is useful - the page append happens far more frequently in full-screen browser on large monitors, and on a mobile phone. it seems to be about 50% of the time when you hit back, the page will append the new page to the bottom of the old one, and the whole site will just break.

This also occurs on Svelte 3.5.4.

[Rich-Harris]

What's the easiest way to reproduce that failure while using this branch?

[antony]

@Rich-Harris which failure are we looking at? they've all gotten a bit conflated.

The easiest way to replicate the detach failure - I believe, is to add some svelte:head meta tags to your pages. You can see what that does on my QA site: https://qa.beyonk.com/uk - not a single page/link works - I am trying to make a very small repro of this but I'm blocked by other issues in doing so at the moment.

In terms of the page-appended-to-page issue - this exists in 3.5.4 and possibly earlier too, but with a much less severity - I think therefore it's not entirely related, but it's also hard to reliably reproduce - I have discovered that my site suffers most because I a paginate when an intersectionobserver is hit in my footer. The way this then causes pages to be appended is as follows:

1. Hit the back button
2. Svelte starts to change page (title changes, url changes), but
3. Svelte scrolls to my previous position on the homepage BEFORE detaching
4. The intersectionobserver is hit and triggers pagination, adding 9 more cards (2.5 screens) to the current page
5. Svelte can't properly detach that newly loaded content and instead appends my homepage to the bottom of the search page.

I think I can recreate the second by a repro adding content to the page on a timer. I'm also working on this.

[Rich-Harris]

Re the argument in favour of consolidating the check — I definitely see the appeal, but there is a performance difference — in a test I ran just now, detaching many nodes from a parent, doing it without a check went 20% faster in Firefox than with the check. In Chrome it was 8%.

But my larger concern, echoing @Conduitry, is about masking bugs. I really would prefer to get to the underlying problem rather than working around it. In Svelte's early days, it would detach every node from its parent during component destruction (rather than just the top-level node), and it was catastrophic for performance — that's the kind of thing that can easily creep back in if we start writing code defensively rather than correctly.

[char]

@Rich-Harris Is there much performance difference between a check and a try/catch?

[Rich-Harris]

@half-cambodian-hacker-man looks like in Chrome, try-catch is on a par with no check. In Firefox, there's a significant try-catch penalty

[antony]

@Rich-Harris here's the simplest possible reproduction of the detach issue:

https://github.com/beyonk-adventures/svelte-detach-issue

Basically just the sapper template with a meta tag added to head - hope this is what you meant.

Working on the append issue now.

[antony]

I can't run that previous repro against this branch because I get the following issue when trying to npm run dev

✗ client
Cannot read property 'children' of undefined
✗ server
Cannot read property 'children' of undefined                                        

[Rich-Harris]

@antony I've merged master into this branch, and navigating away from the index page works in your repro with this branch linked.

I've got half a mind to merge this branch and cut a release, and deal with any additional bugs thereafter. Might be easier to isolate things that way.

[antony]

@Rich-Harris I'm inclined to agree

[evdama]

that fixed the meta tag issue inside head sveltejs/sapper-template#158
I don't see that error anymore, lighthouse now gives a SEO score of 100%