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

feat: add react-native-macos support #513

Merged
merged 22 commits into from
Oct 31, 2024
Merged

Conversation

shirakaba
Copy link
Contributor

@shirakaba shirakaba commented Jul 23, 2024

Summary

Adds react-native-macos support and integrates react-native-test-app.

Downside: downgrades react-native from ^0.74.2 to ~0.73.9 in order to keep all React Native versions on the same minor (which reduces the chance of duplicated dependencies like Metro).

Update:

  • Adds react-native-macos support
  • Adds a new example folder, example-macos, based on react-native-test-app. This is a compromise to allow us to continue to develop Fabric on react-native v0.74 whilst still having access to react-native-macos which is limited to v0.73 for now. Mixing them in the same folder would result in clashes of tooling like the CLI and Metro, as discussed below.

Test Plan

I've ported the existing iOS sample app to macOS so we can test it manually. Not sure whether this repo has automated tests.

Status

PR now ready for review as the following tasks have been completed.

See now-complete checklist of tasks
  • Get the macOS example app to build.
    • It's complaining about /Users/jamie/Documents/git/react-native-safe-area-context/example/macos/Pods/Headers/Public/React-Core/React/RCTBridgeModule.h:172:1 Property with 'retain (or strong)' attribute must be of object type. It sounds like this issue, but I've not been able to figure out how to fix it.
    • It's also complaining about _providerView and _bridge being unavailable. Perhaps due to the same root cause.
    • Would be nice if the Pods project would stop targeting macOS 10.6. It's specified in the generated example/macos/Pods/Pods.xcodeproj/project.pbxproj, but I'm not sure how to configure that declaratively.
  • Check whether all the other platforms are working.
image image

In future

Once [email protected] is available, it would be nice to use a single example directory again and use React Native Test App to manage all the platforms.

@jacobp100
Copy link
Collaborator

I’m not sure we’re able to downgrade react native, as we only super fabric on 0.74

@shirakaba
Copy link
Contributor Author

shirakaba commented Jul 23, 2024

@Saadnajmi @tido64 would react-native-test-app work properly with react-native-macos on v0.73 yet react-native on v0.74? If I just updated the react-native dep to v0.74, would it work out-of-the-box or would further changes be needed?

@tido64
Copy link
Contributor

tido64 commented Jul 23, 2024

@Saadnajmi @tido64 would react-native-test-app work with react-native-macos on v0.73 yet react-native on v0.74? If I just updated the react-native dep to v0.74, would it work out-of-the-box or would further changes be needed?

It should. The reason we don't recommend it is because it will introduce duplicates of CLI, Metro, etc. It can be confusing to debug because we don't know which is being used.

@tido64
Copy link
Contributor

tido64 commented Jul 23, 2024

I'll also add that maybe adding a separate example-macos is an option to ensure we don't use the wrong CLI/Metro. But if this repo is all in on Fabric, I'm not sure if react-native-macos is ready yet. Maybe @Saadnajmi can chime in.

@Saadnajmi
Copy link

Saadnajmi commented Jul 23, 2024

A separate example-macos is usually what I recommend. I would personally separate adding RNTA and adding macOS support to two PRs as well.

Sidenote: Does macOS support actually do anything, as in, are we respecting like the safe area of the MacBook notch? If the blocker is that a project that depends on this iOS only project shows ups in macOS projects, then RN 0.74 has a change to not do that, where codegen will ifdef out platforms not supported in the podspec: facebook/react-native#42047

@Saadnajmi
Copy link

For macOS Fabric support, we're close. I think it's suitable enough to test. RNM 0.74 will come, soon. We're actively working on it, my plan is for it to come before 0.75 releases, but I'm hesitant to guess a better timeline than that.

@shirakaba
Copy link
Contributor Author

shirakaba commented Jul 24, 2024

@Saadnajmi @tido64 I'm not even sure I can get RNTestApp to build at this rate. The RNTestApp iOS example app runs fine, but I cannot get it to build for macOS. Have you seen this "Cannot create __weak reference because the current deployment target does not support weak references" problem before (as shown in my screenshot)?

I tried making some post-install changes to example/macos/Podfile but it's still fighting me. Got no ideas left at this point.

