software-mansion / react-native-reanimated

React Native's Animated library reimplemented
https://docs.swmansion.com/react-native-reanimated/
MIT License
8.86k stars 1.29k forks source link

Strict mode bug: TypeError: Cannot assign to read-only property 'callbackId' #5525

Closed osamaqarem closed 9 months ago

osamaqarem commented 9 months ago

Description

Specifically when there is a framecallback.setActive invocation within a Gesture callback, the error is thrown

Temporarily removing 'use strict' from the hook fixes the problem:

https://github.com/software-mansion/react-native-reanimated/blob/f40ee24ebf31424ec7f75b58c0d80e34b13dae29/src/reanimated2/hook/useFrameCallback.ts#L1

Steps to reproduce

  const frameCb = useFrameCallback(() => {}, false);

  const panGesture = Gesture.Pan().onStart(() => {
    runOnJS(frameCb.setActive)(true);
  });

  return (
      <GestureDetector gesture={panGesture}>
        <View>
          <Text>Test</Text>
        </View>
      </GestureDetector>
    )

Snack or a link to a repository

https://github.com/osamaqarem/reanimated-strict-mode-bug/blob/main/App.tsx

Reanimated version

3.6.1

React Native version

0.73.1

Platforms

iOS

JavaScript runtime

Hermes

Workflow

React Native

Architecture

Paper (Old Architecture)

Build type

Debug app & dev bundle

Device

iOS simulator

Device model

No response

Acknowledgements

Yes

tjzel commented 9 months ago

Hi @osamaqarem, this is not a bug it's a feature 😄!

What I mean by that is without use strict here you'd get a silent fail here instead of an error. I'm currently investigating it. Thanks for the report1

tjzel commented 9 months ago

Actually it seems to be a pretty simple problem. The problem is with passing the whole Frame Object to a worklet.

Why

The way how worklets work is that they copy all of their closure values (except for mutables etc.) to the UI thread from JS thread.

If you look here https://github.com/software-mansion/react-native-reanimated/blob/main/src/reanimated2/shareables.ts#L262 you can see that in dev bundles we freeze the objects that we copy. This is done to avoid situations when user would do:

const a = 1;
  useFrameCallback(() => {
    console.log(a);
  }, true);

And expect the outcome to be different once he changes a on JS thread, that's what Shared Values are for. In your case you have a worklet

const panGesture = Gesture.Pan().onStart(() => {
  // This here is a worklet!
});

To which you are passing whole frameCb (we don't get only the accessed property of an object into the closure, we get the whole object). Because of that useFrameCallback cannot modify its internals anymore.

How to solve

Always pass as granular objects as possible to worklets. Best solutions in this case would be:

  const setFrameCbActive = () => frameCb.setActive(true);

  const panGesture = Gesture.Pan().onStart(() => {
    runOnJS(setFrameCbActive)();
  });

or

  const setFrameCbActive = frameCb.setActive;

  const panGesture = Gesture.Pan().onStart(() => {
    runOnJS(setFrameCbActive)(true);
  });
osamaqarem commented 9 months ago

Thanks for the thorough explanation, makes complete sense. It wasn't obvious to me that 'setActive' would be a mutation of the object itself - TIL 💡