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

fix(runtime-dom): fix anchor loss caused by unmount #9134

Closed

Conversation

Alfred-Skyblue
Copy link
Member

@Alfred-Skyblue Alfred-Skyblue commented Sep 5, 2023

fixed #9071
fixed #8105

unmount(n1, parentComponent, parentSuspense, true)

Before executing 'unmount,' the 'anchor' was obtained. After 'unmount' completes, it is possible that the 'anchor' has been removed, causing an error when trying to find the 'anchor' during a subsequent 'mount.' This scenario only occurs when the 'anchor' is the last node. Therefore, using 'parent.appendChild(child)' for mounting is recommended.

@github-actions
Copy link

github-actions bot commented Sep 5, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 87.2 kB (+21 B) 33.1 kB (+13 B) 29.9 kB (+9 B)
vue.global.prod.js 133 kB (+21 B) 50 kB (+9 B) 44.8 kB (+56 B)

Usages

Name Size Gzip Brotli
createApp 48.4 kB (+23 B) 19 kB (+8 B) 17.4 kB (-49 B)
createSSRApp 51.6 kB (+23 B) 20.3 kB (+10 B) 18.5 kB (+8 B)
defineCustomElement 50.8 kB (+23 B) 19.8 kB (+9 B) 18.1 kB (-27 B)
overall 61.8 kB (+23 B) 23.9 kB (+10 B) 21.7 kB (-3 B)

@Alfred-Skyblue
Copy link
Member Author

If we obtain the anchor after unmount, it may lead to rendering errors with defineAsyncComponent

Copy link
Member

@baiwusanyu-c baiwusanyu-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a unit test

@baiwusanyu-c baiwusanyu-c added need test The PR has missing test cases. ready for review This PR requires more reviews labels Sep 5, 2023
@sxzz sxzz removed the need test The PR has missing test cases. label Sep 6, 2023
Copy link
Member

@baiwusanyu-c baiwusanyu-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@coreyworrell
Copy link

coreyworrell commented Sep 22, 2023

Testing this locally after coming across #8105 and the fix doesn't seem to work in my case. I'm still getting an error: Uncaught TypeError: vnode is null with a trace ending at move webpack://site/./node_modules/@vue/runtime-core/dist/runtime-core.esm-bundler.js?:6193, which is called by that unmount function you included above.

My app:

<script setup>
import { useFrameStore } from '../stores/frame.js'

const store = useFrameStore()

// store.frameTransition is either 'slide-up' or 'slide-down'
</script>

<template>
    <div>
        <Transition :name="store.frameTransition" mode="out-in" appear>
            <KeepAlive>
                <Suspense>
                    <component :is="store.frame"></component>
                    <template #fallback>
                        <p>Loading...</p>
                    </template>
                </Suspense>
            </KeepAlive>
        </Transition>
    </div>
</template>

<style lang="stylus">
.slide-up-enter-active,
.slide-up-leave-active,
.slide-down-enter-active,
.slide-down-leave-active {
    transition-property: opacity, transform;
    transition-duration: .25s;
    transition-timing-function: cubic-bezier(.7, 0, .265, 1);
}

.slide-up-enter-from,
.slide-up-leave-to,
.slide-down-enter-from,
.slide-down-leave-to {
    opacity: 0;
}

.slide-up-enter-from,
.slide-down-leave-to {
    transform: translateY(5%);
}

.slide-up-leave-to,
.slide-down-enter-from {
    transform: translateY(-5%);
}
</style>

@Alfred-Skyblue
Copy link
Member Author

@coreyworrell Can you put the use case into the Playground to help me better troubleshoot the issue?

@coreyworrell
Copy link

@Alfred-Skyblue nevermind, I think my issue was resolved by adding a :key= to the default slot in Suspense.

@edison1105
Copy link
Member

edison1105 commented Sep 28, 2023

This PR seems not the proper fix.

@pikax
Copy link
Member

pikax commented Oct 13, 2023

The error is not present but the behaviour is incorrect:
playground

You can see that the Component does not change from A to B

@pikax pikax removed the ready for review This PR requires more reviews label Oct 13, 2023
@Alfred-Skyblue
Copy link
Member Author

The error is not present but the behaviour is incorrect: playground

You can see that the Component does not change from A to B

In issue #8105, there is an error behavior where an error occurs when the target node has only one child node. From the perspective of animation behavior, it seems to be waiting for the completion of the nextTick execution to switch, rather than waiting for the animation to complete. I'm not sure if this behavior is expected.

@lk77
Copy link

lk77 commented Nov 29, 2023

to which vue version we should rollback to to fix this issue ?

@TomasSestak
Copy link

when this will get merged? It's making nuxt unusable

@pikax
Copy link
Member

pikax commented Dec 4, 2023

@TomasSestak can you confirm if the issue is still present on 3.3.10, my playground example seems to be working as expected with this fix #9309

@Alfred-Skyblue
Copy link
Member Author

I'm not sure if I need to close this PR, but in some cases, acquiring the anchor may, for some reason, lead to its removal, causing a failure in insertBefore. Perhaps we should consider directly inserting the child when there are no child elements in the parentNode. If we believe that parent-child relationship checks should not be performed here, please close this PR.

Copy link

codspeed-hq bot commented Dec 20, 2023

CodSpeed Performance Report

Merging #9134 will not alter performance

Comparing Alfred-Skyblue:fix-node-insertBefore (1644baf) with main (9fa8241)

Summary

✅ 53 untouched benchmarks

@yyx990803 yyx990803 closed this in 8655ced Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Rejected
8 participants