@shirakaba
Copy link
Contributor Author

A separate example-macos is usually what I recommend. I would personally separate adding RNTA and adding macOS support to two PRs as well.

That's the approach I took in FormidableLabs/react-native-app-auth#1002, but I found integrating into a monorepo is really hard and RNTA was quite attractive for handling that for me (and Tommy's video walkthrough was a blessing).

When you say a "separate example-macos", how are you envisaging the setup? Would we have a example-common folder to store all the common TypeScript sources into ?

@shirakaba
Copy link
Contributor Author

Sidenote: Does macOS support actually do anything, as in, are we respecting like the safe area of the MacBook notch?

I'm not entirely sure. I just mirrored the iOS code:

  • In RNCSafeAreaContext.mm, there is no such property as safeAreaInsets on NSWindow, so I return NSEdgeInsetsZero. Maybe around here we need to think about the NSScreen's safeAreaInsets? Not sure.
  • In RNCSafeAreaProvider.m, I return self.safeAreaInsets. I guess this is NSView's safeAreaInsets.
  • In RNCSafeAreaView.m, I return the description of the insets on providerView and currentSafeArea on macOS 11.0 upwards.

So, perhaps it does something except for windows.

If the blocker is that a project that depends on this iOS only project shows ups in macOS projects, then RN 0.74 has a change to not do that, where codegen will ifdef out platforms not supported in the podspec: facebook/react-native#42047

I'm not sure why I've not run into that codegen problem, but react-native-safe-area-context does actually build and run on react-native-macos; it's implemented via CompatNativeSafeAreaProvider, a fully JS solution.

My goal with this PR was to make a NativeSafeAreaProvider for macOS, even if it always passed zero insets, to replace the CompatNativeSafeAreaProvider, as it's causing errors to spew in the CLI upon window resize:

image

So it's blocking my usage of Stack in React Navigation (unless I freeze the window size).

@tido64
Copy link
Contributor

tido64 commented Jul 24, 2024

@Saadnajmi @tido64 I'm not even sure I can get RNTestApp to build at this rate. The RNTestApp iOS example app runs fine, but I cannot get it to build for macOS. Have you seen this "Cannot create __weak reference because the current deployment target does not support weak references" problem before (as shown in my screenshot)?

I tried making some post-install changes to example/macos/Podfile but it's still fighting me. Got no ideas left at this point.

Try setting the deployment targets in the .podspec:

diff --git a/react-native-safe-area-context.podspec b/react-native-safe-area-context.podspec
index 8b98104..18dccdc 100644
--- a/react-native-safe-area-context.podspec
+++ b/react-native-safe-area-context.podspec
@@ -18,6 +18,10 @@ Pod::Spec.new do |s|
   s.source_files  = "ios/**/*.{h,m,mm}"
   s.exclude_files = "ios/Fabric"

+  s.ios.deployment_target = "12.4"
+  s.osx.deployment_target = "10.15"
+  s.visionos.deployment_target = "1.0"
+
   s.dependency "React-Core"

   if fabric_enabled

We should target macOS 11.0 instead to get rid of this warning:

image

diff --git a/example/macos/Podfile b/example/macos/Podfile
index a9ec924..c16ad9e 100644
--- a/example/macos/Podfile
+++ b/example/macos/Podfile
@@ -6,24 +6,6 @@ require "#{ws_dir}/node_modules/react-native-test-app/macos/test_app.rb"

 workspace 'SafeAreaContextExample.xcworkspace'

-my_post_install_function = ->(installer) {
-  puts "Running custom post_install with installer: #{installer}"
-  installer.pods_project.targets.each do |target|
-    case target.name
-    when "react-native-safe-area-context"
-      target.build_configurations.each do |config|
-        # For some reason it gets set as 10.6 if we don't do this.
-        config.build_settings[MACOSX_DEPLOYMENT_TARGET] = '10.15'
-      end
-    when /\AReact/, /\ARCT/
-      target.build_configurations.each do |config|
-        config.build_settings['CLANG_ENABLE_OBJC_WEAK'] = 'YES'
-        config.build_settings['CLANG_ENABLE_OBJC_ARC'] = 'YES'
-        config.build_settings['CLANG_CXX_LANGUAGE_STANDARD'] = 'C++20'
-      end
-    else
-    end
-  end
-}
+platform :osx, '11.0'

