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

Computed value not updated in SSR #5300

Closed
SquidEpps opened this issue Jan 20, 2022 · 4 comments · Fixed by #9688
Closed

Computed value not updated in SSR #5300

SquidEpps opened this issue Jan 20, 2022 · 4 comments · Fixed by #9688
Labels
❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. 🐞 bug Something isn't working

Comments

@SquidEpps
Copy link

Version

3.2.27

Reproduction link

github.com

Steps to reproduce

yarn

node index.js

What is expected?

Computed value is updated and html renders with correct message.

<div>hello world</div>

What is actually happening?

Computed value is not updated and an empty div is rendered.

<div></div>

I'm not sure if this a bug or is it me misusing computed in SSR context, but such an app architecture as in my reproduction demo seems to me completely valid and should work.

There is a computed value in a component which is based on part of app's state. No value serves as indication of the need to fetch data. Normally this should happen either during SSR or on the client if the state (or its part) was not hydrated. Async setup function allows to wait for data to be fetched which should trigger an update of state and computed value.

This acually worked in 3.2.26. New behavior was possibly introduced in commit f4f0966

@yyx990803 yyx990803 added ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. 🐞 bug Something isn't working labels Jan 21, 2022
@yellinzero
Copy link

this.effect.active = !isSSR

maybe this code make computed inactive in SSR env?

@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2023
@posva posva reopened this Nov 2, 2023
@vuejs vuejs unlocked this conversation Nov 2, 2023
@posva
Copy link
Member

posva commented Nov 2, 2023

I'm reopening this as it is still not fixed (the same reproduction still fails). It looks like it got fixed because of the line that reads msg.value after fetching:

But removing that line from the test makes it fail. I think that that line should just be removed from the test anyway as the real assertion should happen on the rendered HTML.

It would be worth adding this test case too:

test('computed reactivity during SSR with onServerPrefetch', async () => {
  const store = {
    // initial state could be hydrated
    state: reactive({ items: null as null | string[] }),

    // pretend to fetch some data from an api
    async fetchData() {
      this.state.items = ['hello', 'world']
    }
  }

  const getterSpy = vi.fn()

  const App = defineComponent(() => {
    const msg = computed(() => {
      getterSpy()
      return store.state.items?.join(' ')
    })

    // If msg value is falsy then we are either in ssr context or on the client
    // and the initial state was not modified/hydrated.
    // In both cases we need to fetch data.
    onServerPrefetch(() => store.fetchData())

    // simulate the read from a composable (e.g. filtering a list of results)
    msg.value

    return () => h('div', null, msg.value)
  })

  const app = createSSRApp(App)

  // in real world serve this html and append store state for hydration on client
  const html = await renderToString(app)

  expect(html).toMatch('hello world')

  // should only be called twice since access should be cached
  // during the render phase
  expect(getterSpy).toHaveBeenCalledTimes(2)
})

it uses onServerPrefetch() instead of an async component

@posva
Copy link
Member

posva commented Nov 2, 2023

BTW a workaround is to read again the computed once it has been fetched:

const App = defineComponent(() => {
  const msg = computed(() => store.state.items?.join(' '))

  // If msg value is falsy then we are either in ssr context or on the client
  // and the initial state was not modified/hydrated.
  // In both cases we need to fetch data.
  onServerPrefetch(async () => {
    await store.fetchData()
    msg.value
  })

  // simulate the read from a composable (e.g. filtering a list of results)
  msg.value

  return () => h('div', null, msg.value)
})

or with await

const App = defineComponent(async () => {
  const msg = computed(() => store.state.items?.join(' '))

  // simulate the read from a composable (e.g. filtering a list of results)
  msg.value

  // If msg value is falsy then we are either in ssr context or on the client
  // and the initial state was not modified/hydrated.
  // In both cases we need to fetch data.
  await store.fetchData()
  msg.value

  return () => h('div', null, msg.value)
})

@edison1105
Copy link
Member

edison1105 commented Nov 27, 2023

The root cause is that computed getters are cached before rendering:

// perf: enable caching of computed getters during render
// since there cannot be state mutations during render.
for (const e of instance.scope.effects) {
if (e.computed) e.computed._cacheable = true
}

If we had to do this, we should make the computed dirty before caching

    for (const e of instance.scope.effects) {
      if (e.computed){
        e.computed._dirty = true
        e.computed._cacheable = true
      }
    }

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. 🐞 bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants