sveltejs / svelte

web development for the rest of us
https://svelte.dev
MIT License
79.8k stars 4.24k forks source link

onDestroy not called for nested component removed during parent nodes exit transition #5268

Closed Tharit closed 2 weeks ago

Tharit commented 4 years ago

Describe the bug

Components are not destroyed when they are removed from their parent node while that node is currently running its exit transition.

Example: I have a component in a div that has a fade transition, and I remove the component while that div is fading out. The component is not destroyed correctly. It is gone from the DOM, but still "alive" (store subscriptions not closed, onDestroy not called, etc).

To Reproduce

I have created a small REPL example that reproduces the issue.

Note how the console only logs the onDestroy call for the Outer component, but NOT the inner component when you click on the "Close" button. If you remove "transition:fade" in App.svelte:9 then everything works correctly.

https://svelte.dev/repl/eff9e77c7d5c41059616211e46f3f021?version=3.24.1

The example is a bit artificial of course. In our actual application we have a lot of stores subscribed to realtime updates via websocket. It can frequently happen that you switch/navigate between different content (using transitions) and an update just happens to arrive for the "old" content while the transition is active.

Expected behavior onDestroy should be called / component should be destroyed when using transitions.

Information about your Svelte project:

Severity

Now that I managed to trace down the cause of this issue it is not that severe, as it's easy to work around; it just blocks us from using transitions / some third party components that rely on them. However before I managed to isolate it was extremely frustrating as it appeared as seemingly "random" behaviour with some components not unmounting / resulting in dangling store subscriptions and associated weirdness.

Tharit commented 4 years ago

It seems like the bug is not only triggered if the transition is on the parent; it's enough if the parent is waiting for some child to finish its exit transition. See another REPL here: https://svelte.dev/repl/9ac8610241a643ec81432f72f3427baa?version=3.24.1 Again, if you remove the transition from the div it works.

vsych commented 4 years ago

+1

tanhauhau commented 4 years ago

I hvn't wrap my head around a fix on this, but from what I observe is that, if the parent starts to be unmount first, then the Inner onDestroy did not get called.

a workaround would be moving the on:close after on:close={() => mode='B'}

<Inner on:close={() => mode='B'} on:close ></Inner>

https://svelte.dev/repl/808a7d0e9f704941abf1ba084b846d74?version=3.24.1

M1sf3t commented 4 years ago

