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.

Sign up for GitHub

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

Jump to bottom

fix(server-renderer): Fix call to serverPrefetch in server renderer with an async setup #10893

Merged
merged 6 commits into from
Sep 3, 2024
Merged

fix(server-renderer): Fix call to serverPrefetch in server renderer with an async setup #10893

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
abc54c6
test(server-renderer): create serverPrefetch w/async setup test case
deleteme May 8, 2024
a6f421f
fix(server-renderer): Fix renderComponentVNode to call serverPrefetch…
deleteme May 8, 2024
4ec094b
fix(server-renderer): refactor renderComponentVNode to be async and s…
deleteme Aug 16, 2024
7af4f77
fix(server-renderer): undo async renderComponentVNode, preserve fast …
deleteme Aug 20, 2024
e159b16
fix(server-renderer): simplify logic, remove two promises
deleteme Aug 20, 2024
7bf6732
fix(server-renderer): guard reassignment of prefetches behind hasAsyn…
deleteme Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions packages/server-renderer/__tests__/render.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -873,6 +873,26 @@ function testRender(type: string, render: typeof renderToString) {
expect(html).toBe(`<div>hello</div>`)
})

test('serverPrefetch w/ async setup', async () => {
const msg = Promise.resolve('hello')
const app = createApp({
data() {
return {
msg: '',
}
},
async serverPrefetch() {
this.msg = await msg
},
render() {
return h('div', this.msg)
},
async setup() {},
})
const html = await render(app)
expect(html).toBe(`<div>hello</div>`)
})

// #2763
test('error handling w/ async setup', async () => {
const fn = vi.fn()
Expand Down
30 changes: 17 additions & 13 deletions packages/server-renderer/src/render.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,22 +94,26 @@ export function renderComponentVNode(
const instance = createComponentInstance(vnode, parentComponent, null)
const res = setupComponent(instance, true /* isSSR */)
const hasAsyncSetup = isPromise(res)
const prefetches = instance.sp /* LifecycleHooks.SERVER_PREFETCH */
if (hasAsyncSetup || prefetches) {
let p: Promise<unknown> = hasAsyncSetup
const getPrefetches = () => instance.sp /* LifecycleHooks.SERVER_PREFETCH */
if (hasAsyncSetup || getPrefetches()) {
const setupResolved: Promise<unknown> = hasAsyncSetup
? (res as Promise<void>)
: Promise.resolve()
if (prefetches) {
p = p
.then(() =>
Promise.all(
const setupAndPrefetched = setupResolved
.then(() => {
// instance.sp may be null until an async setup resolves, so evaluate it here
const prefetches = getPrefetches()
Copy link
Member

@edison1105 edison1105 Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It seems getPrefetches will be called twice, if hasAsyncSetup equals false. I think this should be avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@edison1105 I pushed a commit which:

  • Makes renderComponentVNode async. It now always returns a promise.
  • Uses await directly on the setupComponent call. No need to inspect it to see if it is a promise.
  • Simplifies the implementation to remove branching logic.
  • Reads instance.sp once, after setup.

Please let me know if this is what you had in mind. This extra commit is more involved.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

@edison1105 edison1105 Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)
  }

Copy link
Contributor Author

@deleteme deleteme Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@edison1105 I pushed up that change in 7af4f77, and then I went a little further and optimized away two promises one promise in e159b16.

  1. Removes the unnecessary hasAsyncSetup ternary conditional by using Promise.resolve directly around the res object because it returns the exact same promise if it is given one.

  2. Removes a new promise by removing a .then callback

  3. Removes another new promise by removing a .catch callback in favor of assigning the noop as the second argument Edit: I was mistaken about this change; It isn't pushed, nor needed here.

So maybe this will be a little faster or at least produce a little less garbage.

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))
)
}
})
// Note: error display is already done by the wrapped lifecycle hook function.
.catch(NOOP)
return setupAndPrefetched.then(() =>
renderComponentSubTree(instance, slotScopeId),
)
} else {
return renderComponentSubTree(instance, slotScopeId)
}
Expand Down