Mechazawa / vue2-teleport

Teleport component for vue2 that works the same as vue3's teleport
Creative Commons Zero v1.0 Universal
30 stars 4 forks source link

fix: Slot nodes are not cleaned up after component destruction. #7

Closed jackiotyu closed 3 months ago

jackiotyu commented 4 months ago

My mistake, this line of code caused the nodes to not synchronize destruction after the teleport component was destroyed. It still works fine after removing it. fix #5 fix #6

eriksaulnier commented 4 months ago

I tested this fix in my codebase and it looks like it works for static content but it still leaves behind orphaned nodes when the child has v-if on it

jackiotyu commented 4 months ago

I tested this fix in my codebase and it looks like it works for static content but it still leaves behind orphaned nodes when the child has v-if on it

Okay, let me try a better way. Can you please try the latest code?

eriksaulnier commented 4 months ago

Just gave it a try with the latest code and it still seems to be orphaning those conditional child nodes :/

jackiotyu commented 4 months ago

Just gave it a try with the latest code and it still seems to be orphaning those conditional child nodes :/

Can you give me some code for reproduction?

eriksaulnier commented 4 months ago

I unfortunately don't have an easy way to share my code because it is so integrated into my project which isn't public. I am doing essentially the same thing as the first snippet in this issue though: https://github.com/Mechazawa/vue2-teleport/issues/3

In my scenario modalOpen defaults to false and the modal is visible when the unmount happens.

Your patch still fixes the main issue so maybe not worth holding back this PR for this edge case?

jackiotyu commented 4 months ago

@eriksaulnier Can you try to reproduce this issue in the live code? stackblitz-vue2

eriksaulnier commented 4 months ago

@jackiotyu This is not quite my setup but I think it demonstrates the problem. If you open and close the modal a couple of times it gets duplicated in the dom because the teleport doesn't move the content back to the original location when it gets disabled. https://stackblitz.com/edit/vue2-vite-starter-dbxubs?file=src%2FApp.vue

jackiotyu commented 4 months ago

@jackiotyu This is not quite my setup but I think it demonstrates the problem. If you open and close the modal a couple of times it gets duplicated in the dom because the teleport doesn't move the content back to the original location when it gets disabled. stackblitz.com/edit/vue2-vite-starter-dbxubs?file=src%2FApp.vue

Okay, I understand what you mean now. I'll do my best to fix it.

jackiotyu commented 4 months ago

@eriksaulnier Please try this out. I believe it's working correctly now.πŸ˜€ https://stackblitz.com/edit/vue2-vite-starter-lupose

eriksaulnier commented 4 months ago

@jackiotyu Nice! It looks like that example works great now! I am still having problems with my exact scenario but I can keep tinkering with it and see if I can get a reproduction in stackblitz. All I've been able to figure out is that for some reason, when it hits your new code for removing old nodes, the only node is an instance of #comment. Perhaps something to do with the transition I'm using.

<teleport to="body">
        <transition
            @before-enter="setupTooltip"
            @after-leave="cleanupTooltip"
        >
            <div
                v-if="isOpen"
                ...
image
jackiotyu commented 4 months ago

@jackiotyu Nice! It looks like that example works great now! I am still having problems with my exact scenario but I can keep tinkering with it and see if I can get a reproduction in stackblitz. All I've been able to figure out is that for some reason, when it hits your new code for removing old nodes, the only node is an instance of #comment. Perhaps something to do with the transition I'm using.

<teleport to="body">
        <transition
            @before-enter="setupTooltip"
            @after-leave="cleanupTooltip"
        >
            <div
                v-if="isOpen"
                ...
image

Did it report any errors? Was it unable to find the parentNode? Please try this.πŸ˜€ https://stackblitz.com/edit/vue2-vite-starter-lupose

eriksaulnier commented 4 months ago

@jackiotyu Nice! It looks like that example works great now! I am still having problems with my exact scenario but I can keep tinkering with it and see if I can get a reproduction in stackblitz. All I've been able to figure out is that for some reason, when it hits your new code for removing old nodes, the only node is an instance of #comment. Perhaps something to do with the transition I'm using.

<teleport to="body">
        <transition
            @before-enter="setupTooltip"
            @after-leave="cleanupTooltip"
        >
            <div
                v-if="isOpen"
                ...
image

Did it report any errors? Was it unable to find the parentNode? Please try this.πŸ˜€ https://stackblitz.com/edit/vue2-vite-starter-lupose

There are no errors thrown and no difference when trying your latest patch 😒

It seems like from the first point where the node list is constructed, in mounted, it is being set to an instance of #comment and doesn't change to anything else. I'm guessing #comment must be a vue placeholder of some sort.

eriksaulnier commented 4 months ago

It seems like your patch works without the transition so that must be what is causing trouble.

jackiotyu commented 4 months ago

It seems like your patch works without the transition so that must be what is causing trouble.

I'm sorry, I didn't test so many scenarios in my previous code.πŸ˜” @Mechazawa Please consider merging this. If possible, these codes can further improve performance and address existing issues

eriksaulnier commented 4 months ago

It seems like your patch works without the transition so that must be what is causing trouble.

I'm sorry, I didn't test so many scenarios in my previous code.πŸ˜” @Mechazawa Please consider merging this.

I am also in favor of merging this!

I did finally manage to get a repoduction link and this problem happens with the old version of this package pre-childObserver so is an entirely different issue https://stackblitz.com/edit/github-oecgmy

Sorry for leading you down a bit of a rabbit-hole.

jackiotyu commented 4 months ago

It seems like your patch works without the transition so that must be what is causing trouble.

I'm sorry, I didn't test so many scenarios in my previous code.πŸ˜” @Mechazawa Please consider merging this.

I am also in favor of merging this!

I did finally manage to get a repoduction link and this problem happens with the old version of this package pre-childObserver so is an entirely different issue stackblitz.com/edit/github-oecgmy

Sorry for leading you down a bit of a rabbit-hole.

I think this might be the solution. https://stackblitz.com/edit/github-oecgmy-8uhm7d?file=src%2Fcomponents%2FTeleport.vue

Maybe can simplify many code and still work corrently.πŸš€ https://stackblitz.com/edit/vue2-vite-starter-lupose

eriksaulnier commented 4 months ago

The latest patch works great!

eriksaulnier commented 3 months ago

@Mechazawa can you merge and release this?

Mechazawa commented 3 months ago

Published as v1.1.4