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

Unmounted hooks should run with afterLeave #994

Closed
jods4 opened this issue Apr 20, 2020 · 7 comments
Closed

Unmounted hooks should run with afterLeave #994

jods4 opened this issue Apr 20, 2020 · 7 comments

Comments

@jods4
Copy link
Contributor

jods4 commented Apr 20, 2020

What problem does this feature solve?

unmounted hooks (component, vnode, directives) should run at the same time as afterLeave when an element has a leave transition applied.

Currently unmounted hooks run immediately when vnode is removed from vdom, even though the host removal of dom element is delayed by a leave transition.
You can pass callbacks to run when the leave transition ends with afterLeave.

This is not "intuitive" in the sense that devs will expect that elements are removed from DOM when unmounted runs, which is not the case if there's a leave transition in the parent chain.

In some cases, this breaks encapsulation by forcing components to be aware of the transition and somehow registering an afterLeave callback. This basically means our components are not nicely composable anymore.

As an example, imagine:

  • One component <anim-leave> (could be a <transition leave="...">) used throughout an application to make things disappear nicely;
  • One component <popper-js> (using the library of the same name) that absolutely positions an element next to an anchor.
<anim-leave speed="slow">
  <popper-js side="top">
    <div>Some content!</div>
  </popper-js>
</anim-leave>

An issue arises when you try to compose those two.
You need to clean up some global event listeners with popper.destroy() when you are done with the positionned element.
When doing so, not only does popper remove its event listeners but it also removes all its styling. This would typically be done in an unmounted hook.

Because unmounted hook runs before leave transition has a chance to complete, the styles are removed too early and the animation is ruined (element jumps somewhere totally unrelated).

The only workaround in beta-2 is that popper-js needs to be aware of the transition: its cleanup must be put into transition.afterLeave, which is not clean.

What does the proposed API look like?

no new api

@yyx990803
Copy link
Member

yyx990803 commented Apr 20, 2020

This is expected behavior and is also consistent with Vue 2's behavior. The element is considered unmounted the moment the leave transition starts (or, the leave transition is triggered by the unmount of the element). This conceptually aligns with the fact that once the leave transition starts, the content is considered "dead" and no longer receive any updates.

The fact that Popper removes styling on cleanup is unfortunate, but I don't think that should affect Vue's behavior in this regard. I also don't think it's even necessary to "cleanup" in this case, since event listeners of no longer referenced elements are auto released in modern browsers.

@jods4
Copy link
Contributor Author

jods4 commented Apr 20, 2020

@yyx990803 I feel like you dismissed this issue a bit quickly, without providing guidance for something that is more legitimate than you make it sound.

If unmounted is linked to the vnode lifecycle by design that's fine, then maybe we need another event that's linked to the DOM removal?

Or maybe just change when unmounted is called on directives? Components and vnode events target vdom, that makes sense.

But I'm gonna say it's confusing on a directive. Directives are pretty much geared towards the real dom, to the point that they accept an Element as first parameter and can't modify vnodes.

If we did a poll asking Vue devs if they think el can still be attached to the document when unmounted is called on a directive, I'd be surprised if the majority answers yes.

As for the use-case:

The fact that Popper removes styling on cleanup is unfortunate, but I don't think that should affect Vue's behavior in this regard.

It was an example (of an excellent library that fulfills an important job).
Fact is: if you want to do some cleanup after DOM removal you can't do it reliably and easily.

I also don't think it's even necessary to "cleanup" in this case, since event listeners of no longer referenced elements are auto released in modern browsers.

You have to clean up because it registers some listeners higher up the chain, e.g. to detect scroll in a parent. This is not unique, many libraries do.
Fact is: sometimes you need to clean up.

It's unfortunate that Vue has no way to execute code after an element is removed from DOM.

@yyx990803
Copy link
Member

yyx990803 commented Apr 20, 2020

In this case, I don't understand why Popper.js removes the styling on cleanup. Why can't it only remove the listeners?

What I mean by unfortunate is that Popper.js' cleanup contains both interaction cleanup and visual updates that cannot be separated.

Conceptually in Vue, an unmounted element is forever discarded, so it doesn't make sense to apply visual changes to it. If you rely on specific timing of DOM removal, you have to do it in the transition afterLeave hook.

The reason why a removed hook can be problematic is that it requires Vue to traverse an entire unmounted tree and collect all the removed hooks so that they can be flushed together when the root node is removed. It adds quite a bit of cost to unmounting even when transition is not used.

@jods4
Copy link
Contributor Author

jods4 commented Apr 20, 2020

I think it's good etiquette in good ol' pure JS libraries to leave things as they found it when they have a destroy function. So although it's not helping in this particular case, I'm not surprised by behavior.

I worked around it in Vue already, in a very ninja-way (= not great, very hacky 🤕).
It's also possible to tweak popper to not remove its styles (although not in a straightforward way), maybe I'll do that instead since you won't provide a hook in Vue. Might be cleaner.

A third option that might be better if this happens to someone else with a different lib: use a MutationObserver to know when the element is actually removed.

@houfeng0923
Copy link

houfeng0923 commented Mar 23, 2021

@yyx990803 @jods4 Sorry for appending comments in a closed issue .

