fix(runtime-core): ensure custom events are not emitted anymore after unmount. (fix #5674)

[LinusBorg]

This change ensures that calling emit after the component has unmounted will no longer call any of the registered custom event listeners.

Considerations

Is it possible/realistic that we have implementations out there that rely on the current behavior where this event would still be triggered?

If so, how do we approach this?

close: #5674

[netlify]

✅ Deploy Preview for vuejs-coverage ready!

Name	Link
🔨 Latest commit	ca327e0
🔍 Latest deploy log	https://app.netlify.com/sites/vuejs-coverage/deploys/624e860b5d8aef0008b57c7e
😎 Deploy Preview	https://deploy-preview-5679--vuejs-coverage.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[netlify]

✅ Deploy Preview for vue-sfc-playground ready!

Name	Link
🔨 Latest commit	ca327e0
🔍 Latest deploy log	https://app.netlify.com/sites/vue-sfc-playground/deploys/624e860b34d5200008f74560
😎 Deploy Preview	https://deploy-preview-5679--vue-sfc-playground.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[netlify]

✅ Deploy Preview for vue-next-template-explorer ready!

Name	Link
🔨 Latest commit	ca327e0
🔍 Latest deploy log	https://app.netlify.com/sites/vue-next-template-explorer/deploys/624e860b4f67ed0008af4003
😎 Deploy Preview	https://deploy-preview-5679--vue-next-template-explorer.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[bbotto-pdga]

Apologies if I am misunderstanding this change, @LinusBorg, but at a glance it doesn't look like it entirely fixes the problem I reported. It looks like this prevents a component from emitting events after it's been unmounted. But what about unmounted components handling events? Shouldn't event listeners be removed between the beforeUnmount and unmounted lifecycle phases?

I've updated one of the StackBlitz snippets that I provided to show another example of the issue. I added a custom element (CustomEle.js) that dispatches a "delayed" event 5 seconds after it's constructed. The event handler is wired up in AlertList.vue. Note that if the AlertList is unmounted before delayed fires, the handler still executes. (The "Toggle Alerts" button unmounts the AlertList.)

https://stackblitz.com/edit/vue-kurpoq?file=src%2Fcomponents%2FAlertList.vue

To add some context, the issue that we're experiencing is with a UI library that dispatches events asynchronously after animations complete. We also use Vue Router, and sometimes our users navigate away from a page in the middle of an animation. When that happens, we're seeing event handlers run in unmounted components, and that causes crashes if those handlers depend on injected dependencies, or $refs, etc.

[LinusBorg]

Thanks for checking this PR! :)

But what about unmounted components handling events?

That should not be an issue as child components are unmounted before the parent.

Shouldn't event listeners be removed between the beforeUnmount and unmounted lifecycle phases?

Event listeners are part of the vnode props. to remove them cleanly, we would have to run another patch run (re-ender), or remove them manually which feels icky.

Preventing events from being emitted should have the same effect and be a bit cleaner.

I added a custom element (CustomEle.js) that dispatches a "delayed" event 5 seconds after it's constructed.

Thanks for this, good catch! This is because defineCustomElement replaces the original emit function so it can instead emit a CustomEvent.

I forgot to apply the fix to that part of the codebase as well, will fix that!

[bbotto-pdga]

Thanks, @LinusBorg, for helping with this issue.

This is because defineCustomElement replaces the original emit function so it can instead emit a CustomEvent.

I forgot to apply the fix to that part of the codebase as well, will fix that!

I'll take your word for it on this one--I don't know enough about the Vue code base. I'll throw it out there, and sorry if I'm being naive, that the custom element I made was defined using the native defineCustomElement, not Vue's, and it uses the native dispatchEvent, not emit. Using a custom element was just a quick example that I threw together, but it could be anything that dispatches an event after the parent component is unmounted. (In the code that I'm working on, it's UIkit that is dispatching custom events after animations complete.)

Here's another example that illustrates the problem without using a custom element. Click "Toggle Menu" within 5 seconds and not that the event handler in RemovableMenu runs after it is unmounted.

https://stackblitz.com/edit/vue-72sys8?file=src%2FApp.vue

[LinusBorg]

the custom element I made was defined using the native defineCustomElement

