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

[BUG] Animations do not resume after components resume after suspense #2269

Open
Pinpickle opened this issue Aug 1, 2023 · 10 comments
Open
Labels
bug Something isn't working

Comments

@Pinpickle
Copy link

Pinpickle commented Aug 1, 2023

1. Read the FAQs πŸ‘‡

βœ…

2. Describe the bug

Animations that are interrupted with suspense have weird behaviour. Sometimes they will freeze in place. Sometimes they will freeze at the beginning. Sometimes they will complete.

3. IMPORTANT: Provide a CodeSandbox reproduction of the bug

I made a StackBlitz, hopefully that's OK: https://stackblitz.com/edit/vitejs-vite-uhm4h9?file=src%2FApp.tsx

4. Steps to reproduce

Steps to reproduce the behavior:

  1. Wait for the animation to show "suspended"
  2. Notice how the first animation is frozen in the wrong spot at the end

5. Expected behavior

For the animation to complete eventually once the suspended component re-mounts. Any one of these would be reasonable IMO:

  • Proceed the animation by the amount of time the component has been suspended (so if it suspends at t=0.5 for 1 second, it resumes at t=1.5)
  • Start the animation again
  • Mount with the animation completed

There would be different behaviours for repeating animations and unmounting animations.

6. Video or screenshots

CleanShot.2023-08-01.at.14.25.16.png.mp4

7. Environment details

Chrome on macOS (M1)
Version 114.0.5735.198 (Official Build) (arm64)

@Pinpickle Pinpickle added the bug Something isn't working label Aug 1, 2023
@Pinpickle Pinpickle changed the title [BUG] [BUG] Animations do not resume after components resume after suspense Aug 1, 2023
@gurkerl83
Copy link

gurkerl83 commented Aug 2, 2023

@Pinpickle Hi, it seems the following SuspenseTest component works, it uses a ref to store the promise instead of a state, not forcing any re-render when setting or resetting.

import { useEffect, useRef } from 'react';
import { motion } from 'framer-motion';
export const SuspenseTest = ({ shouldSuspend }: { shouldSuspend: boolean }) => {
  const promiseRef = useRef<null | Promise<void>>(null);
  const resolvePromiseTimeoutRef = useRef<null | NodeJS.Timeout>(null);

  useEffect(() => {
    if (shouldSuspend) {
      const setupPromiseTimeout = setTimeout(() => {
        promiseRef.current = new Promise(resolve => {
          resolvePromiseTimeoutRef.current = setTimeout(() => {
            resolve();
            promiseRef.current = null;
          }, 1000);
        });
      }, 1000);

      return () => {
        clearTimeout(setupPromiseTimeout);
        if (resolvePromiseTimeoutRef.current != null) {
          clearTimeout(resolvePromiseTimeoutRef.current);
        }
      };
    }
  }, []);

  if (shouldSuspend && promiseRef.current) {
    throw promiseRef.current;
  }

  return (
    <motion.div
      style={{ fontWeight: 'bold' }}
      initial={{ opacity: 0.2, scale: 1 }}
      animate={{ opacity: 1, scale: 2 }}
      transition={{ duration: 2 }}
    >
      {shouldSuspend ? 'With suspense' : 'Without suspense'}
    </motion.div>
  );
};

@Pinpickle
Copy link
Author

Pinpickle commented Aug 2, 2023

@gurkerl83 you're right that this no longer causes the animation to become stuck. But that's because the component never suspends, so it just means it's no longer running into the bug in Motion.

In this example you can technically create the promise completely outside of React, but this was just a contrived example to get to causing suspense as fast as possible.

A more useful example could be using React.lazy, which triggers suspense:

const LazyDisplayNumber2 = lazy(async () => {
  // artificial network delay
  await new Promise((r) => setTimeout(r, 1000));
  return import("./DisplayNumber2");
});

const SuspenseTest = ({ shouldSuspend }: { shouldSuspend: boolean }) => {
  const [isDisplaying, setIsDisplaying] = useState(false);

  useEffect(() => {
    // We need to make sure the component manages to mount
    // and start the animation before triggering suspense.
    setIsDisplaying(true);
  }, []);

  return (
    <div style={{ display: "flex", justifyContent: "center" }}>
      <motion.div
        style={{ fontWeight: "bold" }}
        initial={{ opacity: 0.2, scale: 1 }}
        animate={{ opacity: 1, scale: 2 }}
        transition={{ duration: 2 }}
      >
        {isDisplaying && shouldSuspend ? <LazyDisplayNumber2 /> : "1"}
      </motion.div>
    </div>
  );
};

This has the same problem of "getting stuck"

