nandorojo / moti

šŸ¼ The React Native (+ Web) animation library, powered by Reanimated 3.
https://moti.fyi
MIT License
3.9k stars 120 forks source link

0.25 and reanimated 2 #282

Closed enagorny closed 11 months ago

enagorny commented 1 year ago

Is there an existing issue for this?

Current Behavior

Even latest expo is still using Reanimated 2. So after installing moti 0.25.1 the app crashes

TypeError: undefined is not a function
node modules/moti/src/core/use-motify.ts
...

This errors make it impossible to use latest library with expo. Is there way to make it work? Otherwise please update the installation docs to reflect it.

Expected Behavior

Should not crash

Steps To Reproduce

  1. Install latest expo
  2. use reanimated version which expo supports (npx expo install --check)
  3. build and run the app

Versions

- Moti: 0.25.1
- Reanimated: 2.14.4
- React Native:0.71.7
- Expo:48.0.15

Screenshots

SCR-20230427-bbzv

Reproduction

asdf

jasonkahkeat commented 1 year ago

I'm facing a similar issue as well, and app was built via create-react-app. It started occurring once I upgraded my React Native to 0.71.7, moti version 0.25.1 and reanimated version 3.1.0. Reinstalled these packages and cleared cache but the error remained.

Error Message on Terminal

ReanimatedError: undefined is not a function, js engine: reanimated

Source

File location: src/core/use-motify.ts const sequenceValue = stepAnimation( stepValue, stepConfig as any, (completed = false, maybeValue) => { callback(completed, maybeValue, { attemptedSequenceValue: stepValue, }) if (stepOnDidAnimate) { runOnJS(stepOnDidAnimate)(completed, maybeValue, { attemptedSequenceItemValue: stepValue, attemptedSequenceArray: maybeValue, }) } } )

enagorny commented 1 year ago

Downgrading to 0.24.2 fixes the issue

jasonkahkeat commented 1 year ago

Downgrading to 0.24.2 fixes the issue

Alright seems to work for me. Thanks!

nandorojo commented 1 year ago

looking into this

nandorojo commented 1 year ago

it's odd because the file does exist

https://github.com/nandorojo/moti/blob/master/packages/moti/src/core/use-motify.ts

nandorojo commented 1 year ago

i think i have an idea, can you share the code you used that broke it?

nandorojo commented 1 year ago

thanks @jasonkahkeat for the source line, that helps

nandorojo commented 1 year ago

Can you guys provide a minimal reproduction for me to look at

nandorojo commented 1 year ago

Just pushed 0.25.3, won't be sure if it fixes it until you try it or I have a minimal reproduction.

jasonkahkeat commented 1 year ago

Can you guys provide a minimal reproduction for me to look at

Sure thing, here's an example where the error occurred.

return (
  <MotiView
    from={{ translateY: 50 }}
    animate={{ translateY: 0 }}
    style={styles.container}
    transition={{
      type: 'timing',
      duration: 500,
      easing: Easing.inOut(Easing.linear),
    }}
  >
    <View>
        //other contents
    </View>
  </MotiView>
)

I inherited an older codebase and was in the process of upgrading everything, but nothing seems out of the ordinary to me. Meanwhile I'll try out the latest version to see if it works.

d4rky-pl commented 1 year ago

@nandorojo unfortunately the issue still persist on 0.25.3. Here's a minimal repro: https://github.com/d4rky-pl/moti-bug

I could confirm the issue happening on both iOS (Simulator) and Android (real device), same stack trace as the original reporter.

nandorojo commented 1 year ago

thanks for sharing that, i'll try to address it when i have some time. should be somewhat easy to debug

d4rky-pl commented 1 year ago

@nandorojo I believe this is a Hermes bug. The offending change is in this line:

sequenceArray.forEach((step) => {

which replaced previous

 for (const step of sequenceArray) {

From my debugging it seems that animation argument passed to getSequenceArray is no longer visible inside the function passed to forEach. If you assign it to a temporary variable though, that variable works properly (scoping issue?):

const getSequenceArray = (
  sequenceKey: string,
  sequenceArray: SequenceItem<any>[],
  delayMs: number | undefined,
  config: object,
  animation: (...props: any) => any,
  callback: (
    completed: boolean | undefined,
    value: any | undefined,
    info: {
      attemptedSequenceValue: any
    }
  ) => void
) => {
  'worklet'

  const temporaryAnimationConst = animation
  console.log('temporaryAnimationConst', temporaryAnimationConst)
  console.log('animation', animation)

  const sequence: any[] = []

  sequenceArray.forEach((step) => {
    console.log('temporaryAnimationConst inside forEach', temporaryAnimationConst)
    console.log('animation inside forEach', animation)

This results in:

 LOG  temporaryAnimationConst [Function hostFunction]
 LOG  animation [Function hostFunction]
 LOG  temporaryAnimationConst inside forEach [Function hostFunction]
 LOG  animation inside forEach undefined

I'm afraid I don't have enough experience with either Hermes nor reanimated worklets to figure out why is this happening and if there should be a follow-up bug report to either of the libraries

nandorojo commented 1 year ago

interestingā€¦what if you add ā€˜workletā€™ inside of the each function? sounds like a reanimated worklet quirk

d4rky-pl commented 1 year ago

@nandorojo doesn't solve the problem šŸ„²

Until you have time to figure out exactly why this is exploding, this patch solves the issue for me and I couldn't find any negative side effects:

diff --git a/node_modules/moti/src/core/use-motify.ts b/node_modules/moti/src/core/use-motify.ts
index 0b0680c..a95c5ae 100644
--- a/node_modules/moti/src/core/use-motify.ts
+++ b/node_modules/moti/src/core/use-motify.ts
@@ -244,7 +244,7 @@ const getSequenceArray = (

   const sequence: any[] = []

-  sequenceArray.forEach((step) => {
+  for (const step of sequenceArray) {
     const shouldPush =
       typeof step === 'object'
         ? step && step?.value != null && step?.value !== false
@@ -302,7 +302,7 @@ const getSequenceArray = (
         sequence.push(sequenceValue)
       }
     }
-  })
+  }

   return sequence
 }
nandorojo commented 1 year ago

funny enough, i changed that line to solve hermes issues that previously arose with for (const. if that fixes it, feel free to PR

d4rky-pl commented 1 year ago

@nandorojo do you have a link or any reference to the previous issue? I can see if I can confirm that the patch above doesn't introduce a regression

nandorojo commented 1 year ago

see 0.22 release notes. however, these changes didnā€™t do anything in the getSequenceArray function I believe. so itā€™s probably save to revert the changes there.

https://github.com/nandorojo/moti/releases/tag/v0.22.0

nandorojo commented 11 months ago

should be fixed in 0.25.4