realm-js icon indicating copy to clipboard operation
realm-js copied to clipboard

Support for react-native-macos

Open asabhaney opened this issue 2 years ago • 11 comments

What, How & Why?

This closes #2907, making some minor changes to be able to support this library on native MacOS apps if using Microsoft's react-native-macos library. This PR does not make any substantial changes to the build process (and no changes to realm-core), with most changes isolated to the build-ios.sh script. This PR incorporates some of the code & ideas from one of the comments in the original issue thread.

I have tested these changes in a simple app that does some basic reads and writes, which I was able to successfully run on both macOS and iOS.

Despite it working, there are a couple known issues:

  • In the build-ios.sh script (see here), I was unable to have the archive output folder named Release-macosx as intended, seemingly because $EFFECTIVE_PLATFORM_NAME was not being set when cmake was being run. I thought adding set_property(GLOBAL PROPERTY XCODE_EMIT_EFFECTIVE_PLATFORM_NAME ON) to xcode.toolchain.cmake in realm-core might address this, but I still couldn't get it to work. So unfortunately, the output directory name is simply Release, though perhaps it is irrelevant since it seems to just be an intermediary build artifact. On the bright side, realm-core remains untouched.

  • In the Podfile, I could not use the vendored_frameworks for OS X, and had to resort to using vendored_libraries and public_header_files - along with some conditional preprocessor directives in RealmReact.mm - to get it to work on macOS.

There's definitely a fair amount about the Realm build process I'm unaware of (and cmake in general, to be honest), but happy to make any improvements (with some guidance) if needed.

☑️ ToDos

  • [x] 📝 Changelog entry
  • [x] 📝 Compatibility label is updated or copied from previous entry
  • [x] 🚦 Tests
  • [x] 📱 Check the React Native/other sample apps work if necessary
  • [x] 📝 Public documentation PR created or is not necessary
  • [x] 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

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

asabhaney avatar May 06 '22 01:05 asabhaney

Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundation’s CLA. Our records show that CLA has not been signed by @asabhaney. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a @cla-bot check comment. The GitHub usernames you file there will need to match that of your Pull Requests. If you have any questions or cannot file the CLA electronically, make a comment here and we will be happy to help you out.

cla-bot[bot] avatar May 06 '22 01:05 cla-bot[bot]

Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundation’s CLA. Our records show that CLA has not been signed by @asabhaney. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a @cla-bot check comment. The GitHub usernames you file there will need to match that of your Pull Requests. If you have any questions or cannot file the CLA electronically, make a comment here and we will be happy to help you out.

cla-bot[bot] avatar May 06 '22 01:05 cla-bot[bot]

@cla-bot check

asabhaney avatar May 06 '22 01:05 asabhaney

The cla-bot has been summoned, and re-checked this pull request!

cla-bot[bot] avatar May 06 '22 01:05 cla-bot[bot]

@asabhaney Thanks so much for your contribution. The team is going to take a look and discuss how we would like to proceed with this. We will post here when we have any updates.

takameyer avatar May 09 '22 14:05 takameyer

Thanks @takameyer - looking forward to hearing the team's thoughts!

asabhaney avatar May 10 '22 13:05 asabhaney

@asabhaney We have been discussing internally how we want to generally handle external PRs. I'll first quote an upcoming update to our contribution guide:

[...] indicate how you would like us to support you. We will happily guide you and work with you to move the PR to a point where it can be merged. It might require considerable work at your end to meet our expectations (code quality, API docs, TS defs, tests, etc.). In the case you want to move on and not work with us, please let us know. If the PR meets our expectations, we will merge it - and if it doesn't, we will either take over or close it, depending on the requirement on our time and our current priorities.

So, let us know your intentions with the PR 🙂. We have this on our backlog, but it will probably take a while before this gets prioritised on our end. If you would like to work on it, it may take some time with the back and forth and a bit of patience, but will in the end be more likely to be accepted into our code base.

takameyer avatar May 16 '22 18:05 takameyer

@takameyer Thanks for the note and for pointing me to the updated contribution guide. I would like to work with your team to move this PR to a point where it can be merged in. I eagerly await your feedback :)

asabhaney avatar May 16 '22 18:05 asabhaney

Added @kraenhansen and @kneth to this PR. How should we proceed? One of the larger aspects of this work is making sure we can run tests on this platform.

takameyer avatar Jun 15 '22 07:06 takameyer

First I will say that I am impressed by the work done by @asabhaney. You have worked in areas of our code base which are not the trivial parts.

But I am a bit reluctant to merge it. We will - sooner or later - merge the v11 branch and the React Native for MacOS support will not work. We need to find a long-term solution - and I would prefer it is also includes React Native for Windows.

kneth avatar Jun 15 '22 11:06 kneth

Thanks for the feedback @takameyer @knet. Fair enough. If there's anything I can do to help when the v11 branch is merged in, please feel free to let me know. Adding support for RN for Windows would be terrific, but is a little more tricky than it is for macOS since a new Native Module for Windows would need to be written in C++ or C#.

In case anyone else comes across this thread: In the meantime, for our own project we are using the Realm Swift SDK, and we've created a RN Native Module written in Swift to bridge our RN code. It works quite well, though there are certain things that are a little more complicated to bridge (like lazy evaluation of results) but that's not really a big deal for us.

asabhaney avatar Jun 15 '22 21:06 asabhaney

@asabhaney We have released v11.0.0 which is using JSI to integration with JSC and Hermes (we are still ising N-API for node.js). I imagine that this PR will require a major rewrite in order to work - or even compile.

kneth avatar Oct 20 '22 11:10 kneth