software-mansion / react-native-reanimated

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

Variables defined after worklet functions are mistakingly undefined in worklets #5365

Open gaearon opened 12 months ago

gaearon commented 12 months ago

Description

This works:

  const [x, setX] = React.useState(0)

  function someWorkletFn() {
    'worklet'
    console.log('lol')
  }

  const handler = useAnimatedScrollHandler({
    onScroll() {
      console.log(x)
      someWorkletFn()
    },
  })

This doesn't:

  const handler = useAnimatedScrollHandler({
    onScroll() {
      console.log(x)
      someWorkletFn()
    },
  })

  const [x, setX] = React.useState(0)

  function someWorkletFn() {
    'worklet'
    console.log('lol')
  }

However, from the perspective of the code inside onScroll, this code should be equivalent — the variables have already been declared before it was called.

If the Reanimated plugin can't handle it, I think this should be a build error or something. Right now they're just undefined.

Steps to reproduce

import * as React from 'react'
import {View} from 'react-native'
import Animated, {useAnimatedScrollHandler} from 'react-native-reanimated'

export default function App() {
  const handler = useAnimatedScrollHandler({
    onScroll() {
      console.log(x)
      someWorkletFn()
    },
  })

  const [x, setX] = React.useState(0)

  function someWorkletFn() {
    'worklet'
    console.log('lol')
  }

  React.useEffect(() => {
    let id = setInterval(() => setX(x => x + 1), 1000)
    return () => clearInterval(id)
  }, [])

  return (
    <Animated.FlatList
      onScroll={handler}
      style={{flex: 1, backgroundColor: 'red'}}
      contentContainerStyle={{
        paddingTop: 1000,
      }}
    />
  )
}

Expected: it works

Actual:

LOG  undefined
ERROR  ReanimatedError: someWorkletFn is not a function (it is undefined), js engine: reanimated

This is definitely broken on Android and iOS. Not sure about web cause I can't get onScroll there to fire at all in this example.

Snack or a link to a repository

https://github.com/bluesky-social/social-app/tree/reanimated-order-bug

Reanimated version

3.4.2

React Native version

0.72.5

Platforms

Android, iOS

JavaScript runtime

Hermes

Workflow

None

Architecture

Paper (Old Architecture)

Build type

None

Device

None

Device model

No response

Acknowledgements

Yes

github-actions[bot] commented 12 months ago

Hey! 👋

The issue doesn't seem to contain a minimal reproduction.

Could you provide a snack or a link to a GitHub repository under your username that reproduces the problem?

gaearon commented 12 months ago

I did :)

Latropos commented 11 months ago

This is a known issue, probably we can't solve it. We were considering releasing a set of eslint rules to simplify working with reanimated instead cc @tjzel

gaearon commented 11 months ago

Curious, can you share more about what's tricky about it?

tomekzaw commented 11 months ago

Currently, worklets need to be transpiled by Reanimated Babel plugin which, as a side effect, converts all function declarations (function foo() { /* ... */ }) into following function factories:

var foo = function () {
  var _e = [new global.Error(), 1, -27];
  var foo = function foo(x) { /* ... */ };
  foo.__closure = {};
  foo.__initData = _worklet_9810417751380_init_data;
  foo.__workletHash = 9810417751380;
  foo.__stackDetails = _e;
  return foo;
}();

Unfortunately, this breaks hoisting. More details here: https://github.com/software-mansion/react-native-reanimated/issues/4509#issuecomment-1569781301

gaearon commented 11 months ago

Hmm, I don't think my issue is about hoisting per se. Let's forget the function declaration and just have this:

const onStuff = () => {
  'worklet'
  console.log(foo.value)
}

const foo = useSharedValue(0)

This breaks.

However, this works:

const foo = useSharedValue(0)

const onStuff = () => {
  'worklet'
  console.log(foo.value)
}

I don't quite see why this wouldn't be possible to support yet.

gaearon commented 11 months ago

If it's due to the closure, can that be lazy?

E.g. instead of

onStuff.__closure = { foo }

can it be

onStuff.__closure = () => ({ foo })

?

Then by the time it's called it'll be defined.

tjzel commented 11 months ago

This seems like an interesting idea. We'll definitely look into it!