Codesandbox (I couldn't get Stackblitz to work this time πŸ™ƒ): https://codesandbox.io/s/vigorous-lamarr-sjssxd?file=/src/App.tsx

@gurkerl83
Copy link

gurkerl83 commented Aug 2, 2023

You are absolutely right, the promise is never thrown, at least not when no state change gets triggered during the promise gets created and resolves in the timeout.

Working with Suspense often requires the usage of the useTransition hook. Props put into a component wrapped by a suspense boundary require doing a state update starting a transition. But in the second example state update is executed within. Maybe lazy automatically adds the Suspense component over LazyDisplayNumber2, then this makes sense.

From the new example is this what you want to achieve?

You need useTransition from react. Suspense and Transition has improved over the last year dramatically I recommend installing a 18.3 canary version of react and react-dom.

const SuspenseTest = ({ shouldSuspend }: { shouldSuspend: boolean }) => {
  const [isDisplaying, setIsDisplaying] = useState(false);

  const [, startTransition] = useTransition();

  useEffect(() => {
    if (shouldSuspend) {
      startTransition(() => {
        setIsDisplaying(true);
      });
    }
  }, []);

  return (
    <div style={{ display: "flex", justifyContent: "center" }}>
      <motion.div
        style={{ fontWeight: "bold" }}
        initial={{ opacity: 0.2, scale: 1 }}
        animate={{ opacity: 1, scale: 2 }}
        transition={{ duration: 5 }}
      >
        {isDisplaying && shouldSuspend ? <LazyDisplayNumber2 /> : "1"}
      </motion.div>
    </div>
  );
};

@Pinpickle
Copy link
Author

@gurkerl83 there are plenty of workarounds to make sure a component doesn't suspend in isolated cases.

Regardless, I'm pretty sure this is a bug in Framer Motion and (in my opinion), animations should still work even if they've been interrupted with suspense.

@mattgperry
Copy link
Collaborator

The technical reason behind this looks like when the component throws, the motion.div doesn't re-render but its ref function is passed null (an unmount signal). Then, when the component stops throwing, the motion.div isn't re-rendered or effects don't run, but the ref function does fire. Which is weird IMO. I'll need to get a better handle on React's behaviour here before making any fixes as these lifecycles are in place usually for quite specific reasons.

@Pinpickle
Copy link
Author

@mattgperry thanks for investigating! Any suggestions for workarounds in the meantime? Is there a way to force an animation to restart?

@gurkerl83
Copy link

gurkerl83 commented Aug 8, 2023

@mattgperry @Pinpickle Found the issue, with the ref problem, please the the comments made in the component.

export function useMotionRef<Instance, RenderState>(

export function useMotionRef<Instance, RenderState>(
    visualState: VisualState<Instance, RenderState>,
    visualElement?: VisualElement<Instance> | null,
    externalRef?: React.Ref<Instance>
): React.Ref<Instance> {
    

    useEffect(() => {
        // 2. Requirement
        return () => {
            if (visualElement) {
                visualElement.unmount()
            }
        }
    }, [visualElement])

    return useCallback(
        (instance: Instance) => {
            /** Original Code
            instance && visualState.mount && visualState.mount(instance)

            if (visualElement) {
                instance
                    ? visualElement.mount(instance)
                    : visualElement.unmount()
            */

            /** Pseude Code 
            if (instance && visualState.mount) {
                console.log("visual - State.mount: ", instance)
                visualState.mount(instance)
            }

            if (visualElement) {
                if (instance) {
                    console.log("visual - Element.mount: ", instance)
                    visualElement.mount(instance)
                } else {
                    // the refs corresponding element (instance) was detach from dom
                    // hits when the something suspense, a promise or thenable was catched by a SuspenseBoundary outside of a motion component
                    // immediate after the initial render e.g. using React.lazy()
                    console.log("visual - Element.unmount: ", instance)
                    visualElement.unmount()
                }
            }
            */

            // Workaround
            // 1. Mount visualelement and visualstate in callback
            // 2. Unmount visualelement when the component unmounts in effect cleanup

            // 1. Requirement
            if (instance) {
                visualState.mount && visualState.mount(instance)
                visualElement && visualElement.mount(instance)
            }

            if (externalRef) {
                if (typeof externalRef === "function") {
                    externalRef(instance)
                } else if (isRefObject(externalRef)) {
                    ;(externalRef as any).current = instance
                }
            }
        },
        /**
         * Only pass a new ref callback to React if we've received a visual element
         * factory. Otherwise we'll be mounting/remounting every time externalRef
         * or other dependencies change.
         */
        [visualElement]
    )
}

The final hook looks like this

import * as React from "react"
import { useCallback, useEffect } from "react"
import type { VisualElement } from "../../render/VisualElement"
import { isRefObject } from "../../utils/is-ref-object"
import { VisualState } from "./use-visual-state"

/**
 * Creates a ref function that, when called, hydrates the provided
 * external ref and VisualElement.
 */
export function useMotionRef<Instance, RenderState>(
    visualState: VisualState<Instance, RenderState>,
    visualElement?: VisualElement<Instance> | null,
    externalRef?: React.Ref<Instance>
): React.Ref<Instance> {
    /**
     * This React effect ensures the systematic unmounting of 'visualElement'
     * during the component's unmount phase. This cleanup is especially pivotal
     * in contexts where the component's rendering might have been affected by asynchronous
     * operations, such as with React.lazy and Suspense.
     *
     * Given that 'visualElement' animations are allowed to continue even when certain
     * child components might be rendered invalid by promises from React.lazy, it becomes
     * paramount to ensure that resources or side-effects associated with this DOM node
     * are properly managed and cleaned up to avoid potential pitfalls.
     */
    useEffect(() => {
        return () => {
            if (visualElement) {
                visualElement.unmount()
            }
        }
    }, [visualElement])

    return useCallback(
        (instance: Instance) => {
            /**
             * This section manages the lifecycle of 'visualState' and 'visualElement' based on
             * the presence of the 'instance' variable, which corresponds to a real DOM node.
             *
             * - When a valid DOM node (represented by 'instance') is detected, both 'visualState'
             *   and 'visualElement' are triggered to mount. This signifies the preparation or
             *   setup of visual components or states based on the detected node.
             *
             * - A complex scenario emerges when 'instance' becomes null, particularly within
             *   the environment of an outer Suspense boundary. With React.lazy, components are
             *   loaded lazily and the promises (or thenables) might render certain child components
             *   invalid based on their resolution or rejection. This can lead to situations where
             *   the expected DOM node isn't available. Yet, in these cases, the 'visualElement'
             *   doesn't get immediately unmounted. Animations tied to it persist, maintaining a
             *   consistent visual experience.
             */
            if (instance) {
                visualState.mount && visualState.mount(instance)
                visualElement && visualElement.mount(instance)
            }

            if (externalRef) {
                if (typeof externalRef === "function") {
                    externalRef(instance)
                } else if (isRefObject(externalRef)) {
                    ;(externalRef as any).current = instance
                }
            }
        },
        /**
         * Only pass a new ref callback to React if we've received a visual element
         * factory. Otherwise we'll be mounting/remounting every time externalRef
         * or other dependencies change.
         */
        [visualElement]
    )
}

@gurkerl83
Copy link

gurkerl83 commented Aug 8, 2023

While framer-motion can execute the animation with the current changes, it doesn't truly resume the animation. Specifying Suspense outside of a motion component doesn't seem advantageous. Without modifying any sources, the same outcome can be achieved by integrating Suspense directly within the component and subsequently using lazy loading. By doing this, the promise or "thenable" throw is confined specifically to this Suspense boundary, ensuring that framer-motion remains uninterrupted.

For illustrative purposes, please avoid using an external Suspense component in this example.

import { motion } from 'framer-motion';
import { useState, useEffect, FC, lazy, Suspense } from 'react';

function delayForDemo(promise) {
  return new Promise(resolve => {
    setTimeout(resolve, 2000);
  }).then(() => promise);
}

const LazyDisplayNumber2 = lazy(() => delayForDemo(import('./DisplayNumber2')));

export const SuspenseTest: FC<{ shouldSuspend: boolean }> = ({
  shouldSuspend
}) => {
  const [isDisplaying, setIsDisplaying] = useState(false);

  useEffect(() => {
    // We need to make sure the component manages to mount
    // and start the animation before triggering suspense.
    setIsDisplaying(true);
  }, []);

  return (
    <div style={{ display: 'flex', justifyContent: 'center' }}>
      <motion.div
        style={{ fontWeight: 'bold' }}
        initial={{ opacity: 0.2, scale: 1 }}
        animate={{ opacity: 1, scale: 2 }}
        transition={{ duration: 4 }}
      >
        {isDisplaying && shouldSuspend ? (
          <Suspense fallback={<div>Suspended</div>}>
            <LazyDisplayNumber2 />
          </Suspense>
        ) : (
          1
        )}
      </motion.div>
    </div>
  );
};

@gurkerl83
Copy link

The provided PR #2283 is certainly not the right approach to fix this problem.

@Pinpickle
Copy link
Author

@mattgperry I don't suppose you've had any time to further investigate this? The bug is particularly problematic with AnimatePresence where a suspense can prevent an element from unmounting successfully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants