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

Support Hermes #3792

Closed
wants to merge 51 commits into from
Closed

Support Hermes #3792

wants to merge 51 commits into from

Conversation

RedBeard0531
Copy link
Contributor

@RedBeard0531 RedBeard0531 commented Jun 7, 2021

What, How & Why?

🚧 WARNING 🚧

This is still a WIP, and there are still several TODOs and XXXs that will need to be addressed. Many parts of the implementation and integration are a bit hacky at the moment in order to get to a testable implementation that we can iterate on as quickly as possible.

Closes #2455

☑️ ToDos

  • port existing abstraction layer over to JSI/hermes
  • iOS support
  • Android support
  • Benchmark load time to prioritize lazy TurboModules vs eager NativeModule implementation
  • Revisit all TODOs and XXXs
  • Figure out how to support existing JSC implementation (and remote debugger) and Hermes side-by. (Or decide that it isn't practical and we will need a hard cut-over)
  • Figure out how we want to depend on hermes and where to get jsi.h from (this affects RN version compatability, since JSI doesn't seem to have a stable ABI)
  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 🚦 Tests
  • 📝 Public documentation PR created or is not necessary
  • 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

  • typescript definitions file is updated
  • jsdoc files updated
  • Chrome debug API is updated if API is available on React Native

Comment on lines 961 to 948
/// Moves a Symbol, String, or Object rvalue into a new JS value.
#if 0
template <typename T>
#else // XXX Realm modification
template <typename T, typename = std::enable_if_t<
!std::is_reference_v<T> &&
(std::is_base_of<Symbol, T>::value ||
std::is_base_of<String, T>::value ||
std::is_base_of<Object, T>::value)
>>
#endif
/* implicit */ Value(T&& other) : Value(kindOf(other)) {
static_assert(
std::is_base_of<Symbol, T>::value ||
std::is_base_of<String, T>::value ||
std::is_base_of<Object, T>::value,
"Value cannot be implicitly move-constructed from this type");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I've modified this to move the error out of the function to overload resolution. This results in much better error messages when I screw up. Before considering this "done" I'll want to either replace this file with a clean copy from upstream, or ideal set up the build so that we just use theirs without our own copy. But for now, it is nice having this modification in place.

Also, I plan to open a PR to do this upstream, so maybe we can have the best of both worlds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Base automatically changed from ms/hermes-prep to master June 8, 2021 08:41
@RedBeard0531 RedBeard0531 marked this pull request as draft June 10, 2021 08:23
@RedBeard0531 RedBeard0531 changed the title initial port of abstraction layer to hermes Support Hermes Jun 10, 2021
@ianpward ianpward mentioned this pull request Jun 11, 2021
@Frans-L Frans-L mentioned this pull request Jun 23, 2021
@HSReact
Copy link

HSReact commented Jun 25, 2021

How’s it going guys? Can’t tell you how excited the community is for this feature…may the force be with you all 🙏🙏

@sintylapse
Copy link

Sorry for off topic, but the main thread is locked, and I think it is important for people to have backup plan, if they decide to migrate out of Realm to another DB and save their users data #3819 .

@ghost
Copy link

ghost commented Jul 2, 2021

We are also following this thread closely. Any news?

@RedBeard0531
Copy link
Contributor Author

We have it working on iOS, but have been running into issues on android. There is some indication that our android issues may be fixed by facebook/react-native@24f9f75 (since the old version of libfbjni seems to break parts of the C++ stdlib), but it is only in the 0.65 RCs and not in a released version yet.

@prscX
Copy link

prscX commented Jul 9, 2021

Thank you @RedBeard0531 for sharing the update. I'm curious to know, is it possible to have two separate releases, one for iOS and another for Android? Considering we have achieved support for iOS, maybe we can push a release for iOS now and later for Android. It will allow us to migrate our iOS apps to Hermes.

Thanks
</ Pranav >

@HCSharma
Copy link

Thank you @RedBeard0531 for sharing the update. I'm curious to know, is it possible to have two separate releases, one for iOS and another for Android? Considering we have achieved support for iOS, maybe we can push a release for iOS now and later for Android. It will allow us to migrate our iOS apps to Hermes.

Thanks
</ Pranav >

That is actually a great idea

@HCSharma
Copy link

@RedBeard0531 Its obvious but would like you to clarify, the version of realm that you are adding hermes support to, is the mongodb-realm version? Many folks have migrated to mongodb-realm after we were told Oct 2021 is end of support for traditional realm.

@agape-apps
Copy link

... have been running into issues on android. There is some indication that our android issues may be fixed by facebook/react-native@24f9f75 (since the old version of libfbjni seems to break parts of the C++ stdlib), but it is only in the 0.65 RCs and not in a released version yet.

The availability of 0.65.0 RC3 means that 0.65.0 is getting closer to be released. Is Hermes support for Android already available for testing with RN 0.65.0 RC3?

@leecommamichael
Copy link

It's kind of hard to believe that every React-Native app which uses Realm cannot currently produce a nice performance profile using Hermes. Profiling React-Native without Hermes is a mess.

@prscX
Copy link

prscX commented Aug 12, 2021

Thank you @RedBeard0531 for sharing the update. I'm curious to know, is it possible to have two separate releases, one for iOS and another for Android? Considering we have achieved support for iOS, maybe we can push a release for iOS now and later for Android. It will allow us to migrate our iOS apps to Hermes.

Thanks
</ Pranav >

Would be very helpful if you can plan a separate Hermes support release for iOS and Android. It will allow us to port our iOS apps first. Please let us know your thoughts on the same.

Thanks,
</ Pranav >

@marcosrdz
Copy link

https://reactnative.dev/blog/2021/08/17/version-065

… support Hermes (#4079)

Rework the namespace names in our Hermes/JSI layer to more accurately reflect that we support JSI, and to remove ambiguities and namespace clashes.

This closes RJS-1337.
@kraenhansen
Copy link
Member

Closing this PR (and its branch) as we've now released two alpha releases and a third is expected any day.

We will be releasing from this hermes branch going forward.

@kraenhansen kraenhansen deleted the ms/hermes branch November 24, 2021 15:54
@kraenhansen
Copy link
Member

We've released a new version of Realm JS with support for the Hermes engine: Realm JS v10.20.0-alpha.2.
This fix the crash when reloading the app on Android.

Please take it for a spin and provide any feedback or issues you might have using the Hermes issue template:

npm install realm@hermes

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Hermes engine
10 participants