Reanimated 1.3.0 Node values are updated when they shouldn't

[sebqq]

Hello there.

As I mentioned in #415, after 1.3.0 upgrade my animation node values are updated even when another node value (in different component) is changed (and no state change is made). Whats worse, even when I push a new screen and then go back, origins screen nodes values are changed. But all these problems disappears when I remove diffClamp method.

It happens only on 1.3.0. When I downgraded back to 1.2.0 the issue dissapears.

Gif - Behvaiour on 1.3.0:

Gif - Behvaiour on 1.2.0 or without using diffClamp method:

I'm enclosing reproducible demo https://github.com/sebinq/react-native-reanimated-bug

Steps to reproduce:

1. At HomeScreen, move any object just a little bit to any side.
2. Change Screen using provided button.
3. Go back to HomeScreen and you will see position is changed!

You can also see debug log in console that translation value was changed.

When you remove diffClamp in 'components/Movable.js' and try to use just withOffset, you can see that everything works fine!

Thank you so much for any help!

react-native 0.61.2
react-native-reanimated: 1.3.0
react-native-gesture-handler: 1.4.1
react-native-screens: 1.0.0-alpha.23
react-navigation-stack: 2.0.0-alpha.23
react-native-redash: 8.2.0

[sebqq]

I find that problem is with this commit ec1e29104dba9be504226aa9d54345405162ef4f. Without code from this commit,, there is no issue.

[sebqq]

For anyone who want to use 1.3.0 but cannot because of this issue you can try to apply following patch until someone solves the issue:

diff --git a/node_modules/react-native-reanimated/src/core/InternalAnimatedValue.js b/node_modules/react-native-reanimated/src/core/InternalAnimatedValue.js
index 3c22ded..46cfa92 100644
--- a/node_modules/react-native-reanimated/src/core/InternalAnimatedValue.js
+++ b/node_modules/react-native-reanimated/src/core/InternalAnimatedValue.js
@@ -8,16 +8,16 @@ function sanitizeValue(value) {
     : Number(value);
 }
 
-const CONSTANT_VALUES = new Map();
+// const CONSTANT_VALUES = new Map();
 
-function initializeConstantValues() {
-  if (CONSTANT_VALUES.size != 0) {
-    return;
-  }
-  [0, -1, 1, -2, 2].forEach(v =>
-    CONSTANT_VALUES.set(v, new InternalAnimatedValue(v, true))
-  );
-}
+// function initializeConstantValues() {
+//   if (CONSTANT_VALUES.size != 0) {
+//     return;
+//   }
+//   [0, -1, 1, -2, 2].forEach(v =>
+//     CONSTANT_VALUES.set(v, new InternalAnimatedValue(v, true))
+//   );
+// }
 
 /**
  * This class has been made internal in order to omit dependencies' cycles which
@@ -25,10 +25,15 @@ function initializeConstantValues() {
  */
 export default class InternalAnimatedValue extends AnimatedNode {
   static valueForConstant(number) {
-    initializeConstantValues();
-    return (
-      CONSTANT_VALUES.get(number) || new InternalAnimatedValue(number, true)
-    );
+    // initializeConstantValues();
+    // return (
+    //   CONSTANT_VALUES.get(number) || new InternalAnimatedValue(number, true)
+    // );
+    return new InternalAnimatedValue(number, true)
   }
 
   constructor(value, constant = false) {

Also by trying it we can let the maintainer know if it solves your issues too!

[sebqq]

So for now it looks like recycling same instance of InternalAnimatedValue for some basic values from [0, -1, 1, -2, 2] array is causing featured problems. @kmagiera @osdnk guys, what do you think about it?

[ShaMan123]

#415

[sebqq]

@ShaMan123 Unfortunately, I'm not able to wrap every single reanimated value in my code with onChange. It would be much better if we could try to solve the issue instead of hacking 😸

[ShaMan123]

Sure. Me neither.
I'll try your suggestion.
I tagged my issue to track yours.

[ShaMan123]

@sebinq You rock!

I can confirm that your patch resolves the issue for my use cases.

I tested the following:

A: causes recursive updates

export default class InternalAnimatedValue extends AnimatedNode {
+    static valueForConstant = _.memoize((number) => new InternalAnimatedValue(number, true));
...
}

B: solves the issue

export default class InternalAnimatedValue extends AnimatedNode {
+    static valueForConstant = (number) => new InternalAnimatedValue(number, true);
...
}

It seems that the intention was to memoize constant values to save on allocation but for some reason this causes the opposite behavior.

[sebqq]

@ShaMan123 thank you so much for your testing! Anyway, intention to memoize and reuse the instance of most often created values is just great. I hope that someone with more experience and knowledge can help and fix the problem 😄

[ShaMan123]

#403 was created to tackle this error I think

Warning: Please report: Excessive number of pending callbacks: 501. 
Some pending callbacks that might have leaked by never being called from native code: 

This warning shows when Fast Refresh occurs

[kmagiera]

Thanks @sebinq for reporting and working on a repro case. It's been really helpful in investigating this very obscure bug. Just submitted a fix and we will likely release it some time soon.

Also many thanks to @ShaMan123 for participating in the discussion and figuring out workarounds 🙌

[sebqq]

@kmagiera you don't even know how glad am I to hear this news 🥂 thank you so much!