fix(server-renderer): Fix call to serverPrefetch in server renderer with an async setup #10893
Conversation
… option w/async setup This fixes a bug where the serverPrefetch option would not be called with an async setup method. A `prefetches` reference was created while `instance.sp` was `null`. However, with an async setup, `instance.sp` has the serverPrefetch methods only after setup resolves. The fix is achieved by waiting to for setup to resolve before trying to access the the serverPrefetch options from `instance.sp`.
Size ReportBundles
Usages
|
|
Hi @sodatea, is there anything else you need from me for this PR to merge? |
| const setupAndPrefetched = setupResolved | ||
| .then(() => { | ||
| // instance.sp may be null until an async setup resolves, so evaluate it here | ||
| const prefetches = getPrefetches() |
There was a problem hiding this comment.
It seems getPrefetches will be called twice, if hasAsyncSetup equals false. I think this should be avoided.
There was a problem hiding this comment.
Reading instance.sp only once and caching that value to be used after set up resolves is what caused the bug.
What is the concern about accessing instance.sp twice?
There was a problem hiding this comment.
@edison1105 I pushed a commit which:
- Makes
renderComponentVNodeasync. It now always returns a promise. - Uses
awaitdirectly on thesetupComponentcall. No need to inspect it to see if it is a promise. - Simplifies the implementation to remove branching logic.
- Reads
instance.sponce, after setup.
Please let me know if this is what you had in mind. This extra commit is more involved.
There was a problem hiding this comment.
I'm not sure if it's reasonable to change renderComponentVNode to async. see #11340 (comment)
Let's wait for reviews from other team members
There was a problem hiding this comment.
@edison1105 Ok, I see that there is deliberate synchronous fast paths in place. In that case, I'm happy to revert the last commit or do whatever is needed here.
There was a problem hiding this comment.
Maybe we could change it like below, what do you think?
let prefetches = instance.sp /* LifecycleHooks.SERVER_PREFETCH */
if (hasAsyncSetup || prefetches) {
let p: Promise<unknown> = (
hasAsyncSetup
// instance.sp may be null until an async setup resolves, so evaluate it here
? (res as Promise<void>).then(() => (prefetches = instance.sp))
: Promise.resolve()
)
.then(() => {
if (prefetches) {
return Promise.all(
prefetches.map(prefetch => prefetch.call(instance.proxy)),
)
}
})
// Note: error display is already done by the wrapped lifecycle hook function.
.catch(NOOP)
return p.then(() => renderComponentSubTree(instance, slotScopeId))
} else {
return renderComponentSubTree(instance, slotScopeId)
}There was a problem hiding this comment.
@edison1105 I pushed up that change in 7af4f77, and then I went a little further and optimized away two promises one promise in e159b16.
-
Removes the unnecessary
hasAsyncSetupternary conditional by usingPromise.resolvedirectly around theresobject because it returns the exact same promise if it is given one. -
Removes a new promise by removing a
.thencallback -
Removes another new promise by removing aEdit: I was mistaken about this change; It isn't pushed, nor needed here..catchcallback in favor of assigning the noop as the second argument
So maybe this will be a little faster or at least produce a little less garbage.
…implify internal logic
❌ Deploy Preview for vue-next-template-explorer failed.
|
❌ Deploy Preview for vue-sfc-playground failed.
|
@vue/compiler-core
@vue/compiler-sfc
@vue/compiler-dom
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-dom
@vue/runtime-core
@vue/server-renderer
@vue/shared
@vue/compat
vue
commit: |
| const p: Promise<unknown> = Promise.resolve(res as Promise<void>) | ||
| .then(() => { | ||
| // instance.sp may be null until an async setup resolves, so evaluate it here | ||
| prefetches = instance.sp |
There was a problem hiding this comment.
| prefetches = instance.sp | |
| if(hasAsyncSetup) prefetches = instance.sp |
There was a problem hiding this comment.
I still believe here should check hasAsyncSetup
There was a problem hiding this comment.
Please see commit 7bf6732
| const p: Promise<unknown> = Promise.resolve(res as Promise<void>) | ||
| .then(() => { | ||
| // instance.sp may be null until an async setup resolves, so evaluate it here | ||
| prefetches = instance.sp |
There was a problem hiding this comment.
I still believe here should check hasAsyncSetup
…cSetup conditional
This fixes a bug where the serverPrefetch option would not be called with an async setup method.
About the fix
A
prefetchesreference was created whileinstance.spwasnull. However, with an async setup,instance.sphas the serverPrefetch methods only after setup resolves.The fix is achieved by waiting to for setup to resolve before trying to access the the serverPrefetch options from
instance.sp.I added a unit test that reproduces the problem, by adding an async setup to the component definition. FWIW, I considered and experimented with adding more async setup variations of the existing tests, but backed out after they did not reproduce the problem and decided they weren't critical.
Impact of the bug
The bug can break SSR because the server renderer will not wait long enough to produce serializable state.
Specifically, this bug exists when using:
defineNuxtComponentfrom Nuxt 3 (because it creates an async setup method)serverPrefetchmethod that resolves when all apollo promises resolve)The server will make a request, but the renderer doesn't wait for it to finish. Nuxt dispatches
app:rendered, serializes whatever state it has (which excludes the in-flight apollo graphql requests). Then the page loads in a blank state, then the client in the browser will issue the now redundant graphql requests, and finally render the complete page.This problem affects Nuxt 2 apollo apps that are migrating to Nuxt 3 and is a regression in the ecosystem.