-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
fix(transition): fix KeepAlive with transition out-in mode behavior in production #12468
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
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/runtime-core
@vue/reactivity
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
That branch of the code has a comment on it: // #7121 ensure get the child component subtree in case
// it's been replaced during HMR It would appear that the author of that section only intended for it to be used during HMR. If it's going to be used in production then that comment needs updating. Did you investigate why that code was added just for HMR? More generally, could you also please include more details in your PR descriptions? Presumably you spent a while debugging this before deciding what the appropriate fix should be. What did you find? Why do you think this is the correct fix? If you don't provide that information in the description then anyone reviewing the change is going to need to go through that debugging and exploration process all over again. Reviewing a PR, even a small one like this, can take a reviewer several hours. We don't just need to check that the fix works, we need to assess whether it's actually the best fix for the underlying problem. Providing a brief explanation in the PR description of what you think the underlying problem is and why you think it's the right fix can save us a huge amount of time. It can also be really valuable months down the line when someone wants to understand why a particular change was made. Thanks. |
Thank you very much for your reply, honestly I'm not very good at English, most of the time I rely on translation software, how to use English accurately might be really difficult for me, but that's my problem. Return to PR, To be honest I did overlook that code comment you mentioned. But this is just my simple attempt, he may not be correct debugging the issue is really tricky, one thing to note about this issue is that when class=“test” is removed from the transition, everything goes back to normal. This is due to the indirect effect of fallthroughAttrs, which clones a new vnode when it exists, which causes vnode.component.subTree and vnode.children[0] to get completely different values. Then look at the difference between DEV and PROD, when there are fallthroughAttrs then vnode.component.subTree is the correct child vnode, so everything works fine in the DEV environment too! NOTE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.
I think this PR should be correct. It is similar to #7121 where the root node has switched. We should always apply the transition to their This needs a test case by the way. |
WalkthroughA test was added to verify that the Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant BaseTransition
participant KeepAlive
participant ComponentA
participant ComponentB
TestRunner->>BaseTransition: Mount with KeepAlive and fallthrough attrs (prod mode)
BaseTransition->>KeepAlive: Render branch A (ComponentA)
KeepAlive->>ComponentA: Render with class attribute
TestRunner->>BaseTransition: Toggle to branch B
BaseTransition->>ComponentA: Trigger leave hooks
Note right of BaseTransition: onBeforeLeave/onLeave called
BaseTransition->>KeepAlive: After leave, render branch B (ComponentB)
KeepAlive->>ComponentB: Render with class attribute
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
fix #12465
Summary by CodeRabbit