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

<Transition> wrapped by <Suspense> breaks entirely if interrupted before it completes #8105

Closed
danielroe opened this issue Apr 17, 2023 · 10 comments · Fixed by #9309
Closed
Labels
has PR A pull request has already been submitted to solve the issue 🐞 bug Something isn't working scope: suspense scope: transition

Comments

@danielroe
Copy link
Member

danielroe commented Apr 17, 2023

Vue version

3.2.47

Link to minimal reproduction

https://stackblitz.com/edit/github-z3ry59-hbu5x7
SFC Playground

Steps to reproduce

<template>
  <transition name="page" mode="out-in" :duration="300">
    <Suspense>
      <component :is="Component" />
    </Suspense>
  </transition>
</template>

Click the button marked 'Trigger error'.

This will switch components within the Transition. On next tick, it will switch them back. (To reproduce, it's sufficient to switch them at any point before the transition has finished.)

Note that this follows the component order specified in https://vuejs.org/guide/built-ins/suspense.html#combining-with-other-components.

What is expected?

I expect no errors.

What is actually happening?

The following error is thrown:

Uncaught DOMException: Failed to execute 'insertBefore' on 'Node': The node before which the new node is to be inserted is not a child of this node.

In addition, the content of the Suspense slot is removed and the page remains broken.

System Info

No response

Any additional comments?

No response

@posva
Copy link
Member

posva commented Apr 18, 2023

This rings a bell, there could be another open issue about interrupting a transition here but I couldn't find it. It looks similar to #6835 but clearly not the same.

@kikuchan
Copy link

After some investigations, I've found a workaround to relief the issue with the following patch.

--- packages/runtime-dom/src/nodeOps.ts
+++ packages/runtime-dom/src/nodeOps.ts
@@ -6,8 +6,18 @@ const doc = (typeof document !== 'undefined' ? document : null) as Document

 const templateContainer = doc && /*#__PURE__*/ doc.createElement('template')

+function isSafeAnchor(parent: Element, anchor: Node | null | undefined) {
+  if (!anchor) return true;
+
+  for (let node: Node | null = anchor; node; node = node.parentNode) {
+    if (parent === node) return true;
+  }
+  return false;
+}
+
 export const nodeOps: Omit<RendererOptions<Node, Element>, 'patchProp'> = {
   insert: (child, parent, anchor) => {
+    if (!isSafeAnchor(parent, anchor)) anchor = null;
     parent.insertBefore(child, anchor || null)
   },

It also works on nuxt/nuxt#13350

I also came up with logs that what's happend internally;
(using modified reproduction code that toggle only toggles components once)

Good case:

  1. click the toggle button
  2. insert B to hiddenContainer created in mountSuspense (creation?)
  3. remove A from real DOM
  4. insert B to real DOM, without anchor
  5. wait for a while, and click the toggle button again.
  6. insert A to hiddenContainer created in mountSuspense (creation?)
  7. remove B from real DOM
  8. insert A to real DOM, without anchor

Bad case:

  1. click the toggle button
  2. insert B to hiddenContainer created in mountSuspense (creation?)
  3. click the toggle button again quickly, then component swap occurs again too rapidly.
  4. insert A to the same (!?) hiddenContainer created in mountSuspense (creation?)
  5. remove B from the hiddenContainer
  6. remove A from real DOM
  7. insert A to real DOM, with anchor A (!)

I hope it helps.

@kikuchan
Copy link

const delayEnter =
activeBranch &&
pendingBranch!.transition &&
pendingBranch!.transition.mode === 'out-in'
if (delayEnter) {
activeBranch!.transition!.afterLeave = () => {
if (pendingId === suspense.pendingId) {
move(pendingBranch!, container, anchor, MoveType.ENTER)
}
}
}
// this is initial anchor on mount
let { anchor } = suspense
// unmount current active tree
if (activeBranch) {
// if the fallback tree was mounted, it may have been moved
// as part of a parent suspense. get the latest anchor for insertion
anchor = next(activeBranch)
unmount(activeBranch, parentComponent, suspense, true)
}
if (!delayEnter) {
// move content from off-dom container to actual container
move(pendingBranch!, container, anchor, MoveType.ENTER)
}

The issue happens when activeBranch is truthy, and regardless of whether delayEnter is true or not.
And, I think the anchor re-selection in the if (activeBranch) block needs to be robust (but I don't know how, yet)

Just to be confirmed, removing anchor = next(activeBranch) also resolves the issue without the previous patch. (it fails some tests)

@kikuchan
Copy link

After deep investigation, I've finally found the solution.

--- packages/runtime-core/src/renderer.ts
+++ packages/runtime-core/src/renderer.ts
@@ -2035,6 +2035,7 @@ function baseCreateRenderer(
     if (needTransition) {
       if (moveType === MoveType.ENTER) {
         transition!.beforeEnter(el!)
+        if (anchor && anchor.parent !== container) anchor = null;
         hostInsert(el!, container, anchor)
         queuePostRenderEffect(() => transition!.enter(el!), parentSuspense)
       } else {

The beforeEnter internally calls afterLeave hooks, and it actually removes the anchor from the container before use, thus hostInsert claims it doesn't exist in the tree.

Perhaps, it should be re-calculate anchor here, or you could call afterLeave hooks manually to remove a transition node before anchor selection, but I think it's hard to do so here.

kikuchan added a commit to kikuchan/vuejs-core that referenced this issue Jun 26, 2023
kikuchan added a commit to kikuchan/vuejs-core that referenced this issue Jun 26, 2023
@kikuchan kikuchan mentioned this issue Jun 26, 2023
kikuchan added a commit to kikuchan/vuejs-core that referenced this issue Jun 27, 2023
@kikuchan
Copy link

I've posted a PR (based on another idea), and I've noticed that the same pattern causes the same error, btw.

For example, following lines cause a HMR issue. (failed to reload a page sometimes)

anchor = getNextHostNode(n1)
unmount(n1, parentComponent, parentSuspense, true)

After patching the same way in the PR like this, it has been resolved.

      const anchorCands: RendererNode[] = []
      for (let node = getNextHostNode(n1); node; node = hostNextSibling(node)) {
        anchorCands.push(node)
      }
      unmount(n1, parentComponent, parentSuspense, true)
      anchor = anchorCands.find(x => hostParentNode(x) === container) || null;

In current code, unmount calls vnode's hooks internally, and they could remove multiple nodes at once.
Thus, I think saving a next node as an anchor before unmount is not a safe opearation anymore.

@kikuchan
Copy link

I'm trying to explain what's going on under the hood.

For this issue, there are 2 cases:

  1. In this case, the activeBranch is still in the hiddenContainer (I don't know if this is a valid state).
    Then, move tries to move the pendingBranch where the activeBranch is, and it fails because the anchor (where the activeBranch was) is not in the container.

  2. In the !delayEnter case, the anchor is properly in the container, but move triggers transition.beforeEnter and hooks alter the DOM tree, thus the anchor no longer exists in the container on the very next hostInsert.
    This could be another issue; it happens with my code at least, but I haven't created a minimal test code yet.

Additionally:

  1. This is clearly another issue. unmount removes multiple nodes at once, so capturing a single anchor before the unmount is not a safe operation, especially reloading by HMR causes it sometimes (I haven't found out the issue number yet).

AasmundN added a commit to et-dagen/et-dagen that referenced this issue Aug 30, 2023
Fix page transitions. Turns out there is a bug in Vue itself. Fixed by adding a div to make sure there
is only one root element. vuejs/core#8105

Localization of route navigation was missing in a few places.

Update nuxt dev server to host on local network. This way the application can be viewed on mobile.

Fix a-tag underlines showing on mobile. Also added global style sheet, main.scss.
@pikax pikax added the has PR A pull request has already been submitted to solve the issue label Oct 13, 2023
@avaz
Copy link

avaz commented Dec 4, 2023

Yay!! 🙌🏼

@vanling
Copy link

vanling commented Dec 4, 2023

🎉 Thank you @edison1105

@bgrand-ch
Copy link

Thank you to everyone involved in resolving this problem 🙏

@dargmuesli
Copy link

When I opened my notifications today

Alt Text

Thanks a lot everyone and thank you @yyx990803 for getting the merge train rolling!

@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
has PR A pull request has already been submitted to solve the issue 🐞 bug Something isn't working scope: suspense scope: transition
Projects
None yet
8 participants