wip(suspense): discard side effects when content is unmounted before resolve

packages/runtime-core/__tests__/rendererSuspense.spec.ts

@@ -164,13 +164,7 @@ describe('renderer: suspense', () => {
         })
 
         await p
-        // test resume for returning bindings
-        return {
-          msg: 'async'
-        }
-      },
-      render(this: any) {
-        return h('div', this.msg)
+        return () => h('div', 'async')
       }
     }
 
@@ -201,11 +195,100 @@ describe('renderer: suspense', () => {
     expect(calls).toEqual([`watch callback`, `mounted`, 'unmounted'])
   })
 
-  // should receive updated props/slots when resolved
-  test.todo('content update before suspense resolve')
+  test('content update before suspense resolve', async () => {
+    const Async = createAsyncComponent({
+      setup(props: { msg: string }) {
+        return () => h('div', props.msg)
+      }
+    })
+
+    const msg = ref('foo')
+
+    const Comp = {
+      setup() {
+        return () =>
+          h(Suspense, null, {
+            default: h(Async, { msg: msg.value }),
+            fallback: h('div', `fallback ${msg.value}`)
+          })
+      }
+    }
+
+    const root = nodeOps.createElement('div')
+    render(h(Comp), root)
+    expect(serializeInner(root)).toBe(`<div>fallback foo</div>`)
+
+    // value changed before resolve
+    msg.value = 'bar'
+    await nextTick()
+    // fallback content should be updated
+    expect(serializeInner(root)).toBe(`<div>fallback bar</div>`)
+
+    await Promise.all(deps)
+    await nextTick()
+    // async component should receive updated props/slots when resolved
+    expect(serializeInner(root)).toBe(`<div>bar</div>`)
+  })
 
   // mount/unmount hooks should not even fire
-  test.todo('unmount before suspense resolve')
+  test('unmount before suspense resolve', async () => {
+    const deps: Promise<any>[] = []
+    const calls: string[] = []
+    const toggle = ref(true)
+
+    const Async = {
+      async setup() {
+        const p = new Promise(r => setTimeout(r, 1))
+        deps.push(p)
+
+        watch(() => {
+          calls.push('watch callback')
+        })
+
+        onMounted(() => {
+          calls.push('mounted')
+        })
+
+        onUnmounted(() => {
+          calls.push('unmounted')
+        })
+
+        await p
+        return () => h('div', 'async')
+      }
+    }
+
+    const Comp = {
+      setup() {
+        return () =>
+          h(Suspense, null, {
+            default: toggle.value ? h(Async) : null,
+            fallback: h('div', 'fallback')
+          })
+      }
+    }
+
+    const root = nodeOps.createElement('div')
+    render(h(Comp), root)
+    expect(serializeInner(root)).toBe(`<div>fallback</div>`)
+    expect(calls).toEqual([])
+
+    // remvoe the async dep before it's resolved
+    toggle.value = false
+    await nextTick()
+    // should cause the suspense to resolve immediately
+    expect(serializeInner(root)).toBe(`<!---->`)
+
+    await Promise.all(deps)
+    await nextTick()
+    expect(serializeInner(root)).toBe(`<!---->`)
+    // should discard effects
+    expect(calls).toEqual([])
+  })
+
+  test.todo('unmount suspense after resolve')
+
+  test.todo('unmount suspense before resolve')
 
   test.todo('nested suspense')
 

packages/runtime-core/src/apiWatch.ts