Although I do understand component life-cycle considerations, but i also like that 'unmounted' after 'after leave' is "intuitive".

I had a similar problem with Vue2 and Vue3, and had to solve it in other ways. For example:

child component modify overflow style of $el.parentNode ( which bind a transition effect) to 'hidden' , it will restore this modify in unmounted. but with leave transition , the default scroll bar will be shown during transition .

And for functionality of child component , it should be independent of transition . there are many way to resolve it, but some resolution like 'afterLeave' is not apply all scene.

@Stnaire
Copy link

Stnaire commented Apr 9, 2022

Sorry but I really think this topic should be reopened.
The current behavior is not only counterintuitive, it is also blocking in certain scenarios.

The very nature of what being "mounted" or "unmounted" means is not respected here.

Correct me if I'm wrong but a "mounted" component is a component present in the DOM, so the simple fact of having an "unmounted" component in the DOM can't be the expected behavior, it goes against the very meaning of being mounted or unmounted. The same goes for directives of course.

A component transitioning out of the DOM should always be treated as a normal component, like any other (even receive updates etc, no change at all).
It is >> transitioning <<, it is not dismantled yet.

In other words, the transition should be a postponement of the DOM removal.
Instead of unmounting the component immediately, or do whatever has to be done when the v-if receives a falsy expression, it should wait for the transition to finish before doing it.

I also disagree with the principle of having a directive that does not restore the original state of the component when unmounted. It doesn't matter if the browser may do the cleanup or not, it's bad design. You shouldn't rely on external behaviors like that.

I opened a new issue hoping it would be reconsidered, but it was marked as a duplicate with no additional arguments.

I understand that there may be technical difficulties that make the implementation difficult, but I would prefer to know that this is at least something that is considered an issue, or have a valid argument as to why it is not.

@antoniandre
Copy link

Consider this case for some context:

A tooltip component should not be constrained in an overflow: hidden parent, so it should be "teleported" to another place in the DOM (like in the body).

In the context of a UI library, this component would be used passing an activator slot which will trigger showing/hiding the tooltip on hover (by default) of that slot content. Something like this:

<template lang="pug">
tooltip
  template(#activator="{ on }")
    button(v-on="on") button with tooltip
  span Tooltip content
</template>

And the tooltip component would ideally look like this (simplified for explanation):

<template lang="pug">
slot(name="activator" :on="activatorEventHandlers")
transition(:name="transitionName" appear)
  .w-tooltip(v-if="detachableVisible" :style="styles")
    slot
</template>

Since Vue 2 does not support multi-root components, the 2 elements are wrapped into one div and unwrapped into the DOM on mounted:

<template lang="pug">
div.wrap
  slot(name="activator" :on="activatorEventHandlers")
  transition(:name="transitionName" appear)
    .tooltip(v-if="detachableVisible" :style="styles")
      slot
</template>

Then on each user slot hover, the tooltip div would be rendered (data-driven with v-if) and "teleported" manually into the body for instance, and on mouseleave the tooltip would naturally be removed from the DOM by the v-if and change of variable state.

So far so good, but when the component is unmounted, we have to remove the activator slot element that will remain unwrapped in the DOM. Unbinding events would not be sufficient as on each unmount remount and hot reload, the element would be duplicated and uncontrolled as a dead-node.

That's also fine until this element gets used within a transition which unmounts the tooltip before it is removed from the DOM.

Here is the result in action (on Vue 2).

Screen Recording 2022-04-16 at 11 38 27 AM

So I would also vote for calling the beforeUnmount hook from afterLeave when it is ready to be removed from the DOM (when using a v-if). Also this example is showcasing a tooltip as a simple case, but there are more complex cases to handle with the same logic like context menus.

While it works fine on Vue 3 with multiple-root nodes (we don't have to wrap & unwrap on destroyed), I couldn't find a workable way with Vue 2, after trying different approaches:

  • using functional rendering is also limited to a single root node in this case
  • the v-fragments library does the same unwrapping mechanism that I did
  • keeping only the activator slot as a single-root element, is not possible because may contain multiple nodes

So I am considering to write my own transition which would unmount component at the end of the animation, so I don't have to add complexity in all the components that use transitions (like drawers, dialogs, accordions, menus, etc.) and could potentially include this tooltip component.
By "complexity" I am thinking about having 2 variables, and both a v-if and v-show on the same transitioned element.

I could also write the whole tooltip behavior with Vanilla JS, but that would be wasted in a Vue framework, wouldn't it?

I'd love to get your thoughts on this @yyx990803, and maybe you have another suggestion, or maybe that could tilt the balance in favor of triggering unmount from afterLeave. :)

You can view this tooltip component here:
Vue 2: https://github.com/antoniandre/wave-ui/blob/legacy/src/wave-ui/components/w-tooltip.vue
Vue 3: https://github.com/antoniandre/wave-ui/blob/master/src/wave-ui/components/w-tooltip.vue
https://antoniandre.github.io/wave-ui/w-tooltip

And same related issue here: antoniandre/wave-ui#82, where I did this v-if + v-show to fix it: antoniandre/wave-ui@61bc040

@github-actions github-actions bot locked and limited conversation to collaborators Sep 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants