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

Add failing test case for nested components inside StrictMode #335

Merged

Conversation

amannn
Copy link
Contributor

@amannn amannn commented Sep 24, 2019

I encountered an error in an app that I'm working on and isolated it to the test case in this PR.

When you run the test, you get this error:

Uncaught Error: No valid node provided. Node must be HTMLElement, SVGElement or window.
    at Object.exports.invariant (index.js:15)
    at createDOMStyler (index.js:536)
    at getStyler (index.js:541)
    at index (index.js:547)
    at checkAndConvertChangedValueTypes (framer-motion.cjs.js:3198)
    at unitConversion (framer-motion.cjs.js:3295)
    at ValueAnimationControls.eval [as makeTargetAnimatable] (framer-motion.cjs.js:3304)
    at ValueAnimationControls.animate (framer-motion.cjs.js:1269)
    at getAnimations (framer-motion.cjs.js:1313)
    at ValueAnimationControls.animateVariant (framer-motion.cjs.js:1337)
    at eval (framer-motion.cjs.js:1354)
    at Array.forEach (<anonymous>)
    at ValueAnimationControls.animateChildren (framer-motion.cjs.js:1353)
    at getChildrenAnimations (framer-motion.cjs.js:1317)
    at ValueAnimationControls.animateVariant (framer-motion.cjs.js:1337)
    at eval (framer-motion.cjs.js:1301)
    at Array.map (<anonymous>)
    at ValueAnimationControls.animateVariantLabels (framer-motion.cjs.js:1301)
    at ValueAnimationControls.start (framer-motion.cjs.js:1243)
    at eval (framer-motion.cjs.js:3785)

You can work around the error by:

  1. Removing StrictMode.
  2. Changing animate={useAnimation()} to animate="visible".
  3. Animating opacity instead of y in the inner component.

The stack trace goes into stylefire. I thought maybe you have an idea where the bug could originate from?

Maybe there's an unexpected side effect? If so, it could be helpful to fix this in order to work with the upcoming concurrent mode of React.

@mattgperry
Copy link
Collaborator

mattgperry commented Sep 27, 2019

Great PR. It looks to me like an animation can be invoked in strict mode before the component has mounted.

@amannn
Copy link
Contributor Author

amannn commented Oct 1, 2019

I had another look.

So the main issue is that here:

const elementStyler = styler(element)

… the element is null. Note that in the test we render an outer and an inner motion component. In the outer component element is defined, but it's the inner one where it's null.

It seems like in my test the variant changes from [] to ['visible'] in the nested component.

In this code:

} else {
shouldAnimate =
hasMounted.current ||
hasVariantChanged(resolveVariantLabels(initial), targetVariants)
}

shouldAnimate is true, because the variant has changed.

Is it intentional that it's hasMounted.current ||? If I replace || with &&, my test passes. However it breaks a few tests of useVariants.

I'm struggling a bit to understand what hasMounted expresses in the useVariant hook. It's set to true once the useEffect hook has run for the first time. Is this because useEffect fires in children first and later in the parent?

I feel like the inheritance of variants get's a bit in the way in this case, so I also opened #343 which is something I've encountered in a different scenario as well.

… can easily be differentiated from the outer component in logs.
@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 1, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b86dc9f:

Sandbox Source
Framer Motion: Simple animation Configuration

@amannn
Copy link
Contributor Author

amannn commented Oct 2, 2019

I just did a quick check: Passing inherit={false} to the nested component also avoids the issue.

@mattgperry mattgperry merged commit d4e0a4f into framer:master Nov 21, 2019
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants