software-mansion / react-native-reanimated

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

Fix "Not implemented" exception on JSC #6028

Closed tjzel closed 2 weeks ago

tjzel commented 1 month ago

Summary

In some implementations of JSC, at least react-native-macos here native state methods are not implemented and they are throwing errors. This PR adds logic that will go around this issue, by probing the hasNativeState method to determine whether it should use that.

Fixes #5974

Test plan

tjzel commented 1 month ago

@tomekzaw It's for performance. Why would we surround it with the try-catch on every call, since we will know everything after the first call.

tomekzaw commented 1 month ago

@tjzel I wouldn't worry much about performance in this case, C++ try/catch blocks are quite fast.

SInce we know that some versions react-native-macos don't support jsi::NativeState when using JSC, why can't we just use #ifdefs instead?

Also, the names nativeStateAccess, AccessProbe, Safe and Unsafe are a bit overwhelming, how about simply supportsNativeState with Yes and No instead?

Finally, I don't like the fact that some unrelated files have been formatted, can we move them to a separate PR?

mrousavy commented 1 month ago

Sorry about that, I added jsi::NativeState here.. I didn't test on MacOS / JSC.

tomekzaw commented 1 month ago

@mrousavy No worries

tjzel commented 1 month ago

@tomekzaw I read an interesting article about try/catch blocks performance from which it seems that try blocks are fast but the catch ones are expensive. Since these calls would constantly throw when nativeState is not implemented, I'd rather keep the current approach.

Regarding #ifdef I don't know how would we do that. I don't think there's any flag that would tell us if nativeState is implemented on a given engine.

I'll revert formatting and make the names more approachable.

tjzel commented 2 weeks ago

Well I guess we can do it that way for now, if the problem persists we will resort to runtime checks.

tjzel commented 2 weeks ago

Superseded by #6137