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(Transition): fix transition memory leak edge case #12182

Merged
merged 5 commits into from
Nov 15, 2024
Merged

Conversation

edison1105
Copy link
Member

close #12181

Copy link

github-actions bot commented Oct 15, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 99.7 kB (+86 B) 37.9 kB (+12 B) 34.1 kB (-29 B)
vue.global.prod.js 158 kB (+86 B) 57.7 kB (+13 B) 51.4 kB (+32 B)

Usages

Name Size Gzip Brotli
createApp (CAPI only) 47 kB 18.3 kB 16.7 kB
createApp 55.1 kB 21.3 kB 19.5 kB
createSSRApp 59.1 kB 23 kB 20.9 kB
defineCustomElement 59.9 kB 22.9 kB 20.8 kB
overall 68.9 kB (+86 B) 26.4 kB (+16 B) 24 kB (+86 B)

@edison1105
Copy link
Member Author

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Oct 15, 2024

📝 Ran ecosystem CI: Open

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

Copy link

pkg-pr-new bot commented Oct 16, 2024

Open in Stackblitz

@vue/compiler-core

pnpm add https://pkg.pr.new/@vue/compiler-core@12182

@vue/compiler-dom

pnpm add https://pkg.pr.new/@vue/compiler-dom@12182

@vue/compiler-ssr

pnpm add https://pkg.pr.new/@vue/compiler-ssr@12182

@vue/compiler-sfc

pnpm add https://pkg.pr.new/@vue/compiler-sfc@12182

@vue/reactivity

pnpm add https://pkg.pr.new/@vue/reactivity@12182

@vue/runtime-core

pnpm add https://pkg.pr.new/@vue/runtime-core@12182

@vue/runtime-dom

pnpm add https://pkg.pr.new/@vue/runtime-dom@12182

@vue/server-renderer

pnpm add https://pkg.pr.new/@vue/server-renderer@12182

@vue/shared

pnpm add https://pkg.pr.new/@vue/shared@12182

vue

pnpm add https://pkg.pr.new/vue@12182

@vue/compat

pnpm add https://pkg.pr.new/@vue/compat@12182

commit: 930a9fb

Copy link

pkg-pr-new bot commented Oct 16, 2024

Open in Stackblitz

@vue/compiler-dom

pnpm add https://pkg.pr.new/@vue/compiler-dom@12182

@vue/compiler-core

pnpm add https://pkg.pr.new/@vue/compiler-core@12182

@vue/compiler-sfc

pnpm add https://pkg.pr.new/@vue/compiler-sfc@12182

@vue/compiler-ssr

pnpm add https://pkg.pr.new/@vue/compiler-ssr@12182

@vue/reactivity

pnpm add https://pkg.pr.new/@vue/reactivity@12182

@vue/runtime-core

pnpm add https://pkg.pr.new/@vue/runtime-core@12182

@vue/runtime-dom

pnpm add https://pkg.pr.new/@vue/runtime-dom@12182

@vue/shared

pnpm add https://pkg.pr.new/@vue/shared@12182

@vue/server-renderer

pnpm add https://pkg.pr.new/@vue/server-renderer@12182

vue

pnpm add https://pkg.pr.new/vue@12182

@vue/compat

pnpm add https://pkg.pr.new/@vue/compat@12182

commit: ec2fcec

@edison1105 edison1105 added ready to merge The PR is ready to be merged. scope: transition 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. labels Oct 16, 2024
Copy link
Contributor

@ferferga ferferga left a comment

Choose a reason for hiding this comment

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

Why can't we remove the last check to make this less redundant (we already have an oldInnerChild at the top level)?

@edison1105
Copy link
Member Author

Why can't we remove the last check to make this less redundant (we already have an oldInnerChild at the top level)?

const leavingVNodesCache = getLeavingNodesForType(
state,
oldInnerChild!,
)
leavingVNodesCache[String(oldInnerChild!.key)] = oldInnerChild!

oldInnerChild is used in delayLeave

Copy link
Contributor

@ferferga ferferga left a comment

Choose a reason for hiding this comment

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

Good catch

  • Regarding Possible memory leak with Transition #12181 (comment), this fixes the problem and tests pass (at least locally).
  • Given this is a really tiny change that could pass unnoticed in any refactor, I'd add a reference to the issue as a comment and even a test, I think it could be done with FinalizationRegistry. I could try later if you don't want to do it.

Sorry if my comments sometimes sound dumb or silly, I'm still learning about Vue's codebase.

packages/runtime-core/src/components/BaseTransition.ts Outdated Show resolved Hide resolved
@edison1105
Copy link
Member Author

@ferferga
No worries, reviews of the PR are very welcome.

Given this is a really tiny change that could pass unnoticed in any refactor, I'd add a reference to the issue as a comment and even a test, I think it could be done with FinalizationRegistry. I could try later if you don't want to do it.

I'm not sure how to do it, maybe you could try it later.

@ferferga
Copy link
Contributor

Tests added at #12190

I'm targeting this branch as well, please let me know if I should target main instead.

@edison1105 edison1105 closed this Oct 21, 2024
@edison1105 edison1105 deleted the edison/fix/12181 branch October 21, 2024 07:36
@edison1105 edison1105 restored the edison/fix/12181 branch October 21, 2024 08:13
@edison1105 edison1105 reopened this Oct 21, 2024
Copy link

netlify bot commented Nov 15, 2024

Deploy Preview for vue-sfc-playground failed. Why did it fail? →

Name Link
🔨 Latest commit 4f71f8a
🔍 Latest deploy log https://app.netlify.com/sites/vue-sfc-playground/deploys/6736b1fd12dfc80008e1c8b9

Copy link

netlify bot commented Nov 15, 2024

Deploy Preview for vue-next-template-explorer failed. Why did it fail? →

Name Link
🔨 Latest commit 4f71f8a
🔍 Latest deploy log https://app.netlify.com/sites/vue-next-template-explorer/deploys/6736b1fd3e98c500085a582f

@yyx990803 yyx990803 merged commit 660132d into main Nov 15, 2024
14 of 15 checks passed
@yyx990803 yyx990803 deleted the edison/fix/12181 branch November 15, 2024 02:40
noootwo pushed a commit to noootwo/core that referenced this pull request Nov 15, 2024
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. ready to merge The PR is ready to be merged. scope: transition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible memory leak with Transition
4 participants