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): properly handle async component update before resolve #11619

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

Conversation

linzhe141
Copy link
Contributor

@linzhe141 linzhe141 commented Aug 15, 2024

Copy link

github-actions bot commented Aug 15, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 98.7 kB (+4 B) 37.4 kB (+1 B) 33.7 kB (-49 B)
vue.global.prod.js 156 kB (+4 B) 57.2 kB (+1 B) 50.8 kB (-85 B)

Usages

Name Size Gzip Brotli
createApp 54.2 kB (+4 B) 21 kB 19.1 kB (-41 B)
createSSRApp 58.1 kB (+4 B) 22.6 kB (+3 B) 20.6 kB (+10 B)
defineCustomElement 58.8 kB (+4 B) 22.5 kB 20.5 kB (-55 B)
overall 67.7 kB (+4 B) 26 kB (-1 B) 23.6 kB (+4 B)

@linzhe141 linzhe141 marked this pull request as draft August 15, 2024 05:19
@linzhe141 linzhe141 marked this pull request as ready for review August 15, 2024 05:22
@@ -1438,7 +1438,7 @@ function baseCreateRenderer(
nonHydratedAsyncRoot.asyncDep!.then(() => {
// the instance may be destroyed during the time period
if (!instance.isUnmounted) {
componentUpdateFn()
instance.update()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
instance.update()
update()

@edison1105
Copy link
Member

Thank you for addressing the issue.
Could you please add a test case?

@edison1105 edison1105 added need test The PR has missing test cases. 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. labels Aug 15, 2024
@@ -1438,7 +1438,7 @@ function baseCreateRenderer(
nonHydratedAsyncRoot.asyncDep!.then(() => {
// the instance may be destroyed during the time period
if (!instance.isUnmounted) {
componentUpdateFn()
update()
Copy link
Member

@edison1105 edison1105 Aug 15, 2024

Choose a reason for hiding this comment

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

There is another problem here. The update() is executed 2 times.
This behavior is not related to this PR. But it needs to be fixed.

The flow is as follows:

  1. When locateNonHydratedAsyncRoot is called, subComponent.asyncResolved is false, nonHydratedAsyncRoot has a value, and a then callback is registered.
  2. The then callback is triggered and calls update.
  3. We go back to step 1, but subComponent.asyncResolved is still false because the then at this line is registered later and executes after the previously registered then. The then callback is registered again.
  4. Same as step 2.
  5. Now subComponent.asyncResolved is true, and nonHydratedAsyncRoot is undefined.
  6. The remaining update logic runs.

In this process, the then callback gets registered twice, and update is called twice.
The following code can be used to prevent this behavior.

nonHydratedAsyncRoot.asyncDep!.then(() => {
  // the instance may be destroyed during the time period
  queuePostRenderEffect(()=>{
    if (!instance.isUnmounted) update()
  },parentSuspense)
})

@edison1105
Copy link
Member

edison1105 commented Aug 15, 2024

I'm pretty sure the previous fix is proper.
Is there any reason why you changed it?

@linzhe141
Copy link
Contributor Author

@edison1105 I tried to fix it again, maybe it should be handled like this

I'm pretty sure the previous fix is proper.我很确定之前的修复是正确的。 Is there any reason why you changed it?你有什么理由改变它吗?

You are right! I was wrong. I thought that when patching suspense, I had to confirm which component was pending, but this has nothing to do with pendingBranch.

@edison1105 edison1105 changed the title fix(runtime-core): fix nonHydratedAsyncRoot update fix(runtime-core): properly handle async component update before resolve Aug 15, 2024
@edison1105 edison1105 added ready for review This PR requires more reviews and removed need test The PR has missing test cases. labels Aug 15, 2024
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 for review This PR requires more reviews scope: suspense
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Async Component inside Suspense doesn't receive prop update
2 participants