Skip to content

fix(compile-core): track KeepAlive as block inside v-for #12915

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

edison1105
Copy link
Member

@edison1105 edison1105 commented Feb 19, 2025

close #12914

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of components wrapped in KeepAlive within v-for loops, ensuring correct tracking and lifecycle management even in stable fragments.
  • Tests

    • Added a new test to verify that KeepAlive children are properly unmounted and remounted when used inside a stable fragment and switched dynamically within a keyed list.

Copy link

github-actions bot commented Feb 19, 2025

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 101 kB 38.3 kB 34.5 kB
vue.global.prod.js 159 kB (+12 B) 58.5 kB (+9 B) 52 kB (-20 B)

Usages

Name Size Gzip Brotli
createApp (CAPI only) 46.6 kB 18.2 kB 16.7 kB
createApp 54.5 kB 21.2 kB 19.4 kB
createSSRApp 58.8 kB 23 kB 20.9 kB
defineCustomElement 59.5 kB 22.8 kB 20.8 kB
overall 68.6 kB 26.4 kB 24.1 kB

Copy link

pkg-pr-new bot commented Feb 19, 2025

Open in StackBlitz

@vue/compiler-core

npm i https://pkg.pr.new/@vue/compiler-core@12915

@vue/compiler-dom

npm i https://pkg.pr.new/@vue/compiler-dom@12915

@vue/compiler-ssr

npm i https://pkg.pr.new/@vue/compiler-ssr@12915

@vue/compiler-sfc

npm i https://pkg.pr.new/@vue/compiler-sfc@12915

@vue/runtime-core

npm i https://pkg.pr.new/@vue/runtime-core@12915

@vue/reactivity

npm i https://pkg.pr.new/@vue/reactivity@12915

@vue/runtime-dom

npm i https://pkg.pr.new/@vue/runtime-dom@12915

@vue/server-renderer

npm i https://pkg.pr.new/@vue/server-renderer@12915

@vue/shared

npm i https://pkg.pr.new/@vue/shared@12915

vue

npm i https://pkg.pr.new/vue@12915

@vue/compat

npm i https://pkg.pr.new/@vue/compat@12915

commit: 5a77b2a

@edison1105 edison1105 changed the title fix(compiler-core): always track KeepAlive children as block inside v-for fix(runtime-core): handle KeepAlive children unmount when wrapped in stable v-for Feb 20, 2025
@vuejs vuejs deleted a comment from vue-bot Feb 20, 2025
@edison1105 edison1105 added ready to merge The PR is ready to be merged. 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. scope: keep-alive labels Feb 20, 2025
@skirtles-code
Copy link
Contributor

I'm struggling to understand the proposed change. Here's what I've got so far...

When const maxLength = ref(2) is used in the original reproduction:

  • The fragment is marked as UNKEYED_FRAGMENT.
  • When unmount is called, the value of parentComponent is the KeepAlive.
  • parentComponent.ctx.deactivate(vnode) works fine.

But when const maxLength = 2 is used instead:

  • The fragment is marked as STABLE_FRAGMENT.
  • When unmount is called, the value of parentComponent is the App.
  • parentComponent.ctx.deactivate(vnode) explodes.

So the proposed fix is to use vnode.component.parent instead of parentComponent, as that will be the KeepAlive.

But that raises several questions:

  1. Why is parentComponent different in the first place?
  2. Are other things inside unmount impacted by being passed the 'wrong' parentComponent?
  3. Is the 'wrong' parentComponent being used elsewhere?
  4. If we can use vnode.component.parent instead of parentComponent, do we even need to pass parentComponent at all? Are there other cases where they differ?

I wanted to start by investigating point 2 in particular, so I tried unmounting the components inside the KeepAlive (rather than deactivating them) by using include to disable to KeepAlive. That yielded another error:

Like the original reproduction, it works fine with const maxLength = ref(1) but not const maxLength = 1, so I imagine the underlying reasons are essentially the same.

Copy link

coderabbitai bot commented May 22, 2025

Walkthrough

The changes update the vFor transform logic to always mark KEEP_ALIVE child blocks as blocks, regardless of fragment stability. Additionally, a new test is added to verify the correct unmounting and remounting behavior of KeepAlive children within a stable v-for fragment, addressing a specific reported issue.

Changes

File(s) Change Summary
packages/compiler-core/src/transforms/vFor.ts Updated logic to always mark child blocks with the KEEP_ALIVE tag as blocks in the v-for transform, ensuring correct handling during code generation. Added import for the KEEP_ALIVE runtime helper.
packages/runtime-core/__tests__/rendererOptimizedMode.spec.ts Added a new test case to verify KeepAlive child unmounting/remounting behavior when used inside a stable fragment with v-for. Defined two simple components and toggled between them inside a KeepAlive wrapper within a single-item renderList. No changes to existing tests or exports.

Sequence Diagram(s)

sequenceDiagram
    participant App
    participant KeepAlive
    participant CompA
    participant CompB

    App->>KeepAlive: Render with CompA (via v-for, stable fragment)
    KeepAlive->>CompA: Mount CompA
    App->>KeepAlive: Toggle to CompB
    KeepAlive-->>CompA: Unmount CompA
    KeepAlive->>CompB: Mount CompB
Loading

Assessment against linked issues