-use_test_app!({ post_install: my_post_install_function })
\ No newline at end of file
+use_test_app!
diff --git a/react-native-safe-area-context.podspec b/react-native-safe-area-context.podspec
index 8b98104..3d1e5c6 100644
--- a/react-native-safe-area-context.podspec
+++ b/react-native-safe-area-context.podspec
@@ -12,12 +12,16 @@ Pod::Spec.new do |s|

   s.authors      = package['author']
   s.homepage     = package['homepage']
-  s.platforms    = { :ios => "12.4", :tvos => "12.4", :visionos => "1.0", :macos => "10.15" }
+  s.platforms    = { :ios => "12.4", :tvos => "12.4", :visionos => "1.0", :macos => "11.0" }

   s.source       = { :git => "https://github.com/th3rdwave/react-native-safe-area-context.git", :tag => "v#{s.version}" }
   s.source_files  = "ios/**/*.{h,m,mm}"
   s.exclude_files = "ios/Fabric"

+  s.ios.deployment_target = "12.4"
+  s.osx.deployment_target = "11.0"
+  s.visionos.deployment_target = "1.0"
+
   s.dependency "React-Core"

   if fabric_enabled

I got the app building, but it's blank?

Screenshot 2024-07-24 at 08 41 57

When you say a "separate example-macos", how are you envisaging the setup? Would we have a example-common folder to store all the common TypeScript sources into ?

If there are a lot of example code, having an example-common makes sense.

@shirakaba
Copy link
Contributor Author

shirakaba commented Jul 24, 2024

Woah, thanks! I'll give this a spin today and look into why it's just showing a white screen.

I've never understood why Podspec supports both platforms and deployment_target yet only the latter seems to work correctly 😓 the docs do seem to suggest we should take out the platforms section altogether in favour of deployment_target form multi-platform projects.

Due to the need to maintain a Fabric dev environment for iOS, I guess I should refactor this to remove RNTestApp and put a bespoke react-native-macos app in instead like I did in FormidableLabs/react-native-app-auth#1002 ?

But won't that approach give us two CLIs and two Metros all the same? I don't quite understand how it's any better than just continuing to use RNTestApp with [email protected] and [email protected].

@tido64
Copy link
Contributor

tido64 commented Jul 24, 2024

But won't that approach give us two CLIs and two Metros all the same? I don't quite understand how it's any better than just continuing to use RNTestApp with [email protected] and [email protected].

In this repo, example has its own yarn.lock. I would assume example-macos would also follow the same pattern. In which case, there will be no dupes under example-macos.

In app-auth's case, you can "workaround" dupes by explicitly running node_modules/react-native-macos/node_modules/@react-native-community/cli/build/bin.js(or similar), but there's no telling which Metro package will be run. It might be the correct version if you're lucky, but this is exactly why we don't recommend it 😛

@shirakaba
Copy link
Contributor Author

@tido64 That worked great, thanks!

image

I recognise this. Your screenshot is a blank window only because at your time of day, it's dark mode and AppKit has helpfully chosen to turn the text white 😄

I'm not sure how to open this "dev menu" on macOS 🤔

@tido64
Copy link
Contributor

tido64 commented Jul 24, 2024

I'm not sure how to open this "dev menu" on macOS 🤔

I think right-clicking anywhere should open it.

@shirakaba
Copy link
Contributor Author

Aha! You focus the Metro CLI and press 'D' to bring up the examples. Pretty smart!

image

That green gutter seems to indicate the scrollbar insets. Unclear whether that counts as safe area; I think not, because of the next example.

image

It's always reporting zero for the insets here, which may well be true as macOS tends to have no unsafe insets, except maybe the notch.

Anyway, its compiling and running, and it doesn't spew errors in the CLI when I resize the window (though I note it didn't either with the compat components enabled, so I guess it's some weird interaction between React Navigation and the provider that I was bumping into).

@tido64
Copy link
Contributor

tido64 commented Jul 24, 2024

I recognise this. Your screenshot is a blank window only because at your time of day, it's dark mode and AppKit has helpfully chosen to turn the text white 😄

