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(runtime-core): modify the patchFlag of the label containing the ref attribute #9240

Closed
wants to merge 2 commits into from

Conversation

Alfred-Skyblue
Copy link
Member

fixed #9239

@github-actions
Copy link

github-actions bot commented Sep 18, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 85.9 kB 32.6 kB 29.5 kB
vue.global.prod.js 132 kB (+11 B) 49.3 kB (+5 B) 44.4 kB (+12 B)

Usages

Name Size Gzip Brotli
createApp 47.9 kB 18.8 kB 17.2 kB
createSSRApp 50.6 kB 19.9 kB 18.2 kB
defineCustomElement 50.3 kB 19.6 kB 17.9 kB
overall 61.2 kB 23.7 kB 21.6 kB

@Alfred-Skyblue Alfred-Skyblue marked this pull request as ready for review September 18, 2023 07:02
packages/runtime-core/src/renderer.ts Outdated Show resolved Hide resolved
@Alfred-Skyblue Alfred-Skyblue changed the title fix(runtime-core): fix child node unloading in v-for fix(runtime-core): modify the patchFlag of the label containing the ref attribute Sep 19, 2023
@Alfred-Skyblue Alfred-Skyblue marked this pull request as draft September 19, 2023 06:11
@Alfred-Skyblue Alfred-Skyblue marked this pull request as ready for review September 19, 2023 06:38
@edison1105 edison1105 added the ready to merge The PR is ready to be merged. label Sep 19, 2023
@@ -597,6 +597,7 @@ export function buildProps(
}

if (isVBind && isStaticArgOf(arg, 'ref') && context.scopes.vFor > 0) {
hasRef = true
Copy link
Contributor

Choose a reason for hiding this comment

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

The tests all seem to pass even if this line is omitted. Perhaps there should be another test case added to cover this scenario too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is to address the issue at #9239 (comment), the function cannot be resolved in compile so I can't add a unit test for it.

@@ -747,7 +748,7 @@ export function buildProps(
}
}
if (
!shouldUseBlock &&
!(shouldUseBlock && !hasRef) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unclear why hasRef has been singled out for special treatment. Two lines down from here, we check for hasRef || hasVnodeHook || runtimeDirectives.length > 0. Don't we have the same problem for hasVnodeHook and runtimeDirectives.length > 0?

In both cases, the key seems to prevent unmounted being triggered.

@pikax pikax added need test The PR has missing test cases. need discussion and removed ready to merge The PR is ready to be merged. labels Oct 19, 2023
@Alfred-Skyblue
Copy link
Member Author

Resolved in #11682

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need discussion need test The PR has missing test cases.
Projects
Status: Rejected
4 participants