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(hmr): fix render error in change static nodes (#6978) #7114

Closed
wants to merge 3 commits into from

Conversation

zhangzhonghe
Copy link
Member

@zhangzhonghe zhangzhonghe commented Nov 12, 2022

fix #6978
fix #7138

@netlify
Copy link

netlify bot commented Nov 12, 2022

Deploy Preview for vuejs-coverage failed.

Name Link
🔨 Latest commit 73dd0d1
🔍 Latest deploy log https://app.netlify.com/sites/vuejs-coverage/deploys/636f2f1956f1a6000856cbbe

@lloydtao
Copy link

lloydtao commented Oct 6, 2023

hey! any update on getting this change rolled out? 😅

it's still a pain to deal with this bug, and the fix looks good to go 😸

@oceangravity
Copy link

hey! any update on getting this change rolled out? 😅

it's still a pain to deal with this bug, and the fix looks good to go 😸

It seems like the core team is waiting for the release of Vapor mode.

🙄

Comment on lines +428 to +430
if (__DEV__ && isArray(children)) {
children = [...children]
}
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we should do always this, I wonder if we should change children to be cloned: children = (children as VNode[]).map(deepCloneVNode)

I also wonder if the condition is correct, that way we are unnecessarily adding a new array every time, from this example alone we could do something like:

if (
    __DEV__ &&
    (patchFlag === PatchFlags.HOISTED ||
      patchFlag === PatchFlags.UNKEYED_FRAGMENT) &&
    isArray(children)
  )

But not certain if that will fix every case 🤔

@yyx990803 yyx990803 closed this in 7334376 Oct 21, 2023
@yyx990803
Copy link
Member

Thanks a lot for the PR, it correctly identifies the cause of the problem, but I think a more ideal fix would be to only clone the children array when we know for sure it's hoisted. Unfortunately there isn't a really easy check to do this from runtime, so I implemented a compile-time fix where the hoistStatic transform directly generates code that clones the array if we know we are compiling for HMR. See 7334376

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Rejected
5 participants