🤦

@shirakaba
Copy link
Contributor Author

shirakaba commented Jul 24, 2024

@Saadnajmi @tido64 I've restored the original example app back to its initial state and removed RNTestApp.

I've also added example-macos as a best guess (note how its index.js imports ../example/src/App to avoid duplicating demo sources), but I can't get Metro to work. In fact, this happens whether I'm importing from ./src/App or ../example/src/App:

 LOG  Running "Example" with {"rootTag":1,"initialProps":{}}
 ERROR  Invariant Violation: "Example" has not been registered. This can happen if:
* Metro (the local dev server) is run from the wrong folder. Check if Metro is running, stop it and restart it in the current project.
* A module failed to load due to an error and `AppRegistry.registerComponent` wasn't called.

I'm running npm run start in the correct folder, example-macos, so not sure what its complaint is. Any idea how to make it happy?

@jacobp100
Copy link
Collaborator

I think the main code changes seem fine. They seem to be the standard way of doing this. I didn't see any issues there

I'm not too certain on introducing a second example just for macOS - and presumably, react-native-macos will update one day to 0.74. @janicduplessis what do you think?

@tido64
Copy link
Contributor

tido64 commented Jul 24, 2024

@Saadnajmi @tido64 I've restored the original example app back to its initial state and removed RNTestApp.

Wait, why did you have to remove RNTA? You could've kept it for example-macos?

I'm running npm run start in the correct folder, example-macos, so not sure what its complaint is. Any idea how to make it happy?

It looks like you've called your component RNSACExample. Try replacing Example with that?

@shirakaba
Copy link
Contributor Author

shirakaba commented Jul 25, 2024

Wait, why did you have to remove RNTA? You could've kept it for example-macos?

Just misunderstanding the request 😅 so we're happy to use RNTA for the macOS app, and just want it in a separate folder of its own rather than integrating it into the existing example folder?

I'll make that change later.

It looks like you've called your component RNSACExample. Try replacing Example with that?

Ah, thanks. I often run into this error and had no idea app.json had an influence.

I'm not too certain on introducing a second example just for macOS - and presumably, react-native-macos will update one day to 0.74. @janicduplessis what do you think?

From the above discussions, it sounds like our options are:

  1. A single example folder carrying both [email protected] and [email protected]. This is ideal as long as we don't need to develop against 0.74.
  2. An example folder carrying [email protected] and an example-macos folder carrying [email protected]. This allows us to develop against 0.74 whilst avoiding mixing CLI and Metro versions. The only downside is having twice as much boilerplate.
  3. A single example folder carrying [email protected] and [email protected]. This is not recommended as we'd end up mixing CLI and Metro versions and it can lead to making debugging issues harder.

Please advise on how I should proceed.

@tido64
Copy link
Contributor

tido64 commented Jul 25, 2024

Just misunderstanding the request 😅 so we're happy to use RNTA for the macOS app, and just want it in a separate folder of its own rather than integrating it into the existing example folder?

I'll make that change later.

I'd be happy with example migrating to RNTA as well, but we should save that for a separate PR.

@shirakaba
Copy link
Contributor Author

@tido64 so is your position:

  • keep example unchanged in this PR (keep it as a bespoke example app)
  • use RNTA for example-macos for this PR
  • in a future PR, introduce RNTA to example as well?

@tido64
Copy link
Contributor

tido64 commented Jul 25, 2024

@tido64 so is your position:

  • keep example unchanged in this PR (keep it as a bespoke example app)
  • use RNTA for example-macos for this PR
  • in a future PR, introduce RNTA to example as well?

Yes, as long as the maintainers are happy with it.

@shirakaba shirakaba marked this pull request as ready for review July 27, 2024 00:40
@shirakaba shirakaba changed the title feat: add react-native-macos support and integrate react-native-test-app feat: add react-native-macos support Jul 27, 2024
@jacobp100
Copy link
Collaborator

Where are we standing with this today? What version is react-native-macos on now?

@Saadnajmi
Copy link

@jacobp100 We just updated to version 0.75

@jacobp100
Copy link
Collaborator

Is it now possible to remove the extra example folder here?