It seems I wasn't reading your example carefully enough, my apologies. Coincidentally, my point is still something I need to address in this PR, so still: Thanks for the (involuntary) pointer.

Here's another example that illustrates the problem without using a custom element. Click "Toggle Menu" within 5 seconds and not that the event handler in RemovableMenu runs after it is unmounted.

stackblitz.com/edit/vue-72sys8?file=src%2FApp.vue

What you are no asking here, basically, is that when unmounting, Vue should walk the element tree and remove all event listeners one by one, even though the elements would normally be garbage-collected anyway, so that you don't have to clean up the async side-effect you created yourself. (Of course a full tree walk would not strictly be necessary, it could be optimized, but it would come at a cost in terms of implementation and additional work at runtime one way or another).

You created a timeout that you fail to cancel when your component unmounts. That should be the correct way to solve the problem.

Us removing all the individual listeners would only solve this specific issue with this more general issue: Side-effects, introduced by the developer, that leak out of the component lifecycle.

Instead of an element event listener as in your example, it could be a fetch request, or a DOM selector query, or any of a million other things happening in that timeout callback that will introduce bugs if you let the timeout run beyond the lifecycle of your component it is meant to work inside of.

Here's two ways to write your example in a proper way:

1. Clean up the side-effect when unmounting

mounted() {
  const menu = document.querySelector('.removable-menu');
  this.id = setTimeout(() => menu.dispatchEvent(new Event('delayed')), 5000);
},
beforeUnmount() {
  this.id && clearTimout(this.id)
}

2. safeguard the side-effect in the callback to only run when appropriate, i.e. here, the element is still in the DOM.

mounted() {
  const menu = document.querySelector('.removable-menu');
  this.id = setTimeout(() => {
    if (menu.isConnected) {
      menu.dispatchEvent(new Event('delayed')), 5000);
    }
  }
},

So in short: Yes, removing all event listeners one by one during unmount would provide a little safeguard in very specific scenarios, but is not worth the perf cost IMHO. Whereas the little safeguard concerning component events that I propose in this PR, is practically free.

I understand that your underlying issue is specific to one component (-library). We can talk about what to do about the specific problem on discord (chat.vuejs.org) if you want. This PR would not be the place for that.

[bbotto-pdga]

Instead of an element event listener as in your example, it could be a fetch request, or a DOM selector query, or any of a million other things happening in that timeout callback that will introduce bugs if you let the timeout run beyond the lifecycle of your component it is meant to work inside of.

Fair enough. We've already worked around the issue in our code by manually adding and removing event listeners in mounted/beforeUnmount instead of wiring up listeners in templates via vOn. I thought that this was a bug because the Vue 2 Lifecycle Diagram explicitly says "Teardown ... event listeners" between beforeDestroy and destroyed, whereas the Vue 3 diagram does not.

At any rate, thanks again for the help. The change you made in this PR does fix some of the issues that we're experiencing in our codebase.

[funkyvisions]

@LinusBorg
This fix is breaking me. I have 3 nested components (the top page is under a keep-alive) that emit a custom event (for a back button). I see Component3 emit to Component2 and I see Component2 try to emit to Component1, but it's stopped by this code.

if (instance.isUnmounted)
    return;

Component1 (page - keep-alive - emit handler is never called, so this page is stalled -- can't go back)
    Component2 (toolbar component - emit) - this get blocked
        Component3 (button component - emit)

I'm not sure what to do. Do I not use emit and just use a "function" prop? Unfortunately I haven't been able to consistently reproduce (thus no link to provide yet). I just have to play around with my app until the back button stops working.

UPDATE: I patched my vue install to remove the return and just log a message. Now I see

emit
   emit
      console log
         emit handler

And everything works properly.

[LinusBorg]

I fail to imagine a scenario where the inner component would need to emit a close event, while its parent (the middle component) is already (being) unmounted - qnd yet somehow your page gets stuck.

so I'll wait for your reproduction

When you have that ready, please open a new issue, thanks.

[funkyvisions]

@LinusBorg I'll try to yank all this out of my app and create a reproducible snippet. In the meantime, what it the harm of me patching my vue install to remove those added lines?

[LinusBorg]

Not much, i guess. This patch covered mote of an edge case I'd say from what i recall.

[funkyvisions]

@LinusBorg I've create issue #7355