think i've been fighting with this same bug for a few days now. I'm including a screenshot that shows the hide variable being logged as true, then the action running (timer should've been destroyed on true), then the hide variable being set back to undefined, yet the carousel doesn't re-render. The last part doesn't happen consistently, but it happens enough to be troublesome.

Screenshot_2020-08-26 Svelte REPL

https://svelte.dev/repl/ab82d572c40d482c92cc211affbc290d?version=3.24.1

M1sf3t commented 4 years ago

updated that repl so that the carousel actually worked after the first slide. when i moved the "slide" declaration outside of the set interval, the destroy method for the action broke it 🤦‍♂️.

The general problem is still not solved tho, neither the onDestroy method nor the actions destroy method seem to have any affect on the delay timer when the component is hidden with if. To see the latter in the repl, remove comments, slide declr. & onDestroy method and declare slide inside of interval.

I think I've managed a workaround using svelte component instead of the if statement, so far it seems to be working, tho at the moment there's occasionally a jump when it renders. you can see the workaround here --> https://svelte.dev/repl/40741fc76d52405688e8705ec26c254c?version=3.24.1

pushkine commented 4 years ago

duplicate of #4696 and #4064, kinda explained but not fixed in #4699

The internal function transition_out is used in two ways, one is to start outro, the other is to start outro and schedule a destroy after the transition group has outroed https://github.com/sveltejs/svelte/blob/8adb47401e7f7b420ffabf9752a8236114aaecfc/src/runtime/internal/transitions.ts#L56-L62

Calling transition_out on a block adds it to a Set, so further transition_out calls on the same block are ignored https://github.com/sveltejs/svelte/blob/8adb47401e7f7b420ffabf9752a8236114aaecfc/src/runtime/internal/transitions.ts#L51-L53

From there you can deduce that any transition_out used in the first way makes it so that any destroy scheduled through the second way gets ignored, that is what happens in all of the above when an if_block is outroed by its parent's .o() then outro & destroy scheduled by its parent's .p()

M1sf3t commented 4 years ago

that makes sense but why does it work differently with keyed each and svelte:component? in the last repl I posted, the transition continues to fade out but toggling the svelte component to rerender the carousel at the right moment doesn't break the carousel like it did with the if in the first repl. At least I haven't been able to break it so far anyway.

Same with each, when i first started on the carousel I felt like keyed each would be too much for just one image so I went with if and found that when not timed correctly (using interval/onMount rather than use/timeout at that point, needed to be somewhat around one second after the out ended) the same image would just reappear over and over again.

The latter may not be the exact situation, but it felt like it was a similar issue. Easy enough to fix when it was just that, but as I got to playing with different ways to load the images and swap them out with my spinner, it created more and more problems so I finally just swapped the if out with the keyed each and moved on.

the0neWhoKnocks commented 3 years ago

Not sure if it's the same thing, but onDestroy isn't firing for me when I use custom in/out transitions. This may be by design, and if that's the case, it may be helpful if the example is updated to call that out.

props:

in:toggleDialog="{{ dir: 'in', start: 70 }}"
out:toggleDialog="{{ start: 50 }}"

What I ended up doing within toggleDialog

if (dir !== 'in' && t === 0 && onClose) onClose();

Edit: I discovered that while working with some stores, that the above didn't actually work. I didn't fully understand what was actually occurring when a custom transition function was called (it builds out a static CSS animation); I was thinking it was calling that method during the animation steps, lesson learned.

Here's the current solution I'm using:

props

in:toggleDialog="{{ dir: 'in', start: 70 }}"
out:toggleDialog="{{ start: 50 }}"
on:outroend={handleCloseEnd}

handler

function handleCloseEnd() {
  dialogOpen.set(false); // global store to ensure multiple dialogs aren't open
}
braebo commented 3 years ago
old comment I currently have a project with lots of transitions and noticed this bug randomly occurs when navigating between pages. There will randomly be invisible dom nodes stacking up on pages, pushing the current page content way down. It more or less renders the website unusable. What is the recommended fix currently? I noticed @tanhauhau and @the0neWhoKnocks's fixes might help. Considering the amount of elements, it will be non-trivial to wrap every single component in it's own `{#if}` block and trigger it `on:outroend` (if I understand the fix correctly). But if that's the best way, so be it. Project deadline is coming up 😅 I think this issue should get the `bug` label, because it's pretty devastating to large projects with transitions. PS: I found myself in the situation when we had to introduce an extra store to handle state on page transitions- which I assume is what caused all elements with transitions in the project to start showing this bug.

Edit: It appears my issue was actually caused by the uncaught error mentioned in #6037.

It's a bit early to know for sure, but changing line 203 in svelte/internal/index.js as mentioned in that issue seems to have fixed things.

- node.parentNode.removeChild(node);
+ if (node.parentNode) node.parentNode.removeChild(node);
stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

simonwiles commented 2 years ago

This is still a big problem and should not be closed, imo.

itay-grudev commented 2 years ago

Please prioritize. This is a major pain in the ass.

JoaoCardoso193 commented 2 years ago

Please fix this, it is incredibly frustrating to deal with

kwhitley commented 2 years ago

Encountering this now myself...

I have pages nested within a transitioning block, and they completely fail to trigger onDestroy... furthermore (or as a result), any beforeNavigate calls fail to get unsubscribed/removed, resulting in an ever-growing series of calls as these components continue to be loaded and never disposed of.

Not quite sure how to tackle this issue...

seo-rii commented 2 years ago

Any update about this? It's one of the most critical bug, and I couldn't find any solution to bypass it.

kuechlerm commented 1 year ago

This just cost me the whole day.

I tried to create an example that that is as simple as possible, where you can see the problem, that a component does not get removed from the DOM even if the #if block should clear it:

https://svelte.dev/repl/01e58ca1ea4241e6acadb3011f126008?version=3.53.1

I can only hope that this 2 year old problem can be addressed. Or am I using Svelte here in a very strange way?

The solution for a simple setup could be something like this: https://svelte.dev/repl/6fcf04846d3b4596a5799580e8c467eb?version=3.53.1

BUT the problem could be nested, that is, some nested component down the chain could have a transition and then it is hard to catch the problem.

kuechlerm commented 1 year ago

This just cost me some hours. Am I using svelte wrong? This seems to be a very concerning problem.

I tried to add another example that is hopefully even simpler: https://svelte.dev/repl/6fcf04846d3b4596a5799580e8c467eb?version=3.53.1

For very simple setups this might be a workaround: https://svelte.dev/repl/75bf4eaddc7e4b3ea0a24f184e8290b3?version=3.53.1

d-velopment commented 1 year ago

Solution, indicated @ https://svelte.dev/repl/6fcf04846d3b4596a5799580e8c467eb?version=3.53.1 is good for simple projects.

Impossible (read: Hard-to-manage) to use in a more complex projects where the one component, dynamically loaded into another dynamically loaded component. Svelte should definitely be able to sort out transition things on its level without any additional code due to support of such dynamiс components loading.

Now using an npm patch over Svelte simply to try-catch errors, not solved the problem.

ioucyf commented 1 year ago

I'm having the same problem, and so many components fail to clean up after them because onDestroy is not being called, and causes the webapp to become slower as the end user navigates and uses the app.

This is extremely annoying. 😔

Has anyone not yet figure out the issue that causes this to happen?

gmbeal commented 1 year ago

Not sure if we were experiencing the same issue but we had nested child components in an if statement that weren't being destroyed but only in the case when a drop down menu with the fade transition was present on the DOM. By adding the |local attribute to the fade transition our problem went away.

transition:fade|local={{ duration: 140 }}

Note the transition was not on the nested child component but only the drop down menu that covered the nested child when open. Hope this applies to some of you!

Vibeesarma commented 1 year ago

same issue I am also facing I remove the transition and add a custom transition it worked.

yeori commented 1 year ago

Same issue what I've faced and solved.

Situation

I have two components, BottomSheet, and nested child component(dymamically nested into the BottomSheet)

bottom sheet: https://m3.material.io/components/bottom-sheets/overview

// BottomSheet.svelte
<section
  style="--z-index: {zIndex}"
  transition:fly={bottom: { x: 0, y: 40, duration: 140 }}
>
 <div class="inner" bind:this={innerEl}>
     <!-- slot for child component -->
      <slot/>
  </div>
</section>

BotomSheet is registered globally at the root component

// MainApp.svelte

<main>
  {#if $sheetStore.activeSheet}
    <BottomSheet zIndex={$sheetStore.activeSheet.zIndex}>
      <!-- any child component is dynamically bound -->
      <svelte:component
        this={$sheetStore.activeSheet?.component}
        {...$sheetStore.activeSheet?.props}
      />
    </BottomSheet>
  {/if}
</main>

I found a child component instance of WeatherWidget.svelte was not destroyed when BottomSheet was destroyed, which was because of transition in BottomSheet.svelte(without transition in BottomSheet, WeatherWidget.onDestroy was called).

Workaround

two fixes.

  1. removing<slot/> and injecting child component inside BottomSheet.svelte
  2. makes traition local(It is not necessarily required. Try it if the problem continues)
// MainApp.svelte
<main>
  {#if $sheetStore.activeSheet}
    <!-- don's inject child here-->
    <BottomSheet sheetSpec={$sheetStore.activeSheet}></BottomSheet>
  {/if}
</main>
// BottomSheet.svelte
<script lang="ts">
/**
 * holds dynamic child component and its props
 */
export let sheetSpec:SheetSpec
</script>
<!-- local transition(optional workaround) -->
<section
  style="--z-index: {zIndex}"
  transition:fly|local={{ x: 0, y: 40, duration: 140 }}
>
 <div class="inner">
     <!-- removing slot and inject child here -->
     <svelte:component this={sheetSpec.component} {...sheetSpec.props} />
  </div>
</section>
dummdidumm commented 2 weeks ago

This is fixed in Svelte 5