Skip to content

[iOS] Enabling RCTWebSocket on UIKitForMac (macOS Catalyst)#27469

Closed
andymatuschak wants to merge 1 commit into
react:masterfrom
andymatuschak:catalyst-rctwebsocket
Closed

[iOS] Enabling RCTWebSocket on UIKitForMac (macOS Catalyst)#27469
andymatuschak wants to merge 1 commit into
react:masterfrom
andymatuschak:catalyst-rctwebsocket

Conversation

@andymatuschak

Copy link
Copy Markdown
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
Copy Markdown
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 cla@fb.com. 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
Copy Markdown
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
Copy Markdown
Contributor

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

} else if (dataSize >= 2) {
[data getBytes:&closeCode length:sizeof(closeCode)];
_closeCode = EndianU16_BtoN(closeCode);
_closeCode = NSSwapBigShortToHost(closeCode);

Copy link
Copy Markdown
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

radex commented Dec 10, 2019

Copy link
Copy Markdown
Contributor

Thank you @andymatuschak for this!

@charpeni charpeni left a comment

Copy link
Copy Markdown
Contributor

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.

@elicwhite

elicwhite commented Dec 11, 2019

Copy link
Copy Markdown
Contributor

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
Copy Markdown

Awesome work indeed!

@charpeni

Copy link
Copy Markdown
Contributor

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

@hramos

hramos commented Dec 12, 2019

Copy link
Copy Markdown
Contributor

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.

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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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 8245a68..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: react#27479

Test Plan:
It depends on react#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.

8 participants