motiondivision / motionone

https://motion.dev
MIT License
2.86k stars 52 forks source link

[Bug] Exit animations in Solid.js >= 1.5.0 don't work #135

Closed fli closed 2 years ago

fli commented 2 years ago

1. Describe the bug

Exit animations in Solid.js >= 1.5.0 don't work. It was released recently so wasn't really expecting it to work but just thought it was worth flagging.

2. IMPORTANT: Provide a CodeSandbox reproduction of the bug

https://codesandbox.io/s/sandpack-project-forked-78xcg2

3. Steps to reproduce

Click on toggle and watch it animate in and not animate out.

You can change solid.js to version 1.4.x and it will work again.

4. Expected behavior

Should animate out

mdynnl commented 2 years ago

batching behavior changed in 1.5 https://playground.solidjs.com/?hash=-423275382&version=1.4.8 https://playground.solidjs.com/?hash=1236122958&version=1.5.3

Basically it should work if these lines are swapped https://github.com/motiondivision/motionone/blob/24292336e5c7f797e9fbfa6e42160337968a9afe/packages/solid/src/presence.tsx#L92-L93

https://github.com/motiondivision/motionone/blob/24292336e5c7f797e9fbfa6e42160337968a9afe/packages/solid/src/presence.tsx#L97-L98

with new batching, el() here will get the new value updated beforehand. https://github.com/motiondivision/motionone/blob/24292336e5c7f797e9fbfa6e42160337968a9afe/packages/solid/src/presence.tsx#L120

vxncetxn commented 2 years ago

@mdynnl Your fix here works for me! I think you should open a PR to get this in.

EDIT: Oops upon further testing, I found that the fix only works if exitBeforeEnter is not specified. I will take another look at it later.

EDIT 2: Just one slight modification to your proposed fixes for exitBeforeEnter to work, setEl() on line 92 should be removed. So ultimately, the batching part of the code in presence.tsx should look like this:

batch(() => {
  // exit -> enter
  if (props.exitBeforeEnter) {
    exitTransition(() => !exiting && enterTransition(newEl))
  }
  // exit & enter
  else {
    exitTransition()
    enterTransition(newEl)
  }
})

@mdynnl Thanks for figuring this out! Please open a PR to push your fixes in! 😊

mattgperry commented 2 years ago

This should be fixed in 10.14.2!