Skip to content
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

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

Open
Tharit opened this issue Aug 13, 2020 · 22 comments

Comments

@Tharit
Copy link

Tharit commented Aug 13, 2020

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:

  • Your browser and the version: Chrome 84.0.4147.125 (Official Build) (64-bit)

  • Your operating system: OSX 10.15.5

  • Svelte version: 3.24.1

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
Copy link
Author

Tharit commented Aug 13, 2020

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
Copy link

vsych commented Aug 16, 2020

+1

@tanhauhau
Copy link
Member

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
Copy link

M1sf3t commented Aug 26, 2020

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
Copy link

M1sf3t commented Aug 27, 2020

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
Copy link
Contributor

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

outros.c.push(() => {
outroing.delete(block);
if (callback) {
if (detach) block.d(1);
callback();
}
});

Calling transition_out on a block adds it to a Set, so further transition_out calls on the same block are ignored

export function transition_out(block, local: 0 | 1, detach: 0 | 1, callback) {
if (block && block.o) {
if (outroing.has(block)) return;

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
Copy link

M1sf3t commented Aug 28, 2020

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.

orange4glace added a commit to orange4glace/svelte that referenced this issue Sep 4, 2020
@the0neWhoKnocks
Copy link

the0neWhoKnocks commented May 9, 2021

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
}

@FractalHQ
Copy link
Member

FractalHQ commented May 21, 2021

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);

simonwiles added a commit to sul-cidr/pianolatron that referenced this issue Sep 15, 2021
This is bit of a kludge to address the problem of the `<RollViewer/>` component getting into an inconsistent state if it's not destroyed properly when a new roll is selected.  The problem is caused when `<RollViewer/>`s lifecycle `onDestroy` method is not called, which can happen when it's removed while one or more of its children are in the middle of a `transition` (see sveltejs/svelte#5268).

One option would be to remove all Svelte `transition`s on child elements (the "Downloading roll image..." message, `<RollViewerControls/>`, and `<RollViewerScaleBar/>`) but this seems a shame.  In my testing, this fix seems to address the problem without needing to remove the `transition`s.
@stale
Copy link

stale bot commented Nov 17, 2021

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.

@stale stale bot added the stale-bot label Nov 17, 2021
@simonwiles
Copy link

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

@stale stale bot removed the stale-bot label Nov 17, 2021
simonwiles added a commit to sul-cidr/pianolatron that referenced this issue Jan 18, 2022
Destroying and re-mounting the right sidebar (in particular) when the roll is changed can cause problems if child components aren't destroyed properly due to outro transitions (see sveltejs/svelte#5268).  This has caused a number of bugs with the `<RollViewer/>` component not being recreated properly.

One option is to eschew all outro transitions in other parts of the app; but I think a better solution in this specific case is to avoid destroying and recreating this part of the component tree unless absolutely necessary.
@itay-grudev
Copy link

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

@JoaoCardoso193
Copy link

Please fix this, it is incredibly frustrating to deal with

@kwhitley
Copy link

kwhitley commented Sep 2, 2022

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...

SymphonySimper added a commit to SymphonySimper/note-ie that referenced this issue Oct 8, 2022
for more info onDestroy not getting called refer 'sveltejs/svelte#5268'
add alias fro stores -> $stores
format imports
@seo-rii
Copy link

seo-rii commented Oct 22, 2022

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

@kuechlerm
Copy link

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
Copy link

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
Copy link

d-velopment commented Dec 4, 2022

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.

@ofeenee
Copy link

ofeenee commented Feb 5, 2023

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
Copy link

gmbeal commented Apr 20, 2023

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!

moufmouf added a commit to workadventure/workadventure that referenced this issue Apr 26, 2023
Because of a bug in Svelte (see sveltejs/svelte#5268), the onDestroy callback of sub-components are not called if one of the parent components has a transition.

This in turn caused stores to not be unsubscribed, and ultimately, a "setInterval" to read the microphone audio level not to be cleared.

We fix this by removing any Svelte based transitions/animations.
moufmouf added a commit to workadventure/workadventure that referenced this issue Apr 26, 2023
Because of a bug in Svelte (see sveltejs/svelte#5268), the onDestroy callback of sub-components are not called if one of the parent components has a transition.

This in turn caused stores to not be unsubscribed, and ultimately, a "setInterval" to read the microphone audio level not to be cleared.

We fix this by removing any Svelte based transitions/animations.
@Vibeesarma
Copy link

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

@yeori
Copy link

yeori commented Aug 13, 2023

Same issue what I've faced and solved.

  • svelte 3.59.2
  • not sveltekit

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>
  • BottomSheet and child component were destroyed as expected. (onDestroy(() => {..})

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

No branches or pull requests