I temporarily renamed the "name" field in package.json to "RNSACExample", then ran `npx react-native-macos-init` to generate a project by that name.
@shirakaba
Copy link
Contributor Author

shirakaba commented Oct 30, 2024

Now that [email protected] is released, I've simplified this PR to drop RNTA and the separate example-macos folder, in the hopes that we can get it merged with less friction.

I note that #538 is also in progress, based on RNTA and [email protected], so merging that would stall this PR again. [email protected] is expected to be out soon.

If a maintainer (@janicduplessis?) could just specify what would make them happy to merge this, I'd like to get this across the line!

@janicduplessis
Copy link
Member

@shirakaba Thanks for pushing this forward. I think it looks good overall.

One thing I would like, but can be done in a follow up is add a macos build to the github action CI.

Also do macos supports the new arch? Currently it seems like these changes only apply to old arch.

@janicduplessis
Copy link
Member

Can you also run yarn format:write

const escape = require('escape-string-regexp');
const exclusionList = require('metro-config/src/defaults/exclusionList');
const pak = require('../package.json');
const path = require('node:path');
Copy link
Member

Choose a reason for hiding this comment

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

Are these changes needed or leftover from example changes?

Copy link
Contributor Author

@shirakaba shirakaba Oct 31, 2024

Choose a reason for hiding this comment

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

The default metro.config.js that comes with React Native iOS/Android apps, and the modified one currently on main, does not support out-of-tree platforms. A few changes are needed to do things like alias react-native to react-native-macos on macOS. Additional changes would be needed if supporting react-native-windows in future.

One could express all that without @rnx-kit/metro-config, but it's not pretty. Here's the metro.config.js that you'd get if you set up a brand new React Native v0.75 project with both macOS and Windows support. It's rather verbose even before adding the support needed by this library-plus-example repo (referencing the where to find react-native-safe-area-context):

const {getDefaultConfig, mergeConfig} = require('@react-native/metro-config');

const fs = require('fs');
const path = require('path');
const exclusionList = require('metro-config/src/defaults/exclusionList');

const rnwPath = fs.realpathSync(
  path.resolve(require.resolve('react-native-windows/package.json'), '..'),
);

//

/**
 * Metro configuration
 * https://facebook.github.io/metro/docs/configuration
 *
 * @type {import('metro-config').MetroConfig}
 */

const config = {
  //
  resolver: {
    blockList: exclusionList([
      // This stops "react-native run-windows" from causing the metro server to crash if its already running
      new RegExp(
        `${path.resolve(__dirname, 'windows').replace(/[/\\]/g, '/')}.*`,
      ),
      // This prevents "react-native run-windows" from hitting: EBUSY: resource busy or locked, open msbuild.ProjectImports.zip or other files produced by msbuild
      new RegExp(`${rnwPath}/build/.*`),
      new RegExp(`${rnwPath}/target/.*`),
      /.*\.ProjectImports\.zip/,
    ]),
    //
  },
  transformer: {
    getTransformOptions: async () => ({
      transform: {
        experimentalImportSupport: false,
        inlineRequires: true,
      },
    }),
  },
};

module.exports = mergeConfig(getDefaultConfig(__dirname), config);

@rnx-kit/metro-config does all that under the hood, as well giving some other goodies like better monorepo support and supporting extending Expo's metro config.

@shirakaba
Copy link
Contributor Author

Can you also run yarn format:write

Done!

One thing I would like, but can be done in a follow up is add a macos build to the github action CI.

Got it 👌

Also do macos supports the new arch? Currently it seems like these changes only apply to old arch.

The last update on October 24th was that they aren't quite ready to turn it on by default, but they're working on it.

As this PR is just a bunch of ifdefs around the iOS implementation, I've not thought too deeply about the New Architecture. Could you point me at the bits that iOS is doing specifically to support new arch?

Either way, I'd be happy to come back for New Arch as a follow-up if that'd be alright!

@janicduplessis
Copy link
Member

https://github.com/th3rdwave/react-native-safe-area-context/tree/main/ios/Fabric, we can do that in a follow up so can test properly.

@janicduplessis janicduplessis merged commit ed16a5e into th3rdwave:main Oct 31, 2024
3 checks passed
@shirakaba shirakaba deleted the macos branch October 31, 2024 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants