-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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 KeepAlive with transition out-in mode behavior in production #12468
base: main
Are you sure you want to change the base?
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-sfc
@vue/compiler-dom
@vue/compiler-ssr
@vue/runtime-core
@vue/runtime-dom
@vue/reactivity
@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: |
/ecosystem-ci run |
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.
📝 Ran ecosystem CI: Open
|
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. |
fix #12465