@@ -113,6 +113,9 @@ function doWatch(
   } else {
     // no cb -> simple effect
     getter = () => {
+      if (instance && instance.isUnmounted) {
+        return
+      }
       if (cleanup) {
         cleanup()
       }
@@ -141,6 +144,9 @@ function doWatch(
   let oldValue = isArray(source) ? [] : undefined
   const applyCb = cb
     ? () => {
+        if (instance && instance.isUnmounted) {
+          return
+        }
         const newValue = runner()
         if (deep || newValue !== oldValue) {
           // cleanup before running cb again
@@ -157,20 +163,24 @@ function doWatch(
       }
     : void 0
 
-  const scheduler =
-    flush === 'sync'
-      ? invoke
-      : flush === 'pre'
-        ? (job: () => any) => {
-            if (!instance || instance.vnode.el != null) {
-              queueJob(job)
-            } else {
-              // with 'pre' option, the first call must happen before
-              // the component is mounted so it is called synchronously.
-              job()
-            }
-          }
-        : (job: () => any) => queuePostRenderEffect(job, suspense)
+  let scheduler: (job: () => any) => void
+  if (flush === 'sync') {
+    scheduler = invoke
+  } else if (flush === 'pre') {
+    scheduler = job => {
+      if (!instance || instance.vnode.el != null) {
+        queueJob(job)
+      } else {
+        // with 'pre' option, the first call must happen before
+        // the component is mounted so it is called synchronously.
+        job()
+      }
+    }
+  } else {
+    scheduler = job => {
+      queuePostRenderEffect(job, suspense)
+    }
+  }
 
   const runner = effect(getter, {
     lazy: true,

packages/runtime-core/src/createRenderer.ts

@@ -690,59 +690,146 @@ export function createRenderer<
     optimized: boolean
   ) {
     if (n1 == null) {
-      const suspense = (n2.suspense = createSuspenseBoundary(
+      mountSuspense(
         n2,
+        container,
+        anchor,
+        parentComponent,
         parentSuspense,
-        hostCreateElement('div'),
-        resolveSuspense
-      ))
-
-      function resolveSuspense() {
-        const { subTree, fallbackTree, effects, vnode } = suspense
-        // unmount fallback tree
-        if (fallback.el) {
-          unmount(fallbackTree as HostVNode, parentComponent, suspense, true)
-        }
-        // move content from off-dom container to actual container
-        move(subTree as HostVNode, container, anchor)
-        const el = (vnode.el = (subTree as HostVNode).el as HostNode)
-        // suspense as the root node of a component...
-        if (parentComponent && parentComponent.subTree === vnode) {
-          parentComponent.vnode.el = el
-          updateHOCHostEl(parentComponent, el)
-        }
-        // check if there is a pending parent suspense
-        let parent = suspense.parent
-        let hasUnresolvedAncestor = false
-        while (parent) {
-          if (!parent.isResolved) {
-            // found a pending parent suspense, merge buffered post jobs
-            // into that parent
-            parent.effects.push(...effects)
-            hasUnresolvedAncestor = true
-            break
-          }
+        isSVG,
+        optimized
+      )
+    } else {
+      patchSuspense(
+        n1,
+        n2,
+        container,
+        anchor,
+        parentComponent,
+        isSVG,
+        optimized
+      )
+    }
+  }
+
+  function mountSuspense(
+    n2: HostVNode,
+    container: HostElement,
+    anchor: HostNode | null,
+    parentComponent: ComponentInternalInstance | null,
+    parentSuspense: HostSuspsenseBoundary | null,
+    isSVG: boolean,
+    optimized: boolean
+  ) {
+    const suspense = (n2.suspense = createSuspenseBoundary(
+      n2,
+      parentSuspense,
+      hostCreateElement('div'),
+      resolveSuspense
+    ))
+
+    function resolveSuspense() {
+      if (__DEV__) {
+        if (suspense.isResolved) {
+          throw new Error(
+            `suspense.resolve() is called when it's already resolved`
+          )
         }
-        // no pending parent suspense, flush all jobs
-        if (!hasUnresolvedAncestor) {
-          queuePostFlushCb(effects)
+        if (suspense.isUnmounted) {
+          throw new Error(
+            `suspense.resolve() is called when it's already unmounted`
+          )
         }
-        suspense.isResolved = true
-        // invoke @resolve event
-        const onResolve = vnode.props && vnode.props.onResolve
-        if (isFunction(onResolve)) {
-          onResolve()
+      }
+      const { subTree, fallbackTree, effects, vnode } = suspense
+      // unmount fallback tree
+      if (fallbackTree.el) {
+        unmount(fallbackTree as HostVNode, parentComponent, suspense, true)
+      }
+      // move content from off-dom container to actual container
+      move(subTree as HostVNode, container, anchor)
+      const el = (vnode.el = (subTree as HostVNode).el as HostNode)
+      // suspense as the root node of a component...
+      if (parentComponent && parentComponent.subTree === vnode) {
+        parentComponent.vnode.el = el
+        updateHOCHostEl(parentComponent, el)
+      }
+      // check if there is a pending parent suspense
+      let parent = suspense.parent
+      let hasUnresolvedAncestor = false
+      while (parent) {
+        if (!parent.isResolved) {
+          // found a pending parent suspense, merge buffered post jobs
+          // into that parent
+          parent.effects.push(...effects)
+          hasUnresolvedAncestor = true
+          break
         }
       }
+      // no pending parent suspense, flush all jobs
+      if (!hasUnresolvedAncestor) {
+        queuePostFlushCb(effects)
+      }
+      suspense.isResolved = true
+      // invoke @resolve event
+      const onResolve = vnode.props && vnode.props.onResolve
+      if (isFunction(onResolve)) {
+        onResolve()
+      }
+    }
 
-      const { content, fallback } = normalizeSuspenseChildren(n2)
-      suspense.subTree = content
-      suspense.fallbackTree = fallback
-
-      // start mounting the content subtree in an off-dom container
-      // TODO should buffer postQueue jobs on current boundary
+    const { content, fallback } = normalizeSuspenseChildren(n2)
+    suspense.subTree = content
+    suspense.fallbackTree = fallback
+
+    // start mounting the content subtree in an off-dom container
+    patch(
+      null,
+      content,
+      suspense.container,
+      null,
+      parentComponent,
+      suspense,
+      isSVG,
+      optimized
+    )
+    // now check if we have encountered any async deps
+    if (suspense.deps > 0) {
+      // mount the fallback tree
       patch(
         null,
+        fallback,
+        container,
+        anchor,
+        parentComponent,
+        null, // fallback tree will not have suspense context
+        isSVG,
+        optimized
+      )
+      n2.el = fallback.el
+    } else {
+      // Suspense has no async deps. Just resolve.
+      suspense.resolve()
+    }
+  }
+
+  function patchSuspense(
+    n1: HostVNode,
+    n2: HostVNode,
+    container: HostElement,
+    anchor: HostNode | null,
+    parentComponent: ComponentInternalInstance | null,
+    isSVG: boolean,
+    optimized: boolean
+  ) {
+    const suspense = (n2.suspense = n1.suspense) as HostSuspsenseBoundary
+    suspense.vnode = n2
+    const { content, fallback } = normalizeSuspenseChildren(n2)
+    const oldSubTree = suspense.subTree
+    const oldFallbackTree = suspense.fallbackTree
+    if (!suspense.isResolved) {
+      patch(
+        oldSubTree,
         content,
         suspense.container,
         null,
@@ -751,74 +838,39 @@ export function createRenderer<
         isSVG,
         optimized
       )
-      // now check if we have encountered any async deps
       if (suspense.deps > 0) {
-        // mount the fallback tree
+        // still pending. patch the fallback tree.
         patch(
-          null,
+          oldFallbackTree,
           fallback,
           container,
           anchor,
           parentComponent,
-          suspense,
+          null, // fallback tree will not have suspense context
           isSVG,
           optimized
         )
         n2.el = fallback.el
-      } else {
-        // Suspense has no async deps. Just resolve.
-        suspense.resolve()
       }
+      // If deps somehow becomes 0 after the patch it means the patch caused an
+      // async dep component to unmount and removed its dep. It will cause the
+      // suspense to resolve and we don't need to do anything here.
     } else {
-      const suspense = (n2.suspense = n1.suspense) as HostSuspsenseBoundary
-      suspense.vnode = n2
-      const { content, fallback } = normalizeSuspenseChildren(n2)
-      const oldSubTree = (suspense.oldSubTree = suspense.subTree)
-      suspense.subTree = content
-      const oldFallbackTree = (suspense.oldFallbackTree = suspense.fallbackTree)
-      suspense.fallbackTree = fallback
-      if (!suspense.isResolved) {
-        patch(
-          oldSubTree,
-          content,
-          suspense.container,
-          null,
-          parentComponent,
-          suspense,
-          isSVG,
-          optimized
-        )
-        if (suspense.deps > 0) {
-          // still pending. patch the fallback tree.
-          patch(
-            oldFallbackTree,
-            fallback,
-            container,
-            anchor,
-            parentComponent,
-            suspense,
-            isSVG,
-            optimized
-          )
-          n2.el = fallback.el
-        } else {
-          suspense.resolve()
-        }
-      } else {
-        // just normal patch inner content as a fragment
-        patch(
-          oldSubTree,
-          content,
-          container,
-          anchor,
-          parentComponent,
-          suspense,
-          isSVG,
-          optimized
-        )
-        n2.el = content.el
-      }
+      // just normal patch inner content as a fragment
+      patch(
+        oldSubTree,
+        content,
+        container,
+        anchor,
+        parentComponent,
+        suspense,
+        isSVG,
+        optimized
+      )
+      n2.el = content.el
     }
+    suspense.subTree = content
+    suspense.fallbackTree = fallback
   }
 
   function processComponent(
@@ -913,10 +965,19 @@ export function createRenderer<
     // before proceeding
     if (__FEATURE_SUSPENSE__ && instance.asyncDep) {
       if (!parentSuspense) {
+        // TODO handle this properly
         throw new Error('Async component without a suspense boundary!')
       }
+      if (parentSuspense.isResolved) {
+        // TODO if parentSuspense is already resolved it needs to enter waiting
+        // state again
+      }
       parentSuspense.deps++
       instance.asyncDep.then(asyncSetupResult => {
+        // unmounted before resolve
+        if (instance.isUnmounted || parentSuspense.isUnmounted) {
+          return
+        }
         parentSuspense.deps--
         // retry from this component
         instance.asyncResolved = true
@@ -1481,12 +1542,7 @@ export function createRenderer<
     }
 
     if (__FEATURE_SUSPENSE__ && suspense != null) {
-      unmount(
-        suspense.subTree as HostVNode,
-        parentComponent,
-        parentSuspense,
-        doRemove
-      )
+      unmountSuspense(suspense, parentComponent, parentSuspense, doRemove)
       return
     }
 
@@ -1538,15 +1594,58 @@ export function createRenderer<
         stop(effects[i])
       }
     }
-    stop(update)
-    unmount(subTree, instance, parentSuspense, doRemove)
+    // update may be null if a component is unmounted before its async
+    // setup has resolved.
+    if (update !== null) {
+      stop(update)
+      unmount(subTree, instance, parentSuspense, doRemove)
+    }
     // unmounted hook
     if (um !== null) {
       queuePostRenderEffect(um, parentSuspense)
-      // set unmounted after unmounted hooks are fired
-      queuePostRenderEffect(() => {
-        instance.isUnmounted = true
-      }, parentSuspense)
+    }
+    queuePostFlushCb(() => {
+      instance.isUnmounted = true
+    })
+
+    // A component with async dep inside a pending suspense is unmounted before
+    // its async dep resolves. This should remove the dep from the suspense, and
+    // cause the suspense to resolve immediately if that was the last dep.
+    if (
+      __FEATURE_SUSPENSE__ &&
+      parentSuspense !== null &&
+      !parentSuspense.isResolved &&
+      !parentSuspense.isUnmounted &&
+      instance.asyncDep !== null &&
+      !instance.asyncResolved
+    ) {
+      parentSuspense.deps--
+      if (parentSuspense.deps === 0) {
+        parentSuspense.resolve()
+      }
+    }
+  }
+
+  function unmountSuspense(
+    suspense: HostSuspsenseBoundary,
+    parentComponent: ComponentInternalInstance | null,
+    parentSuspense: HostSuspsenseBoundary | null,
+    doRemove?: boolean
+  ) {
+    suspense.isUnmounted = true
+    unmount(
+      suspense.subTree as HostVNode,
+      parentComponent,
+      parentSuspense,
+      doRemove
+    )
+    if (!suspense.isResolved) {
+      unmount(
+        suspense.fallbackTree as HostVNode,
+        parentComponent,
+        parentSuspense,
+        doRemove
+      )
     }
   }
 

packages/runtime-core/src/suspense.ts

@@ -12,12 +12,11 @@ export interface SuspenseBoundary<
   vnode: HostVNode
   parent: SuspenseBoundary<HostNode, HostElement> | null
   container: HostElement
-  subTree: HostVNode | null
-  oldSubTree: HostVNode | null
-  fallbackTree: HostVNode | null
-  oldFallbackTree: HostVNode | null
+  subTree: HostVNode
+  fallbackTree: HostVNode
   deps: number
   isResolved: boolean
+  isUnmounted: boolean
   effects: Function[]
   resolve(): void
 }
@@ -33,11 +32,10 @@ export function createSuspenseBoundary<HostNode, HostElement>(
     parent,
     container,
     deps: 0,
-    subTree: null,
-    oldSubTree: null,
-    fallbackTree: null,
-    oldFallbackTree: null,
+    subTree: null as any, // will be set immediately after creation
+    fallbackTree: null as any, // will be set immediately after creation
     isResolved: false,
+    isUnmounted: false,
     effects: [],
     resolve
   }