sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.53k stars 4.12k forks source link

$destroy don't play outro transition #4056

Open dishuostec opened 4 years ago

dishuostec commented 4 years ago

Describe the bug When call $destroy of a component instance, the dom node is removed imediatly without play outro transition.

Logs nothing

To Reproduce https://svelte.dev/repl/a9e6f228bf40490c843c639ef4288f38?version=3.16.0

Expected behavior Play outro transition before remove.

Conduitry commented 4 years ago

I suppose this would be a proposal for an outro boolean option to the method, analogous to the intro option in the constructor.

dishuostec commented 4 years ago

We should explain this feature in the documentation, otherwise more people may think this is a bug like me.

silentworks commented 4 years ago

But if this component was used inside of another Svelte component itself, wouldn't you instantiate it differently? as in have Svelte deal with the lifecycle hooks for you, rather than manually calling them? Because Svelte will handle the transitions accordingly this way.

https://svelte.dev/repl/767db42208ad40efa56976f645e28ab8?version=3.16.0

Conduitry commented 4 years ago

I don't think you typically would be manually instantiating or destroying a component inside of another component - that's just what's required in the REPL. Having a way to play outro transitions while manually managing a component (the way you can have it play intro transitions) seems like a reasonable request.

PatrickG commented 3 years ago

For everyone who needs a workaround for this:

import type { SvelteComponent } from 'svelte';
import { check_outros, group_outros, transition_out } from 'svelte/internal';

// Workaround for https://github.com/sveltejs/svelte/issues/4056
const outroAndDestroy = (instance: SvelteComponent) => {
  if (instance.$$.fragment && instance.$$.fragment.o) {
    group_outros();
    transition_out(instance.$$.fragment, 0, 0, () => {
      instance.$destroy();
    });
    check_outros();
  } else {
    instance.$destroy();
  }
};

Instead of calling instance.$destroy(); you can call outroAndDestroy(instance);.

russellsamora commented 3 years ago

@PatrickG could you provide a quick repl to show how you might use this?

dishuostec commented 3 years ago

@PatrickG Thanks, it's works pretty good.

@russellgoldenberg You can try this one: https://svelte.dev/repl/c77471ac6a98495a84d402d106beddcc?version=3.16.0

pikeas commented 3 years ago

Just bit by this (sveltekit v1.0.0-next.71). I have some transitions on a page, and the out transition is firing when navigating to a new page. This makes navigation feel slow, as there's an additional x00ms delay while the transition finishes.

This is somewhat unexpected - I did not expect component outro transitions to fire on nav. I can see how this is useful for top-level components (full page screen wipes and other effects), but it seems like the exceptional case rather than the default case.

Interestingly, intro transitions don't seem to delay nav, only outros.

PatrickG commented 3 years ago

Just bit by this (sveltekit v1.0.0-next.71). I have some transitions on a page, and the out transition is firing when navigating to a new page. This makes navigation feel slow, as there's an additional x00ms delay while the transition finishes.

This is somewhat unexpected - I did not expect component outro transitions to fire on nav. I can see how this is useful for top-level components (full page screen wipes and other effects), but it seems like the exceptional case rather than the default case.

Interestingly, intro transitions don't seem to delay nav, only outros.

That is not related to this issue.

johnnysprinkles commented 2 years ago

I'd rather not dig around in the Svelte internals. My workaround is to take a "destroyed" property and not literally destroy it but wrap all its markup in an {#if}.

So it's like:

    let toast = new Toast({
      target: document.body,
      props: {
        text,
      },
      intro: true,
    });
    setTimeout(() => {
      toast.$set({ destroyed: true });
    }, 5000);

Would love to just have an "outro" option though.