Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS: Support 0.64 and Hermes #1469

Merged
merged 10 commits into from
Jan 27, 2021
Merged

Conversation

mrousavy
Copy link
Contributor

@mrousavy mrousavy commented Nov 23, 2020

Description

This adds support for react native 0.64 and Hermes on iOS.

I've tested this with 0.64.0-rc.1 which is available on npm.

Changes

  • Import RCTEventDispatcher since that's no longer imported in the module per default
  • Update some preprocessor #ifs to match the new RNVERSION
  • If hermes is available, create a HermesExecutorFactory instead of the JSCExecutorFactory
  • If hermes is available, create a hermes runtime instead of the JSC runtime
  • Use c++14 instead of gnu++14 as CLANG_CXX_LANGUAGE_STANDARD since hermes also uses c++14 (without this you'll see warnings on pod install)

Related Issues

fixes #1485
fixes #1372

Checklist

  • Ensured that CI passes
  • Ensured that it works on RN 0.64 (JSC)
  • Ensured that it works on RN 0.64 (Hermes)
  • Ensured that it works on RN 0.63 (JSC)
  • Ensured that it works on RN 0.62 (JSC)

"it works" = Xcode project successfully builds, app successfully starts and animations run as expected.

@mrousavy

This comment has been minimized.

@mrousavy
Copy link
Contributor Author

On Android I'm having build issues because it looks like the path to the .aar file is interpolated with the RNVERSION, which won't work since there is no react-native-reanimated-64.aar. I'll pass that to someone else, since I'm not that experienced in the Android ecosystem

Screenshot 2020-11-23 at 18 21 26

@mrousavy mrousavy changed the title Fix react-native 0.64 compatibility iOS: Fix react-native 0.64 compatibility Nov 23, 2020
@mrousavy mrousavy mentioned this pull request Nov 23, 2020
@jakub-gonet
Copy link
Member

Also, I'm getting the following warnings after running pod install with hermes enabled on iOS

Hermes uses c++14 standard
We use gnu++14

You may try changing our Podspec to also use c++14, I don't know if we use any GNU extensions.

@finkef
Copy link

finkef commented Nov 24, 2020

Can confirm that iOS builds using this branch directly as dependency with a brand new RN project and Hermes enabled.

To get it running however, I had to apply this fix.

@mrousavy
Copy link
Contributor Author

mrousavy commented Nov 24, 2020

In my case I had issues with the warnings about gnu++14 I mentioned - not sure why, as I've seen other libraries use gnu++14 without problems. After replacing it with c++14 in the Podspec, I have no troubles building & running animations on iOS, even without @Szymon20000's PR. @jakub-gonet could you review when you have some time?

As for Android; this probably has to be done in a separate PR, since I'm not that experienced with the Android side.

@jakub-gonet
Copy link
Member

I think @karol-bisztyga, @Szymon20000, @zrebcu411, or @piaskowyk are better reviewers for this one, this PR changes mostly code from Rea2.

@mrousavy
Copy link
Contributor Author

So JFYI: On Android, I had to manually point the build.gradle file to the react-native-reanimated-63.aar, so we obviously need to build a -64.aar for RN 0.64. Then, I tried to get my app running, but it keeps crashing with the following exception:

2020-11-25 13:42:23.104 17248-17395/com.mrousavy.springsale E/AndroidRuntime: FATAL EXCEPTION: create_react_context
    Process: com.mrousavy.XXX, PID: 17248
    java.lang.IncompatibleClassChangeError: Found interface com.facebook.react.uimanager.events.EventDispatcher, but class was expected (declaration of 'com.facebook.react.uimanager.events.EventDispatcher' appears in /data/app/com.mrousavy.XXX-Xf0GGcPgwhI-36pFrrw8AQ==/base.apk)
        at com.swmansion.reanimated.NodesManager.<init>(NodesManager.java:162)
        at com.swmansion.reanimated.ReanimatedModule.getNodesManager(ReanimatedModule.java:100)
        at com.swmansion.reanimated.ReanimatedJSIModulePackage.getJSIModules(ReanimatedJSIModulePackage.java:17)
        at com.facebook.react.ReactInstanceManager.createReactContext(ReactInstanceManager.java:1265)
        at com.facebook.react.ReactInstanceManager.access$1100(ReactInstanceManager.java:131)
        at com.facebook.react.ReactInstanceManager$5.run(ReactInstanceManager.java:1023)
        at java.lang.Thread.run(Thread.java:919)

@finkef
Copy link

finkef commented Nov 25, 2020

I've noticed that after adding Reanimated (from this branch) and running pod install, Hermes is actually not being used anymore. It seems that it's not available anymore here. I have no clue what might be causing this, but wanted to point it out.

@mrousavy Can you confirm that your app is actually still running on Hermes (by checking typeof HermesInternal for example)?

@mrousavy

This comment has been minimized.

@mrousavy mrousavy marked this pull request as draft November 25, 2020 17:11
@mrousavy

This comment has been minimized.

@finkef
Copy link

finkef commented Nov 25, 2020

@mrousavy Can confirm.

@mrousavy mrousavy changed the title iOS: Fix react-native 0.64 compatibility iOS: Support 0.64 and Hermes Nov 25, 2020
@mrousavy
Copy link
Contributor Author

mrousavy commented Nov 25, 2020

Fixed it. It was still creating the JSCExecutorFactory instead of the Hermes one.

@finkef check out my latest commits, see if that works for you.

This means this PR is ready for review, as I am successfully running on HermesInternal/react native 0.64 with reanimated!! 🎉🎉

@mrousavy mrousavy marked this pull request as ready for review November 25, 2020 21:11
Previously that was gnu++14, but since other libraries (such as Hermes) use c++14 and that gives warnings on pod install, I'm going to use c++14 here as well.
@finkef
Copy link

finkef commented Nov 25, 2020

It's using Hermes now 👍 However, I'm greeted with the error below immediately when importing Reanimated. Trying to narrow down the issue, I believe that this line might not properly set the global value.

Simulator Screen Shot - iPhone 12 - 2020-11-25 at 22 03 45

@mrousavy

This comment has been minimized.

@finkef
Copy link

finkef commented Nov 26, 2020

jsi::ThreadSafeRuntime doesn't inherit jsi::Runtime, so it won't let me use it or cast the pointers (at least with my very limited knowledge of C++ 😬 ).

@mrousavy

This comment has been minimized.

@finkef
Copy link

finkef commented Nov 26, 2020

Here is the bare minimum setup with RN 0.64 RC1 and Reanimated using the fixes from this branch.

@mrousavy I'm curious if you get the same error running this project 🤔

EDIT: Missed the link in the previous comment, it seems that it's using the class from here. (Again, very limited with C++).

EDIT2: Added the Runtime inheritance manually to the header file and built with makeThreadSafeHermesRuntime, but same result: ReferenceError: Property '_globalSetter' doesn't exist, js engine: hermes. The RN LogBox fails to mount continuously now (only happened occasionally before), so have to use Flipper to get the errors with the Hermes Debugger.

@ltcaosj
Copy link

ltcaosj commented Nov 27, 2020

I got the same issue when trying Reanimated 2.0 rc0 with RN 0.64 RC1.

@mrousavy

This comment has been minimized.

@finkef
Copy link

finkef commented Nov 27, 2020

Oh god... Was missing the babel plugin in the repro that I threw together (too quickly apparently). 🤦‍♂️ Reanimated is running with Hermes now, had to apply #1471 with alpha.9.1 again, but that PR landed with rc.0.
@mrousavy Could you rebase this PR on rc.0?

@mrousavy
Copy link
Contributor Author

mrousavy commented Nov 27, 2020

haha no worries 👍

Updated the branch - This means everything is working on RN 0.64 with Hermes enabled, and it still works for RN 0.63. I think Mike from callstack mentioned that the official RN 0.64 release is coming out on ~Monday, so it makes sense to prepare reanimated for it so it works on day one - @jakub-gonet can you assign someone to this PR/request a review?

(Also someone needs to look into android, I think in some places there were some changes made, e.g. UIManager's event dispatcher is now an interface, before it was a class)

@jakub-gonet jakub-gonet requested review from Szymon20000, karol-bisztyga and piaskowyk and removed request for Szymon20000 November 27, 2020 15:28
@jakub-gonet
Copy link
Member

Sure, done & done.

@finkef
Copy link

finkef commented Nov 27, 2020

@mrousavy Great work! Can confirm that it's working out of the box now 🎉

@rpavlovs
Copy link

Installed from this PR and still getting the _globalSetter error mentioned above.

Latest master changes seem related: #1482

@mrousavy could you rebase on master again please?

@rpavlovs
Copy link

Ah, my bad - I was just missing the babel plugin. It works! 🙂

@osdnk
Copy link
Contributor

osdnk commented Dec 17, 2020

https://github.com/osdnk/react-native-reanimated/tree/64
I made it workable for android, but I'm too uncertain to discuss some things i did

@pontusab
Copy link

Can confirm this works on iOS RN 0.64 RC2

@jinshin1013
Copy link
Contributor

Any update on the PR guys?

@a-eid
Copy link

a-eid commented Jan 12, 2021

this works for me as well, 0.64-rc2.

@osdnk could u please make a PR, I'm trying to test your repo, but I'm not able to, not really sure which files to update.

Copy link
Member

@piaskowyk piaskowyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I tested this and it works on RN62, RN63, RN64-rc.2
Last time we talked about the problem in the Example app. I resolved this, but we stay with RN63 in the Example app at this moment, so that's why I ask you to revert changes in Example/ios/Podfile.lock If you change this then we can merge this 🎉
Good job! 👏

Example/ios/Podfile.lock Outdated Show resolved Hide resolved
@mrousavy
Copy link
Contributor Author

@piaskowyk Reverted the changes in the Podfile.lock ✅

@piaskowyk piaskowyk merged commit 8c33bec into software-mansion:master Jan 27, 2021
@mrousavy mrousavy deleted the fix/rn64 branch January 27, 2021 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants