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-core): do not fire lifecycle hooks during unmounting #9370

Merged
merged 11 commits into from
Jun 7, 2024

Conversation

edison1105
Copy link
Member

@edison1105 edison1105 commented Oct 10, 2023

close #9264 #8898 #9617
the root cause is mounted hook is already queued in pendingPostFlushCbs and isUnmounted is false.
we can't change instance.isUnmounted = true to synchronous perform because it could be a breaking change

@github-actions
Copy link

github-actions bot commented Oct 10, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 89.6 kB (+84 B) 34.1 kB (+56 B) 30.7 kB (+28 B)
vue.global.prod.js 147 kB (+84 B) 53.3 kB (+23 B) 47.7 kB (+63 B)

Usages

Name Size Gzip Brotli
createApp 49.9 kB (+84 B) 19.5 kB (+36 B) 17.9 kB (+65 B)
createSSRApp 53.2 kB (+84 B) 20.8 kB (+32 B) 19 kB (+23 B)
defineCustomElement 52.2 kB (+84 B) 20.3 kB (+41 B) 18.5 kB (+44 B)
overall 63.4 kB (+84 B) 24.5 kB (+40 B) 22.2 kB (+16 B)

@Disservin
Copy link
Contributor

Thanks for the pr, I cant test it right now but I assume that also fixes the issue of my reproduction right in #9264 and not only posva's ?

@edison1105
Copy link
Member Author

@Disservin Yes, The root cause is the same.

@Disservin
Copy link
Contributor

Nice, thanks!

@Disservin
Copy link
Contributor

#9617 is probably also related

@Disservin
Copy link
Contributor

Any update on this? ^^ There have been 3 issues related to this #9264, #8898 and #9617 and another issue which was opened in pinia.

Copy link

netlify bot commented Jan 9, 2024

Deploy Preview for vue-sfc-playground failed.

Name Link
🔨 Latest commit c011f34
🔍 Latest deploy log https://app.netlify.com/sites/vue-sfc-playground/deploys/659d18f65c263b0008f90231

Copy link

netlify bot commented Jan 9, 2024

Deploy Preview for vue-next-template-explorer failed.

Name Link
🔨 Latest commit c011f34
🔍 Latest deploy log https://app.netlify.com/sites/vue-next-template-explorer/deploys/659d18f6deae4f0008a8d445

@haoqunjiang haoqunjiang added the 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. label Mar 29, 2024
@yyx990803
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented May 31, 2024

📝 Ran ecosystem CI: Open

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

@edison1105
Copy link
Member Author

vant's test case failed, it seems related to this commit and they should update those snapshots.

@skirtles-code
Copy link
Contributor

skirtles-code commented Jun 5, 2024

Does this also need to account for KeepAlive?

I've put together a couple of Playground examples that might be relevant here:

For the first example, the key thing to notice is what happens if you click the button multiple times. Contrast that with what happens if you comment out the show.value = false line.

For the second example, the mounted hook is being cancelled but not the activated hook.

I don't know whether these would be considered out of scope for this change, but I thought they were at least worth considering.

@yyx990803
Copy link
Member

The fix is correct, but I refactored the implementation to make it more efficient in 27d9948

  • Before: iterating hooks, then for each hook there is searching + splicing
  • After: only iterating hooks once

@yyx990803 yyx990803 merged commit aa156ed into vuejs:main Jun 7, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage.
Projects
Development

Successfully merging this pull request may close these issues.

TypeError: dynamic css on dynamically imported component
7 participants