[🐞] Computed unmount & component unmount race condition issue

[Kasheftin]

Which component is affected?

Qwik Runtime

Describe the bug

This bug is about the incorrect order of evaluation/unmounting computed variables and unmounting the component itself.

Suppose, we have a dynamic route [...path] which loads different components depending on the path variable. Then, the global store is populated with different data depending on the path.

The minimal demo app has the following store:

export enum DataType {
  empty = 'empty',
  first = 'first',
  second = 'second'
}

export type FirstData = {
  key1: { key1: string }
}

export type SecondData = {
  key2: { key2: string }
}

export type Store = {
  routepath: string
  data: FirstData | SecondData | null
  dataType: DataType
}

export const getInitialStore = (): Store => ({
  routepath: '',
  data: null,
  dataType: DataType.empty
})

export const loadStore = async (routepath: string) => {
  const store = getInitialStore()
  store.routepath = routepath
  const { data, dataType } = await loadData(store)
  Object.assign(store, { data, dataType })
  return store
}

const loadData = async (store: Store) => {
  await timeout(500)
  if (store.routepath === 'first') {
    const data: FirstData = {
      key1: {
        key1: 'first'
      }
    }
    return {
      dataType: DataType.first,
      data
    }
  } else if (store.routepath === 'second') {
    const data: SecondData = {
      key2: {
        key2: 'second'
      }
    }
    return {
      dataType: DataType.second,
      data
    }
  } else {
    return {
      dataType: DataType.empty,
      data: null
    }
  }
}

Basically, loadStore provides a connected pair of { data, dataType }: if dataType is DataType.first, then data has FirstData type, else if dataType is DataType.second, then data has SecondData type. It's far from typescript best practice, but technically it's totally fine.

[...path]/index.tsx file just loads different page component depending on the dataType:

export default component$(() => {
  const store = useContext(STORE)
  const getPage = (dataType: DataType) => {
    if (dataType === DataType.first) {
      return (
        <FirstPage />
      )
    }
    if (dataType === DataType.second) {
      return (
        <SecondPage />
      )
    }
    return (
      <DefaultPage />
    )
  }
  return (
    <div>{getPage(store.dataType)}</div>
  )
})

Finally, the page component heavily relies on the data type: FirstPage expects data to have FirstData type. Since <FirstPage> is being shown only when dataType is DataType.first, it's natural to expect that store.data has FirstData type:

export default component$(() => {
  const store = useContext(STORE)
  const data = store.data as FirstData
  const content = useComputed$(() => {
    return data.key1.key1
  })

  return (
    <div>
      First Content: {content.value}
    </div>
  )
})

It seems to be a valid code. When the route changes, { data, dataType } pair is substituted into the store in one tick. If dataType changes, it should unmount the previous page and mount the new page.

However, there's something wrong with unmounting logic. When the page changes, it tries to evaluate useComputed$ for the old page using the new data before unmounting this old page. It should somehow know that the component is going to be unmounted and skip evaluating computeds.

Reproduction

https://github.com/Kasheftin/qwik-unmount-computed-bug

Steps to reproduce

• npm run dev or npm run preview
• Click on "First Page" link
• Click on "Second Page" link
• Cannot read properties of undefined error appears

System Info

System:
    OS: Linux 6.2 Ubuntu 22.04.2 LTS 22.04.2 LTS (Jammy Jellyfish)
    CPU: (12) x64 Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz
    Memory: 20.19 GB / 31.30 GB
    Container: Yes
    Shell: 5.1.16 - /bin/bash
  Binaries:
    Node: 19.8.1 - ~/.nvm/versions/node/v19.8.1/bin/node
    npm: 9.5.1 - ~/.nvm/versions/node/v19.8.1/bin/npm
  Browsers:
    Chrome: 115.0.5790.170
  npmPackages:
    @builder.io/qwik: ^1.2.12 => 1.2.12 
    @builder.io/qwik-city: ^1.2.12 => 1.2.12 
    undici: 5.22.1 => 5.22.1 
    vite: 4.4.7 => 4.4.7

Additional Information

No response

[mhevery]

This is still a pretty complicated repro. Please create something simpler next time.... I took a look at the current repro, and it really needs to be simplified further if you think this is a real issue.

So I don't think this is an issue with Qwik, as there is no code path which would prevent the rendering of a new content before the data is available from the server. The issue is that SecondPage renders before the loader has a chance to return the new data. For Qwik to delay rendering of the second page it would have to know that the second page needs data from the server, but with your setup which copies the data from the server to a different store and than updates the UI from the store there is no easy way for Qwik to know that there should be a delay.

So to me this is working as intended. What do you think? Can we close the issue?

[Kasheftin]

Nope:)
It's not about delays.
I prepared a simpler version without the store, just a single variable + useComputed$:
https://github.com/Kasheftin/qwik-unmount-computed-bug2.

Take a look at this file, https://github.com/Kasheftin/qwik-unmount-computed-bug2/blob/master/src/routes/index.tsx
Clicking on a button just sets the variable and conditionally renders an inner component. If we convert the template into the plain javascript, it's going to be quite valid:

render(input) {
  if (input.type === 'first') { 
    .. render first and pass input.data there knowing that it has FirstData type
  } else if (input.type === 'second') {
    .. render second and pass input.data there knowing that it has SecondData type
  }
}

However, the second click on a different button throws an error. The reason is https://github.com/Kasheftin/qwik-unmount-computed-bug2/blob/master/src/components/FirstComponent.tsx has useComputed$ inside which expects props.data to have FirstData type. In reality, we can not rely on a data type inside a computed: when the data is changed, useComputed$ is recalculated first, and only then the conditional rendering unmounts the inner component.

[mhevery]

Ohh I see... That is much better repro (importance of minimal repros)

Yes, the parent component should tear down the component before the useComputed$ has a chance to run...

[DustinJSilk]

I'm also suffering from this now, where a child useComputed$ causes an error when unmounting during navigation.

It seems like its the same underlying issue as another problem i've been experiencing: #4332 and possibly even #4466
which is related to the order of events.

[wmertens]

I made the repro into a playground for easier inspection.

Indeed, the problem is always the same, signals propagate out of tree order.

I'm thinking the solution is to record the tree depth when registering listeners, and running listeners sorted by increasing depth. If we make sure to honor listener removal, that should fix the problem. There might still be issues with components that emit promises in their templates though.

Right @mhevery ?

[mhevery]

Yes, I think that would be a great fix!

[DustinJSilk]

Hey guys, has there been any movement on this? i'm experiencing another issue where a useComputed$ is running eagerly.

useComputed$ isn't useable in its current form

This should be a P3 issue

[sarat1669]

I am also facing this issue. @wmertens , is it a simple fix?

[DustinJSilk]

@sarat1669 I believe this should be fixed when Qwik V2 is released, i'm not sure how long that could be but the team are working hard on it

[gioboa]

I tried this code with v2 and it's still failing