Objective Addressed Explanation
Fix error when using stable v-for with <keep-alive> and ensure correct unmount/remount logic (#12914)

Suggested labels

scope: teleport

Poem

A hop and a skip in the code we explore,
KeepAlive now handled, with bugs no more!
Through loops and fragments, components delight,
Mounting and unmounting, all works just right.
With whiskers and wits, this patch is complete—
Bugs take a nap, as features repeat! 🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 93949e6 and 5a77b2a.

📒 Files selected for processing (2)
  • packages/compiler-core/src/transforms/vFor.ts (2 hunks)
  • packages/runtime-core/__tests__/rendererOptimizedMode.spec.ts (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/compiler-core/src/transforms/vFor.ts (1)
packages/compiler-core/src/runtimeHelpers.ts (1)
  • KEEP_ALIVE (4-4)
🔇 Additional comments (4)
packages/compiler-core/src/transforms/vFor.ts (2)

45-45: LGTM: Added import for KEEP_ALIVE

Adding the KEEP_ALIVE import from runtimeHelpers to support the fix.


212-214: Fix for KeepAlive components in stable fragments

This change ensures that any child block with the KEEP_ALIVE tag is always marked as a block, regardless of fragment stability. This fixes the unmounting behavior of KeepAlive children when wrapped in a stable v-for fragment (issue #12914).

The change is minimal and focused on the specific issue. By marking KeepAlive components as blocks, they'll be properly tracked during code generation, which prevents the unmounting issues described in the PR objectives where parentComponent.ctx.deactivate was being called on the root App instead of the KeepAlive component.

packages/runtime-core/__tests__/rendererOptimizedMode.spec.ts (2)

4-4: LGTM: Added imports for test

Adding imports for KeepAlive and computed to support the new test case.

Also applies to: 9-9


1408-1456: Test case validates the fix for issue #12914

This test correctly verifies that KeepAlive components unmount and remount properly when wrapped inside a stable v-for fragment. It confirms the behavior described in the PR objectives by:

  1. Creating two simple components (CompA, CompB)
  2. Using a reactive toggle to switch between them
  3. Wrapping the components in KeepAlive inside a stable v-for fragment
  4. Verifying the correct component is rendered after toggling

The test effectively demonstrates that the fix in the compiler works as expected.

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@edison1105
Copy link
Member Author

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented May 22, 2025

📝 Ran ecosystem CI: Open

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

@vuejs vuejs deleted a comment from edison1105 May 22, 2025
@edison1105
Copy link
Member Author

edison1105 commented May 22, 2025

@skirtles-code
Thank you for your review. I have reverted the PR implementation back to the original solution:
ad243e7

Root Cause of the Bug:

The dynamicChildren of a v-for Fragment contains both the KeepAlive children and the KeepAlive component itself.

// CompA is KeepAlive child
n1.dynamicChildren = [CompA, KeepAlive]

Issue Analysis:

  • Playground 1 - 12914

    • When clicking the toggle button, patchBlockChildren is executed (patching dynamicChildren)
    • Since the first node in dynamicChildren is CompA, unmounting triggers parentComponent.ctx.deactivate(vnode). However, the parentComponent here is App.vue, causing an error: parentComponent.ctx.deactivate is not a function
  • Playground 2

    • When clicking the toggle button, patchBlockChildren is executed (patching dynamicChildren)
    • CompA is replaced with CompB.
    • Then patching KeepAlive, since CompA has already been unmounted, patchBlockChildren internally receives a null container (because CompA.el has been removed), resulting in an error: TypeError: Cannot read properties of null (reading 'insertBefore')

Solution:

When KeepAlive is used together with v-for, create a Block for it to prevent dynamicChildren from containing both KeepAlive children and the KeepAlive component itself. This avoids the patching issues described above.

function render(_ctx, _cache, $props, $setup, $data, $options) {
  return (_openBlock(), _createElementBlock(_Fragment, null, _renderList($setup.maxLength, (idx) => {
-    return _createVNode(_KeepAlive, { include: ['foo'] }, [
+    return (_openBlock(), _createBlock(_KeepAlive, { include: ['foo'] }, [
      (_openBlock(), _createBlock(_resolveDynamicComponent($setup.val ? $setup.CompB : $setup.CompA)))
    ], 1024 /* DYNAMIC_SLOTS */)
  }), 64 /* STABLE_FRAGMENT */))
}

The scenario where KeepAlive is used together with v-for is relatively uncommon. The changes in this PR currently don't impact runtime performance.

Another potential solution would be:
During dynamicChildren collection, if a vnode is KeepAlive and its children already exist in the currentBlock, replace those nodes in currentBlock with the KeepAlive component itself. However, since createBaseVNode belongs to hot paths, this approach might not be worthwhile.

So far, no similar issues have been observed with other built-in components when used alongside v-for.

@edison1105 edison1105 marked this pull request as ready for review May 22, 2025 07:46
@edison1105 edison1105 changed the title fix(runtime-core): handle KeepAlive children unmount when wrapped in stable v-for fix(runtime-core): track KeepAlive children as block inside v-for May 22, 2025
@edison1105 edison1105 changed the title fix(runtime-core): track KeepAlive children as block inside v-for fix(runtime-core): track KeepAlive as block inside v-for May 22, 2025
@edison1105 edison1105 changed the title fix(runtime-core): track KeepAlive as block inside v-for fix(compile-core): track KeepAlive as block inside v-for May 22, 2025
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: keep-alive
Projects
None yet
Development

Successfully merging this pull request may close these issues.

parentComponent.ctx.deactivate is not a function while using stable v-for and <keep-alive>
3 participants