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] Enabling RCTWebSocket on UIKitForMac (macOS Catalyst) #27469

Closed

Conversation

andymatuschak
Copy link
Contributor

Summary

In #25427, @radex added initial support for running React Native projects on macOS via Catalyst. However, RCTWebSocket was disabled for that target because of some compilation issues. This meant that running projects via a connection to the packager wasn't possible: no live reload, and projects must be run in "Release" mode. It also meant making manual changes to Xcode projects deploying to macOS and scattering a number of conditional checks throughout the codebase.

In this change, I've implemented support for RCTWebSocket on the macOS target and re-enabled the affected features. Live reload and the inspector now work for macOS targets. Manual modifications of Xcode build settings are no longer necessary for react-native projects running on macOS.

Screen Shot 2019-12-10 at 8 36 38 AM

Limitations

There's no binding which displays the developer menu (since there's no shake event on macOS). We'll probably want to add one, perhaps to the menu bar.

I've chosen not to commit the modifications to RNTester which enable macOS support, since that would imply more "official" support for this target than I suspect you all would like to convey. I'm happy to add those chunks if it would be helpful.

Changelog

[iOS] [Added] - Added web socket support for macOS (Catalyst), enabling debug builds and live reload

Test Plan

  • Open RNTester/RNTester.xcodeproj with Xcode 11.2.1, run it like a normal iOS app -- make sure it compiles and runs correctly (no regression)
  • Select "My Mac" as device target, and run. You may need to configure a valid development team to make signing work.
  • RNTester should run fine with no additional configuration. Modify a file in RNTester, note that live reload is now working.
  • Test the developer inspector. To display the developer menu, you'll need to manually show it; here's an example diff which does that:
diff --git a/RNTester/js/RNTesterApp.ios.js b/RNTester/js/RNTesterApp.ios.js
index 8245a68d12..a447ad3b1b 100644
--- a/RNTester/js/RNTesterApp.ios.js
+++ b/RNTester/js/RNTesterApp.ios.js
@@ -19,6 +19,8 @@ const React = require('react');
 const SnapshotViewIOS = require('./examples/Snapshot/SnapshotViewIOS.ios');
 const URIActionMap = require('./utils/URIActionMap');
 
