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(teleport): ensure descendent component would be unmounted correctly #6529

Merged
merged 3 commits into from
Oct 20, 2023

Conversation

nhpupu
Copy link
Contributor

@nhpupu nhpupu commented Aug 22, 2022

fix #6347

This pr makes 2 change.

  1. When doRemove || !isTeleportDisabled(props) is false, would unmount children with doRemove arg being false, instead of doing nothing.
  2. When doRemove is false but !isTeleportDisabled(props) is true, would not execute hostRemove(anchor!) any more.

!!child.dynamicChildren
)
}
// an unmounted teleport should always unmount its children whether it's disabled or not
Copy link
Member

@edison1105 edison1105 Aug 23, 2022

Choose a reason for hiding this comment

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

this will be a breaking change.
misunderstood.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original comment was added in #2870. And it refered to remove but not unmount.
When it comes to remove, there is indeed no need to remove its children when it's disabled and doRmove is false.
But unmount is not equal to remove. We can unmount a child without remove it, by making doRemove false.

While unmounting a component, we should also unmount its children. If not, its children would never be unmounted, and will cause the bug this pr fixes.

@mateo-m
Copy link

mateo-m commented Mar 30, 2023

@edison1105 do you know if this PR could be merged sometime soon?

@edison1105
Copy link
Member

@edison1105 do you know if this PR could be merged sometime soon ?

Sry Im not sure.

@haoqunjiang haoqunjiang added scope: teleport ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. labels Sep 19, 2023
@github-actions
Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 85.9 kB (+7 B) 32.6 kB (-1 B) 29.5 kB (-11 B)
vue.global.prod.js 132 kB (+7 B) 49.3 kB (-3 B) 44.4 kB (-1 B)

Usages

Name Size Gzip Brotli
createApp 47.9 kB 18.8 kB 17.2 kB
createSSRApp 50.6 kB 19.9 kB 18.2 kB
defineCustomElement 50.3 kB 19.6 kB 17.9 kB
overall 61.2 kB 23.7 kB 21.6 kB

@haoqunjiang
Copy link
Member

/ecosystem-ci run

@yyx990803 yyx990803 added the ready to merge The PR is ready to be merged. label Sep 19, 2023
@vue-bot
Copy link
Contributor

vue-bot commented Sep 19, 2023

📝 Ran ecosystem CI: Open

suite result latest scheduled
nuxt success failure
pinia success success
quasar failure failure
router success success
test-utils success success
vant success success
vite-plugin-vue success success
vitepress success success
vue-i18n success success
vue-macros failure failure
vuetify success success
vueuse success success
vue-simple-compiler success success

@yyx990803 yyx990803 merged commit 4162311 into vuejs:main Oct 20, 2023
10 checks passed
lumozx pushed a commit to lumozx/core that referenced this pull request Oct 21, 2023
@Joshua-Leee
Copy link

Hello, has this submission been published yet? If not, when is it planned to be released?

@edison1105
Copy link
Member

@Joshua-Leee It's released in v3.3.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. ready to merge The PR is ready to be merged. scope: teleport
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

The onUnmounted callback is not triggered when using Teleport
7 participants