+import NativeDevMenu from '../../Libraries/NativeModules/specs/NativeDevMenu';
+
 const {
   AppRegistry,
   AsyncStorage,
@@ -143,6 +145,7 @@ class RNTesterApp extends React.Component<Props, RNTesterNavigationState> {
 
   UNSAFE_componentWillMount() {
     BackHandler.addEventListener('hardwareBackPress', this._handleBack);
+    NativeDevMenu.show();
   }
 
   componentDidMount() {

@facebook-github-bot
Copy link
Contributor

Hi andymatuschak! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@react-native-bot react-native-bot added Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Dec 10, 2019
@andymatuschak
Copy link
Contributor Author

ci/circleci: js_coverage failed with this error:

Test Suites: 127 passed, 127 total
Tests:       1 skipped, 1192 passed, 1193 total
Snapshots:   555 passed, 555 total
Time:        106.245s
Ran all test suites.
Done in 107.11s.

/home/circleci/react-native/node_modules/coveralls/bin/coveralls.js:18
        throw err;
        ^
Bad response: 422 {"message":"Couldn't find a repository matching this job.","error":true}

Exited with code exit status 1

That's mysterious to me and strikes me as possibly spurious. If there's something actually broken, a pointer would be helpful!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 10, 2019
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@@ -749,7 +746,7 @@ - (void)handleCloseWithData:(NSData *)data
return;
} else if (dataSize >= 2) {
[data getBytes:&closeCode length:sizeof(closeCode)];
_closeCode = EndianU16_BtoN(closeCode);
_closeCode = NSSwapBigShortToHost(closeCode);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's so simple! I wish I spent 10 more minutes on the problem ;)

@radex
Copy link
Contributor

radex commented Dec 10, 2019

Thank you @andymatuschak for this!

Copy link
Contributor

@charpeni charpeni left a comment

Choose a reason for hiding this comment

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

Thanks for sending that!

I was currently looking at this. I would have loved to see your PR before digging into it. 😅

<Endian.h> is not available on Mac (deprecated in macOS 10.8), and <Machine/Endian.h> doesn't expose the methods we want.

I ended up using CFSwapInt16BigToHost & CFSwapInt64BigToHost in my branch. But it looks like they achieved the same thing!

At some point, we should probably update our SRWebSocket based on the latest version of SocketRocket. 🤷‍♂ Out of scope anyway.

@TheSavior
Copy link
Member

TheSavior commented Dec 11, 2019

This is awesome! I'll give @shergin a chance to review before landing

Does CMD+D on the app bring up the dev menu? (@rickhanlonii reminded me this is the shortcut)

@fungilation
Copy link

Awesome work indeed!

@charpeni
Copy link
Contributor

@TheSavior CMD+D doesn't bring up the dev menu. I know what's up, sending a PR in a few minutes.

@hramos
Copy link
Contributor

hramos commented Dec 12, 2019

I'm going to import these in order to get a head start with our internal test suite, as well as to get it onto @shergin's review queue.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andymatuschak in f21fa4e.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Dec 18, 2019
facebook-github-bot pushed a commit that referenced this pull request Jan 27, 2020
Summary:
This enables the dev menu to bo opened from keyboard shortcuts in dev from a Mac Catalyst app.

cc TheSavior andymatuschak radex

## Changelog

[iOS] [Fixed] - Enable dev keyboard shortcuts on Mac Catalyst
Pull Request resolved: #27479

Test Plan:
It depends on #27469 (to have working WebSocket in debug).

![image](https://user-images.githubusercontent.com/7189823/70629346-d3a68880-1bf7-11ea-8949-7553157a2f9c.png)

Differential Revision: D19576528

Pulled By: shergin

fbshipit-source-id: 32b4f8424fb7d270640af4bc50dba24f488bef4f
alloy pushed a commit that referenced this pull request Feb 13, 2020
Summary:
In #25427, radex added initial support for running React Native projects on macOS via Catalyst. However, `RCTWebSocket` was disabled for that target because of some compilation issues. This meant that running projects via a connection to the packager wasn't possible: no live reload, and projects must be run in "Release" mode. It also meant making manual changes to Xcode projects deploying to macOS and scattering a number of conditional checks throughout the codebase.

In this change, I've implemented support for `RCTWebSocket` on the macOS target and re-enabled the affected features. Live reload and the inspector now work for macOS targets. Manual modifications of Xcode build settings are no longer necessary for react-native projects running on macOS.

![Screen Shot 2019-12-10 at 8 36 38 AM](https://user-images.githubusercontent.com/2771/70549905-ce7b0800-1b29-11ea-85c6-07bf09811ae2.png)

### Limitations

There's no binding which displays the developer menu (since there's no shake event on macOS). We'll probably want to add one, perhaps to the menu bar.

I've chosen not to commit the modifications to RNTester which enable macOS support, since that would imply more "official" support for this target than I suspect you all would like to convey. I'm happy to add those chunks if it would be helpful.

## Changelog

[iOS] [Added] - Added web socket support for macOS (Catalyst), enabling debug builds and live reload
Pull Request resolved: #27469

Test Plan:
* Open RNTester/RNTester.xcodeproj with Xcode 11.2.1, run it like a normal iOS app -- make sure it compiles and runs correctly (no regression)
* Select "My Mac" as device target, and run. You may need to configure a valid development team to make signing work.
* RNTester should run fine with no additional configuration. Modify a file in RNTester, note that live reload is now working.
* Test the developer inspector. To display the developer menu, you'll need to manually show it; here's an example diff which does that:
```
 diff --git a/RNTester/js/RNTesterApp.ios.js b/RNTester/js/RNTesterApp.ios.js
index 8245a68d12..a447ad3b1b 100644
 --- a/RNTester/js/RNTesterApp.ios.js
+++ b/RNTester/js/RNTesterApp.ios.js
@@ -19,6 +19,8 @@ const React = require('react');
 const SnapshotViewIOS = require('./examples/Snapshot/SnapshotViewIOS.ios');
 const URIActionMap = require('./utils/URIActionMap');

+import NativeDevMenu from '../../Libraries/NativeModules/specs/NativeDevMenu';
+
 const {
   AppRegistry,
   AsyncStorage,
@@ -143,6 +145,7 @@ class RNTesterApp extends React.Component<Props, RNTesterNavigationState> {

   UNSAFE_componentWillMount() {
     BackHandler.addEventListener('hardwareBackPress', this._handleBack);
+    NativeDevMenu.show();
   }

   componentDidMount() {
```

Reviewed By: sammy-SC

Differential Revision: D18945861

Pulled By: hramos

fbshipit-source-id: edcf02c5803742c89a845a3e5d72bc7dacae839f
osdnk pushed a commit to osdnk/react-native that referenced this pull request Mar 9, 2020
Summary:
This enables the dev menu to bo opened from keyboard shortcuts in dev from a Mac Catalyst app.

cc TheSavior andymatuschak radex

## Changelog

[iOS] [Fixed] - Enable dev keyboard shortcuts on Mac Catalyst
Pull Request resolved: facebook#27479

Test Plan:
It depends on facebook#27469 (to have working WebSocket in debug).

![image](https://user-images.githubusercontent.com/7189823/70629346-d3a68880-1bf7-11ea-8949-7553157a2f9c.png)

Differential Revision: D19576528

Pulled By: shergin

fbshipit-source-id: 32b4f8424fb7d270640af4bc50dba24f488bef4f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants